Hi Marcelo,
On Fri, Jan 25, 2019 at 12:32:31AM -0200, Marcelo Ricardo Leitner wrote:
> Hook on flow dissector's new interface on ConnTrack from previous patch.
>
> Signed-off-by: Marcelo Ricardo Leitner <mleit...@redhat.com>
> ---
> include/uapi/linux/pkt_cls.h | 9 +++++++++
> net/sched/cls_flower.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index
> 95d0db2a8350dffb1dd20816591f3b179913fb2e..ba1f3bc01b2fdfd810e37a2b3853a1da1f838acf
> 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -490,6 +490,15 @@ enum {
> TCA_FLOWER_KEY_PORT_DST_MIN, /* be16 */
> TCA_FLOWER_KEY_PORT_DST_MAX, /* be16 */
>
> + TCA_FLOWER_KEY_CT_ZONE, /* u16 */
> + TCA_FLOWER_KEY_CT_ZONE_MASK, /* u16 */
> + TCA_FLOWER_KEY_CT_STATE, /* u8 */
> + TCA_FLOWER_KEY_CT_STATE_MASK, /* u8 */
With the corresponding flow dissector patch this API is
exposing the contents of an instance of enum ip_conntrack_info
as an ABI for conntrack state.
I believe (after getting similar review for my geneve options macthing
patches for flower) that this exposes implementation details as an ABI
to a degree that is not desirable.
My suggested would be to define, say in the form of named bits,
an ABI, that describes the state information that is exposed.
These bits may not correspond directly to the implementation of
ip_conntrack_info.
I think there should also be some consideration of if a mask makes
sense for the state as, f.e. in the implementation of enum
ip_conntrack_info not all bit combinations are valid.
> + TCA_FLOWER_KEY_CT_MARK, /* u32 */
> + TCA_FLOWER_KEY_CT_MARK_MASK, /* u32 */
> + TCA_FLOWER_KEY_CT_LABEL, /* 128 bits */
> + TCA_FLOWER_KEY_CT_LABEL_MASK, /* 128 bits */
> +
> __TCA_FLOWER_MAX,
> };
...
