Re: [PATCH v2] sysctl: treewide: constify the ctl_table argument of proc_handlers

[Thomas Weißschuh]

On 2024-07-24 10:41:24+0000, Joel Granados wrote:
> On Mon, Jul 22, 2024 at 09:41:36AM +0200, Thomas Weißschuh wrote:
> > Hi Joel,
> > 
> > On 2024-07-17 21:57:48+0000, Joel Granados wrote:
> > > On Wed, Jul 17, 2024 at 05:26:44PM +0200, Thomas Weißschuh wrote:
> > > > On 2024-07-16 17:27:05+0000, Joel Granados wrote:
> > > > > On Mon, Jul 15, 2024 at 10:58:10PM +0200, Thomas Weißschuh wrote:
> > > > > > On 2024-07-15 22:23:19+0000, Joel Granados wrote:
> > > > > ...
> > > > > > > The merge window is now open. I want to send this patch on the Wednesday
> > > > > > > of next week (jul 24).
> > > > 
> > > > <snip>
> > > > 
> > > > > > > 2. Does it still apply cleanly against the latest master branch?
> > > > > > 
> > > > > > Not against mainline master, but against next-20240715.
> > > > > > To apply cleanly (and compile) on mainline master it still requires the
> > > > > > net/ and sysctl trees to be merged.
> > > > > > Otherwise some modified functions are missing, leading to (trivial) merge
> > > > > > conflicts or the preparation commits are missing, leading to compilation
> > > > > > errors.
> > > > > 
> > > > > Understood. I have just sent Linus the changes for sysctl-next, so those
> > > > > should land in master soon (baring any issues with the pull request).
> > > > > 
> > > > > These [1] and [2] are the two series in net-dev that are the deps for
> > > > > the constification treewide patch. Once these two go into mainline, then
> > > > > we are good to go. Right?
> > > > 
> > > > Right, but...
> > > > 
> > > > It turns out the preparation patch for mm/hugetlb.c [0] is also still
> > > > missing. I missed it in all the errors triggered in net/.
> > > > But as far as I can see this patch will be part of Andrew's PR for mm.
> > > 
> > > Thx for the heads up, I'll Add that one to my radar of things that need
> > > to be in master before we send the PR to Linus. Will check these three
> > > dependencies on Wednesday next week and send your V2 [3] if I see that
> > > it applies cleanly.
> > 
> > All dependency PRs (sysctl, net, mm) are now merged.
> > My compilation tests all succeed now.
> 
> How did you apply the coccinelle script? I ask because if I do this:
> ```
>  make coccicheck MODE=patch SPFLAGS="--in-place --include-headers" COCCI=SCRIPT
> ```
> 
> I have to add "virtual patch" to the first line of the script you had
> sent. So it would look like this:
> ```
> virtual patch
> 
> @@
> identifier func, ctl, write, buffer, lenp, ppos;
> @@
> 
> int func(
> - struct ctl_table *ctl,
> + const struct ctl_table *ctl,
>   int write, void *buffer, size_t *lenp, loff_t *ppos)
> { ... }
> ```

Yes, IIRC I got told during one review to drop it.
But for me it is also necessary.

Thinking back, there were other "virtual" lines, too.
Maybe those were to ones that needed to be removed, but
"virtual patch" should stay.

> Additionally, this cocci script is not changing the header files. Even
> if I pass --include-headers. Did you change those by hand?

Yes, I changed these manually, originally.

To do it through the script, you need a second subpatch:

```
@@
identifier func, ctl, write, buffer, lenp, ppos;
@@

int func(
- struct ctl_table *ctl,
+ const struct ctl_table *ctl,
  int write, void *buffer, size_t *lenp, loff_t *ppos);
```

(It doesn't find anything else, though)

> Best
> 
> > 
> > > > <snip>
> > > > 
> > > > [0] https://lore.kernel.org/lkml/20240518-sysctl-const-handler-hugetlb-v1-1-47e34e2871b2@xxxxxxxxxxxxxx/
> > > > 
> > > > > [1] net: constify ctl_table arguments of utility functions
> > > > >     https://patchwork.kernel.org/project/netdevbpf/list/?series=856252&state=%2A&archive=both
> > > > > [2] bpf: constify member bpf_sysctl_kern::table
> > > > >     https://patchwork.kernel.org/project/netdevbpf/list/?series=854191&state=*
> > > 
> > > [3] https://lore.kernel.org/20240619-sysctl-const-handler-v2-1-e36d00707097@xxxxxxxxxxxxxx
> 
> -- 
> 
> Joel Granados