On 01/02/2019 15:23, Marcelo Leitner wrote:
> On Tue, Jan 29, 2019 at 10:02:01AM +0200, Paul Blakey wrote:
> ...
>> diff --git a/include/uapi/linux/tc_act/tc_ct.h
>> b/include/uapi/linux/tc_act/tc_ct.h
>> new file mode 100644
>> index 0000000..6dbd771
>> --- /dev/null
>> +++ b/include/uapi/linux/tc_act/tc_ct.h
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef __UAPI_TC_CT_H
>> +#define __UAPI_TC_CT_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/pkt_cls.h>
>> +
>> +#define TCA_ACT_CT 18
>> +
>> +struct tc_ct {
>> + tc_gen;
>> + __u16 zone;
>> + __u32 labels[4];
>> + __u32 labels_mask[4];
>> + __u32 mark;
>> + __u32 mark_mask;
>> + bool commit;
>
> This is one of the points that our implementations differs. You used a
> struct and wrapped it into TCA_CT_PARMS attribute, while I broke it up
> into several attributes.
>
> cls_flower and act_bpf, for example, doesn't use structs, but others
> do.
>
> Both have pros and cons and I imagine this topic probably was already
> discussed but I'm not aware of a recommendation. Do we have one?
I guess flower uses a netlink attribute per key attribute because
a lot of time, most of them won't be used, and you would send less.
we can have ct, ct + snat, ct + dnat, zone and mark.... a lot of this
won't be used sometimes.
Also you can't add nested attributes to the struct easily.
Also netlink attributes can be tested for existence, while a struct
would need a special non valid value, or another field to specify which
fields are used.
both are hard to test if a requested attribute was ignored, besides
checking the netlink echo or dumping the action back. if for example a
older kernel module and newer userspace uses a attribute above
enum TCA_CT_MAX (struct attributes also don't have max len, in nla_parse).
All in all, I think mostly netlink attributes would be better.
>
>> +};
>> +
>> +enum {
>> + TCA_CT_UNSPEC,
>> + TCA_CT_PARMS,
>> + TCA_CT_TM,
>> + TCA_CT_PAD,
>> + __TCA_CT_MAX
>> +};
>> +#define TCA_CT_MAX (__TCA_CT_MAX - 1)
>> +
>> +#endif /* __UAPI_TC_CT_H */
> ...
>
