| Message ID | 20170420113540.GC1886@nanopsycho.orion |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 17-04-20 07:35 AM, Jiri Pirko wrote: > Thu, Apr 20, 2017 at 12:42:55PM CEST, jhs@mojatatu.com wrote: >> On 17-04-19 12:17 PM, Jiri Pirko wrote: > > Ha. So the current code is wrong, I see it now. Following is needed: > > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index 82b1d48..c432b22 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -1005,7 +1005,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n, > !netlink_capable(skb, CAP_NET_ADMIN)) > return -EPERM; > > - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL, > + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL, > extack); > if (ret < 0) > return ret; > > You confused me squashing the change to this patch. > > Ok, so the name could be: > TCA_ACTS_* > ? > I believe it is crucial to figure this out right now. TC UAPI is in deep > mess already, no need to push it even more. > I could change that name. Look at the last series i sent, add any extra comments to that and i will get to it by tomorrow. >> They are not the same issue Jiri. We have used bitmasks fine on netlink > > Howcome they are not the same? I'm pretty certain they are. > They are not Jiri. I challenge you to point me to one netlink bitmask representation used that has some bits that were not being used that someone or some compiler is going to set some random values on. Typically flags are u64/32/16 > >> message for a millenia. Nobody sets garbage on a bitmask they are not >> supposed to touch. The struct padding thing is a shame the way it >> turned out - now netlink can no longer have a claim to be a (good) >> wire protocol. >> Other thing: I know you feel strongly about this but I dont agree that >> everything has to be a TLV and in no way, as a matter of principle, you are >> going to convince me (even when the cows come home) that I have to >> use 64 bits to carry a single bit just so I can use TLVs. > > Then I guess we have to agree to disagree. I believe that your approach > is wrong and will break users in future when you add even a single flag. > Argument that "we are doing this for ages-therefore it is correct" has > simply 0 value. > "doing these for 80 years" is not the main driving point. I appreciate that data structures - even when fields are annotated as "pads" there is no guarantee when i allocate one the pads will be zeroes. Bitmasks are atomic in nature and therefore used in whole - not complex like structs. Thats the difference. I dont define this u32 bitmap that has 8 bits that are "pads" because that concept doesnt make sense for atomic fields. This implies that 6 months down the road I dont regret and say "shit, I need 2 more bits for flagbits but some idiot or compiler was setting those fields so i cant use them now because the kernel wont tell the difference between what s/he is doing and real values". Allocations of data structures I can understand, unless we enforce always reset to zeroes all the struct members as part of creation. But lets agree to disagree if you are still not convinced. On your "everything must be a TLV". Caveat is: there is a balance between performance and flexibility. I should be able to pack aligned complex data structures in a TLV for performance reasons. One lesson is: in the future we can make these extra service header like tcamsg very small and always use all their fields instead of defining pads. cheers, jamal
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 82b1d48..c432b22 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1005,7 +1005,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n, !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL, + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL, extack); if (ret < 0) return ret;