diff mbox series

[RFC] ethdev: convert string initialization

Message ID 20240801092722.3732917-1-ferruh.yigit@amd.com (mailing list archive)
State Superseded
Delegated to: Ferruh Yigit
Headers
Series [RFC] ethdev: convert string initialization |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance pending Performance Testing pending
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-unit-arm64-testing pending Testing pending
ci/iol-unit-amd64-testing pending Testing pending
ci/iol-compile-amd64-testing pending Testing pending
ci/iol-abi-testing pending Testing pending
ci/iol-compile-arm64-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Ferruh Yigit Aug. 1, 2024, 9:27 a.m. UTC 
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(-)

Comments

Bruce Richardson Aug. 1, 2024, 10:33 a.m. UTC | #1 
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>
Morten Brørup Aug. 1, 2024, 11:29 a.m. UTC | #2 
> 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>
Ferruh Yigit Aug. 1, 2024, 12:43 p.m. UTC | #3 
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>
>
Morten Brørup Aug. 1, 2024, 1:29 p.m. UTC | #4 
> 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>
> >
Tyler Retzlaff Aug. 6, 2024, 5:54 a.m. UTC | #5 
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
Stephen Hemminger Oct. 4, 2024, 3:17 p.m. UTC | #6 
On Thu, 1 Aug 2024 02:27:22 -0700
Ferruh Yigit <ferruh.yigit@amd.com> 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)


I saw Robin added a bunch more of these in the ipv6 struct changes.
Ferruh Yigit Oct. 4, 2024, 5:58 p.m. UTC | #7 
On 10/4/2024 4:17 PM, Stephen Hemminger wrote:
> On Thu, 1 Aug 2024 02:27:22 -0700
> Ferruh Yigit <ferruh.yigit@amd.com> 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)
> 
> 
> I saw Robin added a bunch more of these in the ipv6 struct changes.
>

Thanks for the heads up, I will comment on his patch.
diff mbox series

Patch

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