RE: [RFC] ethdev: convert string initialization

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> Sent: Thursday, 1 August 2024 14.43
> 
> On 8/1/2024 12:29 PM, Morten Brørup wrote:
> >> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> >> Sent: Thursday, 1 August 2024 11.27
> >>
> >> gcc 15 experimental [1], with -Wextra flag, gives warning in variable
> >> initialization as string [2].
> >>
> >> The warning has a point when initialized variable is intended to use
> as
> >> string, since assignment is missing the required null terminator for
> >> this case. But warning is useless for our usecase.
> >>
> >> I don't know if this behaviour will change in gcc15, as it is still
> >> under development. But if not we may need to update our
> initialization.
> >>
> >> In this patch only updated a few instance to show the issue, there
> are
> >> many instances to fix, if we prefer to go this way.
> >> Other option is to disable warning but it can be useful for actual
> >> string usecases, so I prefer to keep it.
> >
> > Compiler warnings are here to help, so +1 for fixing the code.
> >
> >>
> >> [1]
> >> gcc (GCC) 15.0.0 20240801 (experimental)
> >>
> >> [2]
> >> ../lib/ethdev/rte_flow.h:906:36:
> >>   error: initializer-string for array of ‘unsigned char’ is too long
> >>         [-Werror=unterminated-string-initialization]
> >> 906 |         .hdr.dst_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> >>     |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> ../lib/ethdev/rte_flow.h:907:36:
> >>   error: initializer-string for array of ‘unsigned char’ is too long
> >>          [-Werror=unterminated-string-initialization]
> >> 907 |         .hdr.src_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> >>     |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> ../lib/ethdev/rte_flow.h:1009:25:
> >>   error: initializer-string for array of ‘unsigned char’ is too long
> >>          [-Werror=unterminated-string-initialization]
> >> 1009 |                         "\xff\xff\xff\xff\xff\xff\xff\xff"
> >>      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> ../lib/ethdev/rte_flow.h:1012:25:
> >>   error: initializer-string for array of ‘unsigned char’ is too long
> >>          [-Werror=unterminated-string-initialization]
> >> 1012 |                         "\xff\xff\xff\xff\xff\xff\xff\xff"
> >>      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> ../lib/ethdev/rte_flow.h:1135:20:
> >>   error: initializer-string for array of ‘unsigned char’ is too long
> >>          [-Werror=unterminated-string-initialization]
> >> 1135 |         .hdr.vni = "\xff\xff\xff",
> >>      |                    ^~~~~~~~~~~~~~
> >>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
> >> ---
> >>  lib/ethdev/rte_flow.h | 16 +++++++---------
> >>  1 file changed, 7 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> >> index f864578f806b..8b623974cd44 100644
> >> --- a/lib/ethdev/rte_flow.h
> >> +++ b/lib/ethdev/rte_flow.h
> >> @@ -903,8 +903,8 @@ struct rte_flow_item_eth {
> >>  /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
> >>  #ifndef __cplusplus
> >>  static const struct rte_flow_item_eth rte_flow_item_eth_mask = {
> >> -	.hdr.dst_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> >> -	.hdr.src_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> >> +	.hdr.dst_addr.addr_bytes = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
> >> +	.hdr.src_addr.addr_bytes = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
> >
> > Please define and use an RTE_ETHER_ADDR() macro like RTE_IPV4() [1]:
> >
> > /** Create Ethernet address */
> > #define RTE_ETHER_ADDR(a, b, c, d, e, f) \
> > 	(struct rte_ether_addr){{a, b, c, d, e, f}}
> >
> > Or even better, also add and use an RTE_ETHER_ADDR_BROADCAST
> definition like RTE_IPV4_BROADCAST [2]:
> >
> > #define RTE_ETHER_ADDR_BROADCAST RTE_ETHER_ADDR(0xff, 0xff, 0xff,
> 0xff, 0xff, 0xff)
> >
> > Then the above code could become:
> > +	.hdr.dst_addr = RTE_ETHER_ADDR_BROADCAST,
> > +	.hdr.src_addr = RTE_ETHER_ADDR_BROADCAST,
> >
> > On the other hand, if they are address masks, maybe using
> RTE_ETHER_ADDR(0xff, 0xff, 0xff, 0xff, 0xff, 0xff) would be more
> appropriate.
> >
> 
> Hi Morten,
> 
> These are all masks used for flow API, and as mentioned in the commit
> log there are bunch of them, not just the ones in the patch, I am not
> sure about creating a macro for each is necessary.

I consider an IP address mask as having the same "type" as an IP address; e.g. if using rte_be32_t for an IP address, I would use the same type for an IP address mask. 
Similarly for Ethernet address masks.

So I'm requesting that you use macros for the IP and Ethernet types. (If they are all masks, forget about the _BROADCAST definitions.)

It seems the existing code [4] already follows this principle.

[4]: https://elixir.bootlin.com/dpdk/v24.07/source/lib/ethdev/rte_flow.h#L965

Using specific types also gives us type checking, which reduces the risk of bugs.

And since the macros don't already exist for IPv6 and Ethernet addresses, I'm also requesting that you create them - coordinate with Robin regarding the IPv6 address type and macros.

The fields that do not have existing types can use arrays, such as you did for the .hdr.vni = { 0xff, 0xff, 0xff }.


Alternatively...

Use the arrays as you originally suggested.
Then we can revisit the need for specific IPv6 and Ethernet types here, as part of Robin's IPv6 address type series.
It might be more straight forward. :-)

Either way,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

> 
> 
> > [1]:
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/net/rte_ip.h#L67
> > [2]:
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/net/rte_ip.h#L111
> >
> >>  	.hdr.ether_type = RTE_BE16(0x0000),
> >>  };
> >>  #endif
> >> @@ -1005,12 +1005,10 @@ struct rte_flow_item_ipv6 {
> >>  #ifndef __cplusplus
> >>  static const struct rte_flow_item_ipv6 rte_flow_item_ipv6_mask = {
> >>  	.hdr = {
> >> -		.src_addr =
> >> -			"\xff\xff\xff\xff\xff\xff\xff\xff"
> >> -			"\xff\xff\xff\xff\xff\xff\xff\xff",
> >> -		.dst_addr =
> >> -			"\xff\xff\xff\xff\xff\xff\xff\xff"
> >> -			"\xff\xff\xff\xff\xff\xff\xff\xff",
> >> +		.src_addr = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> >> 0xff,
> >> +			      0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
> >> },
> >> +		.dst_addr = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> >> 0xff,
> >> +			      0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
> >> },
> >
> > Please define and use an RTE_IPV6() macro like RTE_IPV4() [1].
> > Or even better, add and use an RTE_IPV6_BROADCAST definition like
> RTE_IPV4_BROADCAST [2].
> >
> > (Same comment about maybe using RTE_IPV6() instead of
> RTE_IPV6_BROADCAST for masks being more appropriate.)
> >
> > Note: Robin is working on a series to introduce an IPv6 address type
> [3], so you should coordinate the definition of the IPV6
> macros/definitions with him.
> >
> > [3]:
> https://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35E9F5B1@smarts
> erver.smartshare.dk/
> >
> >>  	},
> >>  };
> >>  #endif
> >> @@ -1132,7 +1130,7 @@ struct rte_flow_item_vxlan {
> >>  /** Default mask for RTE_FLOW_ITEM_TYPE_VXLAN. */
> >>  #ifndef __cplusplus
> >>  static const struct rte_flow_item_vxlan rte_flow_item_vxlan_mask = {
> >> -	.hdr.vni = "\xff\xff\xff",
> >> +	.hdr.vni = { 0xff, 0xff, 0xff },
> >
> > Yes you your suggestion here. (No special type/macro required.)
> >
> >>  };
> >>  #endif
> >>
> >> --
> >> 2.43.0
> >
> > With suggested changes,
> >
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >