* Re: [RFC] ethdev: convert string initialization
2024-08-01 9:27 [RFC] ethdev: convert string initialization Ferruh Yigit
@ 2024-08-01 10:33 ` Bruce Richardson
2024-08-01 11:29 ` Morten Brørup
2024-08-06 5:54 ` Tyler Retzlaff
2 siblings, 0 replies; 6+ messages in thread
From: Bruce Richardson @ 2024-08-01 10:33 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Thomas Monjalon, Ori Kam, Andrew Rybchenko, dev
On Thu, Aug 01, 2024 at 02:27:22AM -0700, Ferruh Yigit wrote:
> 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.
>
> [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 },
> .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 },
> },
> };
> #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 },
> };
> #endif
This solution LGTM.
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [RFC] ethdev: convert string initialization
2024-08-01 9:27 [RFC] ethdev: convert string initialization Ferruh Yigit
2024-08-01 10:33 ` Bruce Richardson
@ 2024-08-01 11:29 ` Morten Brørup
2024-08-01 12:43 ` Ferruh Yigit
2024-08-06 5:54 ` Tyler Retzlaff
2 siblings, 1 reply; 6+ messages in thread
From: Morten Brørup @ 2024-08-01 11:29 UTC (permalink / raw)
To: Ferruh Yigit, Thomas Monjalon, Ori Kam, Andrew Rybchenko, Robin Jarry; +Cc: dev
> 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.
[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@smartserver.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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC] ethdev: convert string initialization
2024-08-01 11:29 ` Morten Brørup
@ 2024-08-01 12:43 ` Ferruh Yigit
2024-08-01 13:29 ` Morten Brørup
0 siblings, 1 reply; 6+ messages in thread
From: Ferruh Yigit @ 2024-08-01 12:43 UTC (permalink / raw)
To: Morten Brørup, Thomas Monjalon, Ori Kam, Andrew Rybchenko,
Robin Jarry
Cc: dev
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.
> [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@smartserver.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>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [RFC] ethdev: convert string initialization
2024-08-01 12:43 ` Ferruh Yigit
@ 2024-08-01 13:29 ` Morten Brørup
0 siblings, 0 replies; 6+ messages in thread
From: Morten Brørup @ 2024-08-01 13:29 UTC (permalink / raw)
To: Ferruh Yigit, Thomas Monjalon, Ori Kam, Andrew Rybchenko, Robin Jarry; +Cc: dev
> 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>
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] ethdev: convert string initialization
2024-08-01 9:27 [RFC] ethdev: convert string initialization Ferruh Yigit
2024-08-01 10:33 ` Bruce Richardson
2024-08-01 11:29 ` Morten Brørup
@ 2024-08-06 5:54 ` Tyler Retzlaff
2 siblings, 0 replies; 6+ messages in thread
From: Tyler Retzlaff @ 2024-08-06 5:54 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Thomas Monjalon, Ori Kam, Andrew Rybchenko, dev
On Thu, Aug 01, 2024 at 02:27:22AM -0700, Ferruh Yigit wrote:
> 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.
i investigated this when msvc had some issues with it as well. we
determined that it is actually iso c standard compliant.
what basically happens is the characters of the string are copied up to
the destination size and any remaining characters from the string
literal (including the NUL) are discarded and that is what leads to
the warning.
yes, the copied bytes lost their NUL terminator but the destination
isn't a string so meh, whatever.
either way i support your fix, it deals with the problem with msvc as
well.
thank you!
>
> 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.
>
> [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 },
> .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 },
> },
> };
> #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 },
> };
> #endif
>
> --
> 2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread