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

[Mateusz Polchlopek <mateusz.polchlopek@intel.com>]

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