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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5aa93a65-e325-4c77-aaa8-5ef04f3b9697@embeddedor.com>
Date: Tue, 29 Oct 2024 13:18:56 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Michael Chan <michael.chan@...adcom.com>, Andrew Lunn
 <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Potnuri Bharat Teja <bharat@...lsio.com>,
 Christian Benvenuti <benve@...co.com>, Satish Kharat <satishkh@...co.com>,
 Manish Chopra <manishc@...vell.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of
 -Wflex-array-member-not-at-end warnings



On 29/10/24 12:54, Jakub Kicinski wrote:
> On Tue, 29 Oct 2024 12:48:32 -0600 Gustavo A. R. Silva wrote:
>>>> Is this going to be a priority for any other netdev patches in the future?
>>>
>>> It's been the preferred formatting for a decade or more.
>>> Which is why the net/ethtool/ code you're touching follows
>>> this convention. We're less strict about driver code.
>>
>> I mean, the thing about moving the initialization out of line to accommodate
>> for the convention.
>>
>> What I'm understanding is that now you're asking me to change the following
>>
>>        const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>>        const struct ethtool_link_ksettings *ksettings = &data->ksettings;
>> -    const struct ethtool_link_settings *lsettings = &ksettings->base;
>> +    const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
>>
>> to this:
>>
>>        const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>>        const struct ethtool_link_settings_hdr *lsettings;
>>        const struct ethtool_link_ksettings *ksettings;
>>
>>        ksettings = &data->ksettings;
> 
> You don't have to move this one out of line but either way is fine.
> 
>>        lsettings = &ksettings->base;
>>
>> I just want to have clear if this is going to be a priority and in which scenarios
>> should I/others modify the code to accommodate for the convention?
> 
> I don't understand what you mean by priority. If you see code under
> net/ or drivers/net which follows the reverse xmas tree variable
> sorting you should not be breaking this convention. And yes, if
> there are dependencies between variables you should move the init
> out of line.

By priority I mean if preserving the reverse xmas tree is a most
after any changes that mess in some way with it. As in the case below,
where things were already messed up:

+       const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
         struct bnxt *bp = netdev_priv(dev);
         struct bnxt_link_info *link_info = &bp->link_info;
-       const struct ethtool_link_settings *base = &lk_ksettings->base;
         bool set_pause = false;
         u32 speed, lanes = 0;
         int rc = 0;

Should I leave the rest as-is, or should I now have to rearrange the whole
thing to accommodate for the convention?

How I see this, we can take a couple of directions:

a) when things are already messed up, just implement your changes and leave
the rest as-is.

b) when your changes mess things up, clean it up and accommodate for the
convention.

extra option:

c) this is probably going to be a case by case thing and we may ask you
    to do more changes as we see fit.

To be clear, I have no issue with c) (because it's basically how things
usually work), as long as maintainers don't expect v1 of any patch to
be in pristine form. In any other case, I would really like to be crystal
clear about what's expected and what's not.

Thanks!
--
Gustavo