From: Eric Dumazet <hidden> Date: 2011-01-26 17:22:03
mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
initialized.
Signed-off-by: Eric Dumazet <redacted>
CC: John Fastabend <redacted>
---
net/sched/sch_mqprio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Joe Perches <joe@perches.com> Date: 2011-01-26 17:43:44
On Wed, 2011-01-26 at 18:21 +0100, Eric Dumazet wrote:
quoted hunk
mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
initialized.
Signed-off-by: Eric Dumazet <redacted>
CC: John Fastabend <redacted>
---
net/sched/sch_mqprio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: David Miller <davem@davemloft.net> Date: 2011-01-26 19:54:55
From: Joe Perches <joe@perches.com>
Date: Wed, 26 Jan 2011 09:43:43 -0800
On Wed, 2011-01-26 at 18:21 +0100, Eric Dumazet wrote:
quoted
mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
initialized.
Signed-off-by: Eric Dumazet <redacted>
CC: John Fastabend <redacted>
---
net/sched/sch_mqprio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I think the best style to use memset so that any
possible struct padding is guaranteed to be zeroed.
Such padding does not exist, and we won't add such padding since this is
a user visible data structure and thus whose layout is cast in stone.
Anyways, I'm ambivalent to how this is fixed actually.
From: Eric Dumazet <hidden> Date: 2011-01-26 20:23:15
Le mercredi 26 janvier 2011 à 11:55 -0800, David Miller a écrit :
From: Joe Perches <joe@perches.com>
quoted
I think the best style to use memset so that any
possible struct padding is guaranteed to be zeroed.
Such padding does not exist, and we won't add such padding since this is
a user visible data structure and thus whose layout is cast in stone.
Anyways, I'm ambivalent to how this is fixed actually.
I am perfectly aware of this ugly memset() style some people prefer, and
5 % of the time they swap 2nd and 3rd param.
Two patches instead of one ;)
In this particular case, I only used existing codestyle: In the same
file I noticed :
vi +322 net/sched/sch_mqprio.c
static int mqprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
struct gnet_dump *d)
{
struct net_device *dev = qdisc_dev(sch);
if (cl <= netdev_get_num_tc(dev)) {
int i;
struct Qdisc *qdisc;
struct gnet_stats_queue qstats = {0};
struct gnet_stats_basic_packed bstats = {0};
struct netdev_tc_txq tc = dev->tc_to_txq[cl - 1];
From: David Miller <davem@davemloft.net> Date: 2011-01-26 21:15:10
From: Eric Dumazet <redacted>
Date: Wed, 26 Jan 2011 18:21:57 +0100
mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
initialized.
Signed-off-by: Eric Dumazet <redacted>
CC: John Fastabend <redacted>
From: Joe Perches <joe@perches.com> Date: 2011-01-26 21:25:00
On Wed, 2011-01-26 at 21:23 +0100, Eric Dumazet wrote:
Le mercredi 26 janvier 2011 à 11:55 -0800, David Miller a écrit :
quoted
From: Joe Perches <joe@perches.com>
quoted
I think the best style to use memset so that any
possible struct padding is guaranteed to be zeroed.
Such padding does not exist, and we won't add such padding since this is
a user visible data structure and thus whose layout is cast in stone.
Anyways, I'm ambivalent to how this is fixed actually.
I am perfectly aware of this ugly memset() style some people prefer, and
5 % of the time they swap 2nd and 3rd param.
Ugly maybe, but correct, definitely.
The same can not be said of the {0}.
This use ends up the same so it doesn't
matter here, it's just a style question.
Two patches instead of one ;)
How uninteresting. Any API can be misused.
In this particular case, I only used existing codestyle: In the same
file I noticed :
There are memset's in the file as well.
In fact, memset is used in the same function.
cheers, Joe
From: Joe Perches <joe@perches.com> Date: 2011-01-26 21:33:29
On Wed, 2011-01-26 at 22:28 +0100, Eric Dumazet wrote:
Le mercredi 26 janvier 2011 à 13:24 -0800, Joe Perches a écrit :
quoted
Ugly maybe, but correct, definitely.
The same can not be said of the {0}.
What about fixing real problems Joe ?
What about it? You seem to have fixed it.
Are you telling me I dont know C ?
All I'm saying is that from a style and auditing
perspective, it's better to use memset for all
structs that are exposed to user space.
Other than that, there's no issue here.
cheers, Joe
From: Joe Perches <joe@perches.com> Date: 2011-01-27 06:04:12
On Wed, 2011-01-26 at 11:55 -0800, David Miller wrote:
From: Joe Perches <joe@perches.com>
Date: Wed, 26 Jan 2011 09:43:43 -0800
quoted
On Wed, 2011-01-26 at 18:21 +0100, Eric Dumazet wrote:
quoted
mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
initialized.
Signed-off-by: Eric Dumazet <redacted>
CC: John Fastabend <redacted>
---
net/sched/sch_mqprio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I think the best style to use memset so that any
possible struct padding is guaranteed to be zeroed.
Such padding does not exist, and we won't add such padding since this is
a user visible data structure and thus whose layout is cast in stone.
/* MQPRIO */
#define TC_QOPT_BITMASK 15
#define TC_QOPT_MAX_QUEUE 16
struct tc_mqprio_qopt {
__u8 num_tc;
__u8 prio_tc_map[TC_QOPT_BITMASK + 1];
__u8 hw;
__u16 count[TC_QOPT_MAX_QUEUE];
__u16 offset[TC_QOPT_MAX_QUEUE];
};
I believe this struct needs to be declared __packed.
It could otherwise be 24 bytes not 22.
Or if char array declarations have a different
alignment requirement, could be any size.
memset is better than {0}.
cheers, Joe
On Thu, Jan 27, 2011 at 2:04 PM, Joe Perches [off-list ref] wrote:
On Wed, 2011-01-26 at 11:55 -0800, David Miller wrote:
quoted
From: Joe Perches <joe@perches.com>
Date: Wed, 26 Jan 2011 09:43:43 -0800
quoted
On Wed, 2011-01-26 at 18:21 +0100, Eric Dumazet wrote:
quoted
mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
initialized.
Signed-off-by: Eric Dumazet <redacted>
CC: John Fastabend <redacted>
---
net/sched/sch_mqprio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I think the best style to use memset so that any
possible struct padding is guaranteed to be zeroed.
Such padding does not exist, and we won't add such padding since this is
a user visible data structure and thus whose layout is cast in stone.
/* MQPRIO */
#define TC_QOPT_BITMASK 15
#define TC_QOPT_MAX_QUEUE 16
struct tc_mqprio_qopt {
__u8 num_tc; // 1
__u8 prio_tc_map[TC_QOPT_BITMASK + 1]; // 16
__u8 hw; // 1
__u16 count[TC_QOPT_MAX_QUEUE]; // 32
__u16 offset[TC_QOPT_MAX_QUEUE]; //32
};
I believe this struct needs to be declared __packed.
It could otherwise be 24 bytes not 22.
Or if char array declarations have a different
alignment requirement, could be any size.
The total size is 1 + 16 + 1 + 32 + 32 = 82.
How do you get 24 or 22?
From: Eric Dumazet <hidden> Date: 2011-01-27 07:04:10
Le mercredi 26 janvier 2011 à 22:04 -0800, Joe Perches a écrit :
/* MQPRIO */
#define TC_QOPT_BITMASK 15
#define TC_QOPT_MAX_QUEUE 16
struct tc_mqprio_qopt {
__u8 num_tc;
__u8 prio_tc_map[TC_QOPT_BITMASK + 1];
__u8 hw;
__u16 count[TC_QOPT_MAX_QUEUE];
__u16 offset[TC_QOPT_MAX_QUEUE];
};
I believe this struct needs to be declared __packed.
Oh my god. Yet another ugly thing.
It could otherwise be 24 bytes not 22.
22 ? You are kidding probably. Its 82 exactly.
Listen, I doubled check my patch, its good, while your rants are lazy.
Or if char array declarations have a different
alignment requirement, could be any size.
If if if... could could could...
memset is better than {0}.
You never stop do you ?
The bigger object is u16, therefore alignof() is 2, not 4
No ABI requires a short (u16) is aligned on 4 byte boundary.
If you find a compiler not respecting this, you can bet linux wont run
at all if compiled with it. mqprio 'potential 2 bytes leak' will hardly
be a problem.
and there is no padding in this
structure, I checked this point.
That may be true right now for this particular
structure, but that style is not future-proof.
I think {0,} is a valid init style, because the handling
of the initialization is special and overrides the point
mentioned in the link above.
cheers,
Pádraig.