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

[Thomas Weißschuh]

On 2024-07-24 11:18:17+0000, Joel Granados wrote:
> On Wed, Jul 24, 2024 at 10:56:10AM +0200, Thomas Weißschuh wrote:
> > On 2024-07-24 10:41:24+0000, Joel Granados wrote:
> ...
> > > > > 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.
> I also remember a comment from the XFS part of the patches where you
> changed the formatting on some functions. What did you do to address
> this? Did you change them manually? Or do you have a script?

Yes, the style fixes were done manually.

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

> Understood.
> 
> > 
> > > 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);
> > ```
> Yes. But you are still missing one more subpatch to catch the function
> declarations in header files that don't name the arguments; like the
> ones in include/linux/mm.h. This is what I used for those:
> ```
> @r3@
> identifier func;
> @@
> 
> int func(
> - struct ctl_table *,
> + const struct ctl_table *,
>   int , void *, size_t *, loff_t *);
> ```
> 
> > 
> > (It doesn't find anything else, though)
> Maybe you are missing running it with --include-headers?

There seems to be a slight misunderstanding.
I meant that it does not find anything *new* on top of the existing
patch, not that it can fully recreate the patch.

(I used --include-headers, undid one header hunk from the patch and
validated that the hunk gets recreated)

> This is the full cocci script that I have:
> ```
> virtual patch
> 
> @r1@
> 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);
> 
> @r2@
> 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)
> { ... }
> 
> @r3@
> identifier func;
> @@
> 
> int func(
> - struct ctl_table *,
> + const struct ctl_table *,
>   int , void *, size_t *, loff_t *);
> ```

LGTM, thanks.