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

[Mateusz Polchlopek]

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@xxxxxxxxx/
> > > >
> > > > 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@xxxxxxxxx/
>
> 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