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

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

[Eli Britstein]

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@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.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_ETHERNET_FIELDS \
+    OVS_KEY_FIELD_ARR(__u8, eth_src, ETH_ALEN) \
+    OVS_KEY_FIELD_ARR(__u8, eth_dst, ETH_ALEN)
+
 struct ovs_key_ethernet {
-	__u8	 eth_src[ETH_ALEN];
-	__u8	 eth_dst[ETH_ALEN];
+    OVS_KEY_ETHERNET_FIELDS
 };
 
 struct ovs_key_mpls {
 	__be32 mpls_lse;
 };
 
+#define OVS_KEY_IPV4_FIELDS \
+    OVS_KEY_FIELD(__be32, ipv4_src) \
+    OVS_KEY_FIELD(__be32, ipv4_dst) \
+    OVS_KEY_FIELD(__u8, ipv4_proto) \
+    OVS_KEY_FIELD(__u8, ipv4_tos) \
+    OVS_KEY_FIELD(__u8, ipv4_ttl) \
+    OVS_KEY_FIELD(__u8, ipv4_frag /* One of OVS_FRAG_TYPE_*. */)
+
 struct ovs_key_ipv4 {
-	__be32 ipv4_src;
-	__be32 ipv4_dst;
-	__u8   ipv4_proto;
-	__u8   ipv4_tos;
-	__u8   ipv4_ttl;
-	__u8   ipv4_frag;	/* One of OVS_FRAG_TYPE_*. */
+    OVS_KEY_IPV4_FIELDS
 };
 
+#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
 };
 
+#define OVS_KEY_TCP_FIELDS \
+    OVS_KEY_FIELD(__be16, tcp_src) \
+    OVS_KEY_FIELD(__be16, tcp_dst)
+
 struct ovs_key_tcp {
-	__be16 tcp_src;
-	__be16 tcp_dst;
+    OVS_KEY_TCP_FIELDS
 };
 
+#define OVS_KEY_UDP_FIELDS \
+    OVS_KEY_FIELD(__be16, udp_src) \
+    OVS_KEY_FIELD(__be16, udp_dst)
+
 struct ovs_key_udp {
-	__be16 udp_src;
-	__be16 udp_dst;
+    OVS_KEY_UDP_FIELDS
 };
 
+#define OVS_KEY_SCTP_FIELDS \
+    OVS_KEY_FIELD(__be16, sctp_src) \
+    OVS_KEY_FIELD(__be16, sctp_dst)
+
 struct ovs_key_sctp {
-	__be16 sctp_src;
-	__be16 sctp_dst;
+    OVS_KEY_SCTP_FIELDS
 };
 
+#define OVS_KEY_ICMP_FIELDS \
+    OVS_KEY_FIELD(__u8, icmp_type) \
+    OVS_KEY_FIELD(__u8, icmp_code)
+
 struct ovs_key_icmp {
-	__u8 icmp_type;
-	__u8 icmp_code;
+    OVS_KEY_ICMP_FIELDS
 };
 
+#define OVS_KEY_ICMPV6_FIELDS \
+    OVS_KEY_FIELD(__u8, icmpv6_type) \
+    OVS_KEY_FIELD(__u8, icmpv6_code)
+
 struct ovs_key_icmpv6 {
-	__u8 icmpv6_type;
-	__u8 icmpv6_code;
+    OVS_KEY_ICMPV6_FIELDS
 };
 
+#define OVS_KEY_ARP_FIELDS \
+    OVS_KEY_FIELD(__be32, arp_sip) \
+    OVS_KEY_FIELD(__be32, arp_tip) \
+    OVS_KEY_FIELD(__be16, arp_op) \
+    OVS_KEY_FIELD_ARR(__u8, arp_sha, ETH_ALEN) \
+    OVS_KEY_FIELD_ARR(__u8, arp_tha, ETH_ALEN)
+
 struct ovs_key_arp {
-	__be32 arp_sip;
-	__be32 arp_tip;
-	__be16 arp_op;
-	__u8   arp_sha[ETH_ALEN];
-	__u8   arp_tha[ETH_ALEN];
+    OVS_KEY_ARP_FIELDS
 };
 
+#define OVS_KEY_ND_FIELDS \
+    OVS_KEY_FIELD_ARR(__be32, nd_target, 4) \
+    OVS_KEY_FIELD_ARR(__u8, nd_sll, ETH_ALEN) \
+    OVS_KEY_FIELD_ARR(__u8, nd_tll, ETH_ALEN)
+
 struct ovs_key_nd {
-	__be32	nd_target[4];
-	__u8	nd_sll[ETH_ALEN];
-	__u8	nd_tll[ETH_ALEN];
+    OVS_KEY_ND_FIELDS
 };
 
+#undef OVS_KEY_FIELD_ARR
+#undef OVS_KEY_FIELD
+
 #define OVS_CT_LABELS_LEN_32	4
 #define OVS_CT_LABELS_LEN	(OVS_CT_LABELS_LEN_32 * sizeof(__u32))
 struct ovs_key_ct_labels {
-- 
2.14.5

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

[Pravin Shelar]

On Sun, Feb 3, 2019 at 1:12 AM Eli Britstein <elibr@mellanox.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@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>


Thanks.
Acked-by: Pravin B Shelar <pshelar@ovn.org>

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

[Yi-Hung Wei]

On Sun, Feb 3, 2019 at 1:13 AM Eli Britstein <elibr@mellanox.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@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.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

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

[Gregory Rose]

On 2/3/2019 1:12 AM, Eli Britstein 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@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>

Obscuring the structures with these macros is awful.  I'm opposed but I 
see it has already been
accepted upstream so I guess that's that.

Ugh...

- Greg

> ---
>   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_ETHERNET_FIELDS \
> +    OVS_KEY_FIELD_ARR(__u8, eth_src, ETH_ALEN) \
> +    OVS_KEY_FIELD_ARR(__u8, eth_dst, ETH_ALEN)
> +
>   struct ovs_key_ethernet {
> -	__u8	 eth_src[ETH_ALEN];
> -	__u8	 eth_dst[ETH_ALEN];
> +    OVS_KEY_ETHERNET_FIELDS
>   };
>   
>   struct ovs_key_mpls {
>   	__be32 mpls_lse;
>   };
>   
> +#define OVS_KEY_IPV4_FIELDS \
> +    OVS_KEY_FIELD(__be32, ipv4_src) \
> +    OVS_KEY_FIELD(__be32, ipv4_dst) \
> +    OVS_KEY_FIELD(__u8, ipv4_proto) \
> +    OVS_KEY_FIELD(__u8, ipv4_tos) \
> +    OVS_KEY_FIELD(__u8, ipv4_ttl) \
> +    OVS_KEY_FIELD(__u8, ipv4_frag /* One of OVS_FRAG_TYPE_*. */)
> +
>   struct ovs_key_ipv4 {
> -	__be32 ipv4_src;
> -	__be32 ipv4_dst;
> -	__u8   ipv4_proto;
> -	__u8   ipv4_tos;
> -	__u8   ipv4_ttl;
> -	__u8   ipv4_frag;	/* One of OVS_FRAG_TYPE_*. */
> +    OVS_KEY_IPV4_FIELDS
>   };
>   
> +#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
>   };
>   
> +#define OVS_KEY_TCP_FIELDS \
> +    OVS_KEY_FIELD(__be16, tcp_src) \
> +    OVS_KEY_FIELD(__be16, tcp_dst)
> +
>   struct ovs_key_tcp {
> -	__be16 tcp_src;
> -	__be16 tcp_dst;
> +    OVS_KEY_TCP_FIELDS
>   };
>   
> +#define OVS_KEY_UDP_FIELDS \
> +    OVS_KEY_FIELD(__be16, udp_src) \
> +    OVS_KEY_FIELD(__be16, udp_dst)
> +
>   struct ovs_key_udp {
> -	__be16 udp_src;
> -	__be16 udp_dst;
> +    OVS_KEY_UDP_FIELDS
>   };
>   
> +#define OVS_KEY_SCTP_FIELDS \
> +    OVS_KEY_FIELD(__be16, sctp_src) \
> +    OVS_KEY_FIELD(__be16, sctp_dst)
> +
>   struct ovs_key_sctp {
> -	__be16 sctp_src;
> -	__be16 sctp_dst;
> +    OVS_KEY_SCTP_FIELDS
>   };
>   
> +#define OVS_KEY_ICMP_FIELDS \
> +    OVS_KEY_FIELD(__u8, icmp_type) \
> +    OVS_KEY_FIELD(__u8, icmp_code)
> +
>   struct ovs_key_icmp {
> -	__u8 icmp_type;
> -	__u8 icmp_code;
> +    OVS_KEY_ICMP_FIELDS
>   };
>   
> +#define OVS_KEY_ICMPV6_FIELDS \
> +    OVS_KEY_FIELD(__u8, icmpv6_type) \
> +    OVS_KEY_FIELD(__u8, icmpv6_code)
> +
>   struct ovs_key_icmpv6 {
> -	__u8 icmpv6_type;
> -	__u8 icmpv6_code;
> +    OVS_KEY_ICMPV6_FIELDS
>   };
>   
> +#define OVS_KEY_ARP_FIELDS \
> +    OVS_KEY_FIELD(__be32, arp_sip) \
> +    OVS_KEY_FIELD(__be32, arp_tip) \
> +    OVS_KEY_FIELD(__be16, arp_op) \
> +    OVS_KEY_FIELD_ARR(__u8, arp_sha, ETH_ALEN) \
> +    OVS_KEY_FIELD_ARR(__u8, arp_tha, ETH_ALEN)
> +
>   struct ovs_key_arp {
> -	__be32 arp_sip;
> -	__be32 arp_tip;
> -	__be16 arp_op;
> -	__u8   arp_sha[ETH_ALEN];
> -	__u8   arp_tha[ETH_ALEN];
> +    OVS_KEY_ARP_FIELDS
>   };
>   
> +#define OVS_KEY_ND_FIELDS \
> +    OVS_KEY_FIELD_ARR(__be32, nd_target, 4) \
> +    OVS_KEY_FIELD_ARR(__u8, nd_sll, ETH_ALEN) \
> +    OVS_KEY_FIELD_ARR(__u8, nd_tll, ETH_ALEN)
> +
>   struct ovs_key_nd {
> -	__be32	nd_target[4];
> -	__u8	nd_sll[ETH_ALEN];
> -	__u8	nd_tll[ETH_ALEN];
> +    OVS_KEY_ND_FIELDS
>   };
>   
> +#undef OVS_KEY_FIELD_ARR
> +#undef OVS_KEY_FIELD
> +
>   #define OVS_CT_LABELS_LEN_32	4
>   #define OVS_CT_LABELS_LEN	(OVS_CT_LABELS_LEN_32 * sizeof(__u32))
>   struct ovs_key_ct_labels {

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

[David Miller]

From: Yi-Hung Wei <yihung.wei@gmail.com>
Date: Mon, 4 Feb 2019 10:47:18 -0800

> 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.

I completely agree.

Unless this is totally unavoidable, I do not want to apply a patch
which makes reading and auditing the networking code more difficult.

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

[David Miller]

From: Gregory Rose <gvrose8192@gmail.com>
Date: Mon, 4 Feb 2019 11:41:29 -0800

> 
> On 2/3/2019 1:12 AM, Eli Britstein 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@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
> 
> Obscuring the structures with these macros is awful.  I'm opposed but
> I see it has already been
> accepted upstream so I guess that's that.

I am personally in no way obligated to apply this patch to my tree
just because "upstream" did, and I absolutely have no plans to do so
at this point.

This patch is absolutely awful.

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

[Eli Britstein]

On 2/4/2019 10:07 PM, David Miller wrote:
> From: Yi-Hung Wei <yihung.wei@gmail.com>
> Date: Mon, 4 Feb 2019 10:47:18 -0800
>
>> 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.
> I completely agree.
>
> Unless this is totally unavoidable, I do not want to apply a patch
> which makes reading and auditing the networking code more difficult.
This technique is discussed for example in 
https://stackoverflow.com/questions/6635851/real-world-use-of-x-macros, 
and I found existing examples of using it in the kernel tree:

__BPF_FUNC_MAPPER in commit ebb676daa1a34 ("bpf: Print function name in 
addition to function id")

__AAL_STAT_ITEMS and __SONET_ITEMS in commit 607ca46e97a1b ("UAPI: 
(Scripted) Disintegrate include/linux"), the successor of commit 
1da177e4c3f4 ("Linux-2.6.12-rc2"). I didn't dig deeper to the past.

I can agree it makes that H file a bit more complicated, but for sure 
less than ## macros that are widely used.

However, I think the alternatives of generating such defines by some 
scripts, or having the fields in more than one place are even worse, so 
it is a kind of unavoidable.

Please reconsider regarding applying this patch.

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

[Gregory Rose]

On 2/5/2019 4:02 AM, Eli Britstein wrote:
> On 2/4/2019 10:07 PM, David Miller wrote:
>> From: Yi-Hung Wei <yihung.wei@gmail.com>
>> Date: Mon, 4 Feb 2019 10:47:18 -0800
>>
>>> 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.
>> I completely agree.
>>
>> Unless this is totally unavoidable, I do not want to apply a patch
>> which makes reading and auditing the networking code more difficult.
> This technique is discussed for example in
> https://stackoverflow.com/questions/6635851/real-world-use-of-x-macros,
> and I found existing examples of using it in the kernel tree:
>
> __BPF_FUNC_MAPPER in commit ebb676daa1a34 ("bpf: Print function name in
> addition to function id")
>
> __AAL_STAT_ITEMS and __SONET_ITEMS in commit 607ca46e97a1b ("UAPI:
> (Scripted) Disintegrate include/linux"), the successor of commit
> 1da177e4c3f4 ("Linux-2.6.12-rc2"). I didn't dig deeper to the past.
>
> I can agree it makes that H file a bit more complicated, but for sure
> less than ## macros that are widely used.
>
> However, I think the alternatives of generating such defines by some
> scripts, or having the fields in more than one place are even worse, so
> it is a kind of unavoidable.

Why is using a script to generate defines for the requirements of your 
application or driver so bad?  Your patch
turns openvswitch.h into some hardly readable code - using a script and 
having a machine output the macros
your application or driver needs seems like a much better alternative to me.

- Greg

>
> Please reconsider regarding applying this patch.
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ben Pfaff]

On Tue, Feb 05, 2019 at 10:22:09AM -0800, Gregory Rose wrote:
> 
> On 2/5/2019 4:02 AM, Eli Britstein wrote:
> > On 2/4/2019 10:07 PM, David Miller wrote:
> > > From: Yi-Hung Wei <yihung.wei@gmail.com>
> > > Date: Mon, 4 Feb 2019 10:47:18 -0800
> > > 
> > > > 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.
> > > I completely agree.
> > > 
> > > Unless this is totally unavoidable, I do not want to apply a patch
> > > which makes reading and auditing the networking code more difficult.
> > This technique is discussed for example in
> > https://stackoverflow.com/questions/6635851/real-world-use-of-x-macros,
> > and I found existing examples of using it in the kernel tree:
> > 
> > __BPF_FUNC_MAPPER in commit ebb676daa1a34 ("bpf: Print function name in
> > addition to function id")
> > 
> > __AAL_STAT_ITEMS and __SONET_ITEMS in commit 607ca46e97a1b ("UAPI:
> > (Scripted) Disintegrate include/linux"), the successor of commit
> > 1da177e4c3f4 ("Linux-2.6.12-rc2"). I didn't dig deeper to the past.
> > 
> > I can agree it makes that H file a bit more complicated, but for sure
> > less than ## macros that are widely used.
> > 
> > However, I think the alternatives of generating such defines by some
> > scripts, or having the fields in more than one place are even worse, so
> > it is a kind of unavoidable.
> 
> Why is using a script to generate defines for the requirements of your
> application or driver so bad?  Your patch
> turns openvswitch.h into some hardly readable code - using a script and
> having a machine output the macros
> your application or driver needs seems like a much better alternative to me.

In addition, one of the reasons that developers prefer to avoid
duplication is because of the need to synchronize copies when one of
them changes.  This doesn't apply in the same way to these structures,
because they are ABIs that will not change.

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

[Eli Britstein]

On 2/5/2019 10:23 PM, Ben Pfaff wrote:
> On Tue, Feb 05, 2019 at 10:22:09AM -0800, Gregory Rose wrote:
>> On 2/5/2019 4:02 AM, Eli Britstein wrote:
>>> On 2/4/2019 10:07 PM, David Miller wrote:
>>>> From: Yi-Hung Wei <yihung.wei@gmail.com>
>>>> Date: Mon, 4 Feb 2019 10:47:18 -0800
>>>>
>>>>> 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.
>>>> I completely agree.
>>>>
>>>> Unless this is totally unavoidable, I do not want to apply a patch
>>>> which makes reading and auditing the networking code more difficult.
>>> This technique is discussed for example in
>>> https://stackoverflow.com/questions/6635851/real-world-use-of-x-macros,
>>> and I found existing examples of using it in the kernel tree:
>>>
>>> __BPF_FUNC_MAPPER in commit ebb676daa1a34 ("bpf: Print function name in
>>> addition to function id")
>>>
>>> __AAL_STAT_ITEMS and __SONET_ITEMS in commit 607ca46e97a1b ("UAPI:
>>> (Scripted) Disintegrate include/linux"), the successor of commit
>>> 1da177e4c3f4 ("Linux-2.6.12-rc2"). I didn't dig deeper to the past.
>>>
>>> I can agree it makes that H file a bit more complicated, but for sure
>>> less than ## macros that are widely used.
>>>
>>> However, I think the alternatives of generating such defines by some
>>> scripts, or having the fields in more than one place are even worse, so
>>> it is a kind of unavoidable.
>> Why is using a script to generate defines for the requirements of your
>> application or driver so bad?  Your patch
>> turns openvswitch.h into some hardly readable code - using a script and
>> having a machine output the macros
>> your application or driver needs seems like a much better alternative to me.
OK, let's abandon this patch. I'll go with the script alternative.
> In addition, one of the reasons that developers prefer to avoid
> duplication is because of the need to synchronize copies when one of
> them changes.  This doesn't apply in the same way to these structures,
> because they are ABIs that will not change.
This is correct, but still, though it is not likely to change, it might 
will, so I think we must avoid duplication.

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

[Simon Horman]

On Mon, Feb 04, 2019 at 12:09:00PM -0800, David Miller wrote:
> From: Gregory Rose <gvrose8192@gmail.com>
> Date: Mon, 4 Feb 2019 11:41:29 -0800
> 
> > 
> > On 2/3/2019 1:12 AM, Eli Britstein 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@mellanox.com>
> >> Reviewed-by: Roi Dayan <roid@mellanox.com>
> > 
> > Obscuring the structures with these macros is awful.  I'm opposed but
> > I see it has already been
> > accepted upstream so I guess that's that.
> 
> I am personally in no way obligated to apply this patch to my tree
> just because "upstream" did, and I absolutely have no plans to do so
> at this point.
> 
> This patch is absolutely awful.

I hate to jump on a bandwagon, but this patch makes the code much
less readable.

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

[Eli Britstein]

On 2/8/2019 9:59 AM, Simon Horman wrote:
> On Mon, Feb 04, 2019 at 12:09:00PM -0800, David Miller wrote:
>> From: Gregory Rose <gvrose8192@gmail.com>
>> Date: Mon, 4 Feb 2019 11:41:29 -0800
>>
>>> On 2/3/2019 1:12 AM, Eli Britstein 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@mellanox.com>
>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>>> Obscuring the structures with these macros is awful.  I'm opposed but
>>> I see it has already been
>>> accepted upstream so I guess that's that.
>> I am personally in no way obligated to apply this patch to my tree
>> just because "upstream" did, and I absolutely have no plans to do so
>> at this point.
>>
>> This patch is absolutely awful.
> I hate to jump on a bandwagon, but this patch makes the code much
> less readable.

Please review the alternative I have posted:

https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356000.html