netdev - Re: [PATCH net-next] sctp: Remove commented out code

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: <F83DD790-9085-4670-9694-2668DACFB4C1@linux.dev>
Date: Thu, 13 Feb 2025 11:49:45 +0100
From: Thorsten Blum <thorsten.blum@...ux.dev>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Mateusz Polchlopek <mateusz.polchlopek@...el.com>,
 Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
 Xin Long <lucien.xin@...il.com>,
 "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>,
 linux-sctp@...r.kernel.org,
 netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] sctp: Remove commented out code

Hi Jakub,

> On 13. Feb 2025, at 04:57, Jakub Kicinski wrote:
> On Tue, 11 Feb 2025 12:33:57 +0100 Mateusz Polchlopek wrote:
>>>> I don't think we want to remove that piece of code, please refer
>>>> to the discussion under the link:
>>>> 
>>>> https://lore.kernel.org/netdev/cover.1681917361.git.lucien.xin@gmail.com/  
>>> 
>>> Hm, the commit message (dbda0fba7a14) says payload was deleted because
>>> "the member is not even used anywhere," but it was just commented out.
>>> In the cover letter it then explains that "deleted" actually means
>>> "commented out."
>>> 
>>> However, I can't follow the reasoning in the cover letter either:
>>> 
>>> "Note that instead of completely deleting it, we just leave it as a
>>> comment in the struct, signalling to the reader that we do expect
>>> such variable parameters over there, as Marcelo suggested."
>>> 
>>> Where do I find Marcelo's suggestion and the "variable parameters over
>>> there?"
>>> 
>> 
>> That's good question, I can't find the Marcelo suggestion that author
>> mention. It's hard to find without links to previous series or
>> discussion :/
>> 
>> I guess it should be also commented by maintainers, I see that in the
>> Xin's thread Kuba also commented change with commenting out instead
>> of removing code. Let's wait
> 
> In the linked thread the point was to document what struct will be next
> in memory. Here we'd be leaving an array of u8s which isn't very
> informative. I see there's precedent in this file, but I vote we just
> delete the line.

This patch deletes the line and I'm wondering why the "cr"?

Were you referring to this patch maybe?
https://lore.kernel.org/r/20250114215439.916207-3-thorsten.blum@linux.dev/

Should both payload fields just be deleted since they're not used?

Thanks,
Thorsten