netdev - Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG1aQhJVzbYnYNb-mWH4s6+23jOnDz9UVYTPtBmNkhKAfowrQg@mail.gmail.com>
Date:   Mon, 4 Feb 2019 10:47:18 -0800
From:   Yi-Hung Wei <yihung.wei@...il.com>
To:     Eli Britstein <elibr@...lanox.com>
Cc:     Pravin B Shelar <pshelar@....org>, ovs dev <dev@...nvswitch.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Simon Horman <simon.horman@...ronome.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key
 structures using macros

On Sun, Feb 3, 2019 at 1:13 AM Eli Britstein <elibr@...lanox.com> wrote:
>
> Declare ovs key structures using macros as a pre-step towards to
> enable retrieving fields information, as a work done in proposed
> commit in the OVS tree https://patchwork.ozlabs.org/patch/1023406/
> ("odp-util: Do not rewrite fields with the same values as matched"),
> with no functional change.
>
> Signed-off-by: Eli Britstein <elibr@...lanox.com>
> Reviewed-by: Roi Dayan <roid@...lanox.com>
> ---
>  include/uapi/linux/openvswitch.h | 102 ++++++++++++++++++++++++++-------------
>  1 file changed, 69 insertions(+), 33 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index dbe0cbe4f1b7..dc8246f871fd 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -387,73 +387,109 @@ enum ovs_frag_type {
>
>  #define OVS_FRAG_TYPE_MAX (__OVS_FRAG_TYPE_MAX - 1)
>
> +#define OVS_KEY_FIELD_ARR(type, name, elements) type name[elements];
> +#define OVS_KEY_FIELD(type, name) type name;
......
> +#define OVS_KEY_IPV6_FIELDS \
> +    OVS_KEY_FIELD_ARR(__be32, ipv6_src, 4) \
> +    OVS_KEY_FIELD_ARR(__be32, ipv6_dst, 4) \
> +    OVS_KEY_FIELD(__be32, ipv6_label /* 20-bits in least-significant bits. */) \
> +    OVS_KEY_FIELD(__u8, ipv6_proto) \
> +    OVS_KEY_FIELD(__u8, ipv6_tclass) \
> +    OVS_KEY_FIELD(__u8, ipv6_hlimit) \
> +    OVS_KEY_FIELD(__u8, ipv6_frag /* One of OVS_FRAG_TYPE_*. */)
> +
>  struct ovs_key_ipv6 {
> -       __be32 ipv6_src[4];
> -       __be32 ipv6_dst[4];
> -       __be32 ipv6_label;      /* 20-bits in least-significant bits. */
> -       __u8   ipv6_proto;
> -       __u8   ipv6_tclass;
> -       __u8   ipv6_hlimit;
> -       __u8   ipv6_frag;       /* One of OVS_FRAG_TYPE_*. */
> +    OVS_KEY_IPV6_FIELDS
>  };

Hi Eli,

Thanks for the patch.  In my personal opinion, I feel this patch makes
the header file harder to read.

For example, to see how 'struct ovs_key_ipv6' is defined, now we need
to trace how OVS_KEY_IPV6_FIELDS is defined, and how OVS_KEY_FIELD_ARR
and OVS_KEY_FIELD defined.  I think it makes the header file to be
more complicated.

There are also some discussion on ovs-dev mailing list about this
patch: https://patchwork.ozlabs.org/cover/1023404/

Thanks,

-Yi-Hung