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 for Android: free password hash cracker in your pocket

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

Message-ID: <1fea7e51-9972-4c38-92a2-5e1a14ff1653@intel.com>
Date: Thu, 13 Feb 2025 13:05:38 +0100
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
To: Thorsten Blum <thorsten.blum@...ux.dev>, Jakub Kicinski <kuba@...nel.org>
CC: 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



On 2/13/2025 11:49 AM, Thorsten Blum wrote:
> 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

Going further I see that in this file there are more fields in
structures that are just commented out, like:

struct sctp_fwdtsn_hdr {
         __be32 new_cum_tsn;
         /* struct sctp_fwdtsn_skip skip[]; */
};

or

struct sctp_sackhdr {
         __be32 cum_tsn_ack;
         __be32 a_rwnd;
         __be16 num_gap_ack_blocks;
         __be16 num_dup_tsns;
         /* union sctp_sack_variable variable[]; */
};

Does it make sense to do the cleanup of the whole header in this
patch ?

Thanks