Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 28/10/24 17:21, Jakub Kicinski wrote:
> On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:
>> Fix 3338 of the following -Wflex-array-member-not-at-end warnings:
>>
>> include/linux/ethtool.h:214:38: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> I don't see any change in the number of warnings with W=1:
> gcc (GCC) 14.2.1 20240912 (Red Hat 14.2.1-3)
> Is it only enabled with W=2?

-Wfamnae is not currently part of any upstream build. We are working
to have it enabled. So, these warnings are the ones I see in my local
build with the following patch applied:

diff --git a/Makefile b/Makefile
index d4a41c44e0fc..08d18b5d01f5 100644
--- a/Makefile
+++ b/Makefile
@@ -1002,6 +1002,9 @@ NOSTDINC_FLAGS += -nostdinc
  # perform bounds checking.
  KBUILD_CFLAGS += $(call cc-option, -fstrict-flex-arrays=3)

+# Avoid flexible-array members not at the end of composite structure.
+KBUILD_CFLAGS += $(call cc-option, -Wflex-array-member-not-at-end)
+
  #Currently, disable -Wstringop-overflow for GCC 11, globally.
  KBUILD_CFLAGS-$(CONFIG_CC_NO_STRINGOP_OVERFLOW) += $(call cc-option, -Wno-stringop-overflow)
  KBUILD_CFLAGS-$(CONFIG_CC_STRINGOP_OVERFLOW) += $(call cc-option, -Wstringop-overflow)


> 
>> Additionally, update the type of some variables in various functions
>> that don't access the flexible-array member, changing them to the
>> newly created `struct ethtool_link_settings_hdr`.
> 
> Why? Please avoid unnecessary code changes.

This is actually necessary. As the type of the conflicting middle members
changed, those instances that expect the type to be `struct ethtool_link_settings`
should be adjusted to the new type. Another option is to leave the type
unchanged and instead use container_of. See below.

So, instead of this:

-	struct ethtool_link_settings *base = &link_ksettings->base;
+	struct ethtool_link_settings_hdr *base = &link_ksettings->base;

we would do something like this:

-	struct ethtool_link_settings *base = &link_ksettings->base;
+	struct ethtool_link_settings *base = container_of(&link_ksettings->base,
+							  struct struct ethtool_link_settings, hdr);

I think that in this case, we could avoid using `container_of()`, but
if you prefer that, I can update the patch.


> 
>>   include/linux/ethtool.h                            |  2 +-
> 
> This is probably where most of the warnings come from.
> Please split the changes to this header file as a separate patch
> for ease of review / validation.
> 

Sure thing!

Thanks
--
Gustavo