Re: [Outreachy kernel] [PATCH v2] staging: lustre: Constify kernel_param_ops structures

Re: [Outreachy kernel] [PATCH v2] staging: lustre: Constify kernel_param_ops structures

[Julia Lawall]

On Sun, 2 Oct 2016, Rehas Sachdeva wrote:

> Check for structs which are not modified and hence can be declared as const.
> Done using Coccinelle:

The semantic patch below doesn't fit well with this case.  It probably did
help you find the issue, but not for the right reason.  The semantic patch
actually doesn't find any uses of the structure at all.  But it is used,
via the unfolding of the module_param macro.

Greg gave an interesting hint in his comment, saying that structures
that are used in this way are typically const in the kernel.  So you could
instead write a semantic patch that looks for this specific situation.
This is actually a bit tricky, but it will show some features of
Coccinelle.

The first issue is how to match something like:

module_param(libcfs_debug_mb, debugmb, 0644);

which looks like a function call, but is not actually inside another
function.  If you just put a pattern like that, Coccinelle will only look
for it inside of a function definition.  You need to inform Coccinelle is
a declaration, which may be at top level.  For that, put in your
metavariable list the declaration:

declarer name module_param;

This will cause Coccinelle to treat module_param in a special way.

The next issue is how to go from eg param_ops_debugmb to debugmb, as found
in:

module_param(libcfs_debug_mb, debugmb, 0644);

For this, you will need to use python code (or ocaml code) to make a new
metavariable that inherits the name of the declared structure and cuts the
"param_ops_" part off the front.  For this, you can look at
coccinelle/demos/pythontococci.cocci.  In your case, you would have
something like the following, after your rule r that obtains the structure
name, i:

@script:python s@
i << r.i;
short;
@@

short = {now some python code to remove "param_ops_" from the beginning of i}

Then your subsequent rules can refer to s.short, for the string that
should be in the second argument of module_param

julia


> // <smpl>
> @r disable optional_qualifier@
> identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
> identifier i;
> position p;
> @@
> static struct s i@p = { ... };
>
> @ok@
> identifier r.i;
> expression e;
> position p;
> @@
> e = i@p
>
> @bad@
> position p != {r.p,ok.p};
> identifier r.i;
> identifier r.s;
> struct s e;
> @@
> e@i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r.i;
> identifier r.s;
> @@
> static
> +const
> struct s i = { ... };
> // </smpl>
>
> File size before:
> 	text	data	bss	dec	hex	filename
> 	4651	4296	1344	10291	2833
> 	drivers/staging/lustre/lnet/libcfs/debug.o
>
> File size after:
> 	text	data	bss	dec	hex	filename
> 	4779	4128	1344	10251	280b
> 	drivers/staging/lustre/lnet/libcfs/debug.o
>
> Signed-off-by: Rehas Sachdeva <aquannie@gmail.com>
> ---
>  drivers/staging/lustre/lnet/libcfs/debug.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/lustre/lnet/libcfs/debug.c b/drivers/staging/lustre/lnet/libcfs/debug.c
> index 23b36b8..f9194f3 100644
> --- a/drivers/staging/lustre/lnet/libcfs/debug.c
> +++ b/drivers/staging/lustre/lnet/libcfs/debug.c
> @@ -80,7 +80,7 @@ static int libcfs_param_debug_mb_set(const char *val,
>   * it needs quite a bunch of extra processing, so we define special
>   * debugmb parameter type with corresponding methods to handle this case
>   */
> -static struct kernel_param_ops param_ops_debugmb = {
> +static const struct kernel_param_ops param_ops_debugmb = {
>  	.set = libcfs_param_debug_mb_set,
>  	.get = param_get_uint,
>  };
> @@ -138,7 +138,7 @@ static int param_set_console_max_delay(const char *val,
>  				      libcfs_console_min_delay, INT_MAX);
>  }
>
> -static struct kernel_param_ops param_ops_console_max_delay = {
> +static const struct kernel_param_ops param_ops_console_max_delay = {
>  	.set = param_set_console_max_delay,
>  	.get = param_get_delay,
>  };
> @@ -156,7 +156,7 @@ static int param_set_console_min_delay(const char *val,
>  				      1, libcfs_console_max_delay);
>  }
>
> -static struct kernel_param_ops param_ops_console_min_delay = {
> +static const struct kernel_param_ops param_ops_console_min_delay = {
>  	.set = param_set_console_min_delay,
>  	.get = param_get_delay,
>  };
> @@ -188,7 +188,7 @@ static int param_set_uintpos(const char *val, const struct kernel_param *kp)
>  	return param_set_uint_minmax(val, kp, 1, -1);
>  }
>
> -static struct kernel_param_ops param_ops_uintpos = {
> +static const struct kernel_param_ops param_ops_uintpos = {
>  	.set = param_set_uintpos,
>  	.get = param_get_uint,
>  };
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20161002182515.GA32326%40toblerone.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v2] staging: lustre: Constify kernel_param_ops structures

[Rehas Sachdeva]

On Sun, Oct 02, 2016 at 09:51:41PM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 2 Oct 2016, Rehas Sachdeva wrote:
> 
> > Check for structs which are not modified and hence can be declared as const.
> > Done using Coccinelle:
> 
> The semantic patch below doesn't fit well with this case.  It probably did
> help you find the issue, but not for the right reason.  The semantic patch
> actually doesn't find any uses of the structure at all.  But it is used,
> via the unfolding of the module_param macro.
> 
Oh yes, I just realised it now.

> Greg gave an interesting hint in his comment, saying that structures
> that are used in this way are typically const in the kernel.  So you could
> instead write a semantic patch that looks for this specific situation.
> This is actually a bit tricky, but it will show some features of
> Coccinelle.
> 
> The first issue is how to match something like:
> 
> module_param(libcfs_debug_mb, debugmb, 0644);
> 
> which looks like a function call, but is not actually inside another
> function.  If you just put a pattern like that, Coccinelle will only look
> for it inside of a function definition.  You need to inform Coccinelle is
> a declaration, which may be at top level.  For that, put in your
> metavariable list the declaration:
> 
> declarer name module_param;
> 
> This will cause Coccinelle to treat module_param in a special way.
> 

Understood.

> The next issue is how to go from eg param_ops_debugmb to debugmb, as found
> in:
> 
> module_param(libcfs_debug_mb, debugmb, 0644);
> 
> For this, you will need to use python code (or ocaml code) to make a new
> metavariable that inherits the name of the declared structure and cuts the
> "param_ops_" part off the front.  For this, you can look at
> coccinelle/demos/pythontococci.cocci.  In your case, you would have
> something like the following, after your rule r that obtains the structure
> name, i:
> 
> @script:python s@
> i << r.i;
> short;
> @@
> 
> short = {now some python code to remove "param_ops_" from the beginning of i}
> 
> Then your subsequent rules can refer to s.short, for the string that
> should be in the second argument of module_param
>

Understood. Thanks a lot. I'll try writing script for this case.

Rehas

> julia
> 
> 
> > // <smpl>
> > @r disable optional_qualifier@
> > identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
> > identifier i;
> > position p;
> > @@
> > static struct s i@p = { ... };
> >
> > @ok@
> > identifier r.i;
> > expression e;
> > position p;
> > @@
> > e = i@p
> >
> > @bad@
> > position p != {r.p,ok.p};
> > identifier r.i;
> > identifier r.s;
> > struct s e;
> > @@
> > e@i@p
> >
> > @depends on !bad disable optional_qualifier@
> > identifier r.i;
> > identifier r.s;
> > @@
> > static
> > +const
> > struct s i = { ... };
> > // </smpl>
> >
> > File size before:
> > 	text	data	bss	dec	hex	filename
> > 	4651	4296	1344	10291	2833
> > 	drivers/staging/lustre/lnet/libcfs/debug.o
> >
> > File size after:
> > 	text	data	bss	dec	hex	filename
> > 	4779	4128	1344	10251	280b
> > 	drivers/staging/lustre/lnet/libcfs/debug.o
> >
> > Signed-off-by: Rehas Sachdeva <aquannie@gmail.com>
> > ---
> >  drivers/staging/lustre/lnet/libcfs/debug.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lnet/libcfs/debug.c b/drivers/staging/lustre/lnet/libcfs/debug.c
> > index 23b36b8..f9194f3 100644
> > --- a/drivers/staging/lustre/lnet/libcfs/debug.c
> > +++ b/drivers/staging/lustre/lnet/libcfs/debug.c
> > @@ -80,7 +80,7 @@ static int libcfs_param_debug_mb_set(const char *val,
> >   * it needs quite a bunch of extra processing, so we define special
> >   * debugmb parameter type with corresponding methods to handle this case
> >   */
> > -static struct kernel_param_ops param_ops_debugmb = {
> > +static const struct kernel_param_ops param_ops_debugmb = {
> >  	.set = libcfs_param_debug_mb_set,
> >  	.get = param_get_uint,
> >  };
> > @@ -138,7 +138,7 @@ static int param_set_console_max_delay(const char *val,
> >  				      libcfs_console_min_delay, INT_MAX);
> >  }
> >
> > -static struct kernel_param_ops param_ops_console_max_delay = {
> > +static const struct kernel_param_ops param_ops_console_max_delay = {
> >  	.set = param_set_console_max_delay,
> >  	.get = param_get_delay,
> >  };
> > @@ -156,7 +156,7 @@ static int param_set_console_min_delay(const char *val,
> >  				      1, libcfs_console_max_delay);
> >  }
> >
> > -static struct kernel_param_ops param_ops_console_min_delay = {
> > +static const struct kernel_param_ops param_ops_console_min_delay = {
> >  	.set = param_set_console_min_delay,
> >  	.get = param_get_delay,
> >  };
> > @@ -188,7 +188,7 @@ static int param_set_uintpos(const char *val, const struct kernel_param *kp)
> >  	return param_set_uint_minmax(val, kp, 1, -1);
> >  }
> >
> > -static struct kernel_param_ops param_ops_uintpos = {
> > +static const struct kernel_param_ops param_ops_uintpos = {
> >  	.set = param_set_uintpos,
> >  	.get = param_get_uint,
> >  };
> > --
> > 2.7.4
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20161002182515.GA32326%40toblerone.
> > For more options, visit https://groups.google.com/d/optout.
> >

[PATCH v2] staging: lustre: Constify kernel_param_ops structures

[Rehas Sachdeva]

Check for structs which are not modified and hence can be declared as const.
Done using Coccinelle:

// <smpl>
@r disable optional_qualifier@
identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
identifier i;
position p;
@@
static struct s i@p = { ... };

@ok@
identifier r.i;
expression e;
position p;
@@
e = i@p

@bad@
position p != {r.p,ok.p};
identifier r.i;
identifier r.s;
struct s e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
identifier r.s;
@@
static
+const
struct s i = { ... };
// </smpl>

File size before:
	text	data	bss	dec	hex	filename
	4651	4296	1344	10291	2833
	drivers/staging/lustre/lnet/libcfs/debug.o

File size after:
	text	data	bss	dec	hex	filename
	4779	4128	1344	10251	280b
	drivers/staging/lustre/lnet/libcfs/debug.o

Signed-off-by: Rehas Sachdeva <aquannie@gmail.com>
---

Change in v2:
Fix typo "can are" -> "are" in commit message

 drivers/staging/lustre/lnet/libcfs/debug.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lnet/libcfs/debug.c b/drivers/staging/lustre/lnet/libcfs/debug.c
index 23b36b8..f9194f3 100644
--- a/drivers/staging/lustre/lnet/libcfs/debug.c
+++ b/drivers/staging/lustre/lnet/libcfs/debug.c
@@ -80,7 +80,7 @@ static int libcfs_param_debug_mb_set(const char *val,
  * it needs quite a bunch of extra processing, so we define special
  * debugmb parameter type with corresponding methods to handle this case
  */
-static struct kernel_param_ops param_ops_debugmb = {
+static const struct kernel_param_ops param_ops_debugmb = {
 	.set = libcfs_param_debug_mb_set,
 	.get = param_get_uint,
 };
@@ -138,7 +138,7 @@ static int param_set_console_max_delay(const char *val,
 				      libcfs_console_min_delay, INT_MAX);
 }
 
-static struct kernel_param_ops param_ops_console_max_delay = {
+static const struct kernel_param_ops param_ops_console_max_delay = {
 	.set = param_set_console_max_delay,
 	.get = param_get_delay,
 };
@@ -156,7 +156,7 @@ static int param_set_console_min_delay(const char *val,
 				      1, libcfs_console_max_delay);
 }
 
-static struct kernel_param_ops param_ops_console_min_delay = {
+static const struct kernel_param_ops param_ops_console_min_delay = {
 	.set = param_set_console_min_delay,
 	.get = param_get_delay,
 };
@@ -188,7 +188,7 @@ static int param_set_uintpos(const char *val, const struct kernel_param *kp)
 	return param_set_uint_minmax(val, kp, 1, -1);
 }
 
-static struct kernel_param_ops param_ops_uintpos = {
+static const struct kernel_param_ops param_ops_uintpos = {
 	.set = param_set_uintpos,
 	.get = param_get_uint,
 };
-- 
2.7.4

Re: [PATCH v2] staging: lustre: Constify kernel_param_ops structures

[Greg Kroah-Hartman]

On Sun, Oct 02, 2016 at 11:57:55PM +0530, Rehas Sachdeva wrote:
> Check for structs which are not modified and hence can be declared as const.
> Done using Coccinelle:

<snip>

Why did you send this same patch twice?

confused,

greg k-h

Re: [Outreachy Kernel] [PATCH v2] staging: lustre: Constify kernel_param_ops structures

[Rehas Sachdeva]

On Mon, Oct 03, 2016 at 09:02:33AM +0200, Greg Kroah-Hartman wrote:
> On Sun, Oct 02, 2016 at 11:57:55PM +0530, Rehas Sachdeva wrote:
> > Check for structs which are not modified and hence can be declared as const.
> > Done using Coccinelle:
> 
> <snip>
> 
> Why did you send this same patch twice?
> 
I'm sorry. I forgot to include the changelog in v2.

Rehas

> confused,
> 
> greg k-h