Re: [PATCH v2 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

[Thomas Weißschuh]

(+Cc LKML to at least get the conversation into the archives)

Hi Joel,

On 2024-04-16 18:18:19+0200, Joel Granados wrote:
> On Mon, Apr 08, 2024 at 04:21:58PM +0200, Thomas Weißschuh wrote:
> > On 2024-04-08 10:59:28+0200, Joel Granados wrote:
> > > On Tue, Apr 02, 2024 at 07:15:37PM +0200, Thomas Weißschuh wrote:
> > > > On 2024-03-28 22:00:16+0100, Joel Granados wrote:
> > > > > On Sat, Mar 23, 2024 at 04:44:08PM +0100, Thomas Weißschuh wrote:
> > > > > > * Patch 1 is a bugfix for the stack_erasing sysctl handler
> > > > > > * Patches 2-10 change various helper functions throughout the kernel to
> > > > > >   be able to handle 'const ctl_table'.
> > > > > > * Patch 11 changes the signatures of all proc handlers through the tree.
> > > > > >   Some other signatures are also adapted, for details see the commit
> > > > > >   message.
> > > > > > 
> > > > > > Only patch 1 changes any code at all.
> > > > > > 
> > > > > > The series was compile-tested on top of next-20230322 for
> > > > > > i386, x86_64, arm, arm64, riscv, loongarch and s390.
> > > > > > 
> > > > > > This series was split from my larger series sysctl-const series [0].
> > > > > > It only focusses on the proc_handlers but is an important step to be
> > > > > > able to move all static definitions of ctl_table into .rodata.
> > > > > > 
> > > > > > [0] https://lore.kernel.org/lkml/20231204-const-sysctl-v2-0-7a5060b11447@xxxxxxxxxxxxxx/
> > > > > > 
> > > > > > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Reduce recipient list
> > > > > How did you reduce it? and why did you reduce it? This is quite an
> > > > > extensive change; if anything we should have lots of eyes on it.
> > > > 
> > > > I used get_maintainer.pl for v1 and received negative feedback about
> > > > sending the series to too many people.
> > > I completely missed that. Was that a private mail to you? Do you have a
> > > link of the message? I checked my history and I do not have it.
> > 
> > Yes that message was private. From a fairly prominent maintainer.
> > 
> > > > Advice I got was to only send it to the people whose tree it will be
> > > > going through.
> > > > 
> > > > The only change affecting actual emitted object code in the series is
> > > > "stackleak: don't modify ctl_table argument" and that got acked by Kees
> > > > already quite some time ago.
> > > > If Kees wants to pick this up for one of his own PRs during this cycle
> > > > that would be nice, too.
> > > > 
> > > > I'm open for suggestions to increase the circle of recipients, but
> > > > blindly using get_maintainer.pl again doesn't seem the right way.
> > 
> > > IMO, this can go either way: You can get feedback that tells you that it
> > > is too many people that you are "bothering" and you can also get
> > > maintainers pinging you to be included in what they consider to be part
> > > of their job. You can't make everyone happy :)
> > 
> > Absolutely, this is my expectation, too.
> > 
> > But maintainers can nicely opt-in to all patches touching their
> > subsystems using lore and lei, while it's harder to opt-out of broad
> > recipient lists.
> > (Except putting me in their killfile...)

> Not sure how possible it is to be individually black listed in a
> maintainers inbox. But it is more of a reason to make it public; because
> at least you have done your due diligence and made it available so every
> one can see what is going on (even if you are black listed
> individually).

The reference to the killfile was more meant as a joke.

> > > > The only real feedback I got was from Dave and that I addressed.
> > > yes. I saw this.
> > > 
> > > > If somebody would have had fundamental issues with the aproach they
> > > > could have spoken up on v1.
> > > This seems reasonable. I'll try to get to it this week and add it to
> > > constfy branch if I do not see anything glaring.
> 
> I Just realized that you did not include any kernel lists. I thought
> that you had just removed individuals and left the lists. This reduces
> the number of eyes on the code which is particularly important in this
> specific case where there are so many changes touching so many
> subsystems.

This is absolutely an oversight, I intended to at least keep LKML.
I have to assume it was me being inattentive.

> Not only that, but it also breaks tools like lei and b4. I have configured b4 to
> look at https://lore.kernel.org/all to handle patches coming from contributors.
> If the change is not public it breaks my command (`b4 am -o - MESSAGE_ID | git
> am -3`).

Understood, as mentioned above the trimming went to far.

FYI:
b4 can do the `git am` itself with `b4 shazam MESSAGE_ID`.
Use the config `b4.shazam-am-flags` for the `-3` flag.

> I do not know who spooked you but I suggest you just remove this person
> from the to:/cc: of your patches and leave the rest as it is. Like I
> did with Mathew Wilcox after he asked me to do so here
> https://lore.kernel.org/all/ZZbJRiN8ENV%2FFoTV@xxxxxxxxxxxxxxxxxxxx/.
> Please resend the patchset including the relevant kernel mailing lists and
> maintainers but excepting the person that sent you the private e-mail.

In addition to the complaint I also got guidance from Thomas Gleixner to
reduce the scope of recipients.

What do you think about the following:

You do a review of v2 and give feedback on that and I'll incorporate
that feedback and afterwards send a v3.
In addition to the recipients of v2 I'll add LKML, Greg and Andrew Morton.
 
> This also goes for your "[PATCH] sysctl: treewide: constify ctl_table_header::ctl_table_arg"
> which is also not public.

Thanks for this pointer, too.
I'd like to handle it the same way as proposed above.


Sorry for all the back-and-forth,
Thomas