[PATCH] staging: lustre: Constify kernel_param_ops structures

[PATCH] staging: lustre: Constify kernel_param_ops structures

[Rehas Sachdeva]

Check for structs which can 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>
---
 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: [Outreachy kernel] [PATCH] staging: lustre: Constify kernel_param_ops structures

[Julia Lawall]

On Sat, 1 Oct 2016, Rehas Sachdeva wrote:

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

"can are" -> "are"

This merits a little more discussion, though.  I have the impression that
these structures are used via the module_param macro.  It would be useful
to look at the expansion of the macros and see what exactly is going on.

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/20161001170549.GA2965%40toblerone.
> For more options, visit https://groups.google.com/d/optout.
>

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

[Rehas Sachdeva]

On Sat, Oct 01, 2016 at 07:13:16PM +0200, Julia Lawall wrote:
> 
> 
> On Sat, 1 Oct 2016, Rehas Sachdeva wrote:
> 
> > Check for structs which can are not modified and hence can be declared as
> > const. Done using Coccinelle:
> 
> "can are" -> "are"

Noted. Thanks

> 
> This merits a little more discussion, though.  I have the impression that
> these structures are used via the module_param macro.  It would be useful
> to look at the expansion of the macros and see what exactly is going on.
>

Oh. I'll take a look at that. Thanks
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/20161001170549.GA2965%40toblerone.
> > For more options, visit https://groups.google.com/d/optout.
> >

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

[Greg Kroah-Hartman]

On Sat, Oct 01, 2016 at 07:13:16PM +0200, Julia Lawall wrote:
> 
> 
> On Sat, 1 Oct 2016, Rehas Sachdeva wrote:
> 
> > Check for structs which can are not modified and hence can be declared as
> > const. Done using Coccinelle:
> 
> "can are" -> "are"
> 
> This merits a little more discussion, though.  I have the impression that
> these structures are used via the module_param macro.  It would be useful
> to look at the expansion of the macros and see what exactly is going on.

Based on other uses of this structure in the kernel, I think this is
safe.  But verification would be good...

thanks,

greg k-h

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

[Rehas Sachdeva]

Hello,

I looked into the module_param macro.

Consider the struct kernel_param_ops param_ops_debugmb.

It gets used in the call module_param(libcfs_debug_mb, debugmb, 0644), which

converts to module_param_named(libcfs_debug_mb, libcfs_debug_mb, debugmb, 0644)

which converts to module_param_cb(libcfs_debug_mb, &param_ops_debugmb,
				  &libcfs_debug_mb, 0644)

which converts to __module_param_call(MODULE_PARAM_PREFIX, libcfs_debug_mb,
				      &param_ops_debugmb, &libcfs_debug_mb,
				      0644, -1, 0)

Now this translates to declaration of a variable of type struct kernel_param
whose field const struct kernel_param_ops* is assigned the value of
&param_ops_debugmb. Since this field is const, we can originally declare
struct kernel_param_ops param_ops_debugmb as const as well.

A small doubt:
Is a successful build after this constification change not sufficient to
verify its correctness? Since the preprocessor translates the macros before
compilation anyways.

Thanks,
Rehas

On Sun, Oct 02, 2016 at 12:28:17PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Oct 01, 2016 at 07:13:16PM +0200, Julia Lawall wrote:
> > 
> > 
> > On Sat, 1 Oct 2016, Rehas Sachdeva wrote:
> > 
> > > Check for structs which can are not modified and hence can be declared as
> > > const. Done using Coccinelle:
> > 
> > "can are" -> "are"
> > 
> > This merits a little more discussion, though.  I have the impression that
> > these structures are used via the module_param macro.  It would be useful
> > to look at the expansion of the macros and see what exactly is going on.
> 
> Based on other uses of this structure in the kernel, I think this is
> safe.  But verification would be good...
> 
> thanks,
> 
> greg k-h

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

[Julia Lawall]

On Sun, 2 Oct 2016, Rehas Sachdeva wrote:

> Hello,
>
> I looked into the module_param macro.
>
> Consider the struct kernel_param_ops param_ops_debugmb.
>
> It gets used in the call module_param(libcfs_debug_mb, debugmb, 0644), which
>
> converts to module_param_named(libcfs_debug_mb, libcfs_debug_mb, debugmb, 0644)
>
> which converts to module_param_cb(libcfs_debug_mb, &param_ops_debugmb,
> 				  &libcfs_debug_mb, 0644)
>
> which converts to __module_param_call(MODULE_PARAM_PREFIX, libcfs_debug_mb,
> 				      &param_ops_debugmb, &libcfs_debug_mb,
> 				      0644, -1, 0)
>
> Now this translates to declaration of a variable of type struct kernel_param
> whose field const struct kernel_param_ops* is assigned the value of
> &param_ops_debugmb. Since this field is const, we can originally declare
> struct kernel_param_ops param_ops_debugmb as const as well.
>
> A small doubt:
> Is a successful build after this constification change not sufficient to
> verify its correctness? Since the preprocessor translates the macros before
> compilation anyways.

Yes, but there are always small details.  For example, something could be
done differently for a different architecture.  Or there could be a cast
that does not contain the const annotation.  In that case the compiler
will not complain, but there will be an error at run time if the value
turns out to be updated.

Thanks for the analysis.  Everything seems fine.

julia

>
> Thanks,
> Rehas
>
> On Sun, Oct 02, 2016 at 12:28:17PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Oct 01, 2016 at 07:13:16PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Sat, 1 Oct 2016, Rehas Sachdeva wrote:
> > >
> > > > Check for structs which can are not modified and hence can be declared as
> > > > const. Done using Coccinelle:
> > >
> > > "can are" -> "are"
> > >
> > > This merits a little more discussion, though.  I have the impression that
> > > these structures are used via the module_param macro.  It would be useful
> > > to look at the expansion of the macros and see what exactly is going on.
> >
> > Based on other uses of this structure in the kernel, I think this is
> > safe.  But verification would be good...
> >
> > thanks,
> >
> > greg k-h
>
> --
> 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/20161002125804.GA22828%40toblerone.
> For more options, visit https://groups.google.com/d/optout.
>

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

[Rehas Sachdeva]

On Sun, Oct 02, 2016 at 04:17:10PM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 2 Oct 2016, Rehas Sachdeva wrote:
> 
> > Hello,
> >
> > I looked into the module_param macro.
> >
> > Consider the struct kernel_param_ops param_ops_debugmb.
> >
> > It gets used in the call module_param(libcfs_debug_mb, debugmb, 0644), which
> >
> > converts to module_param_named(libcfs_debug_mb, libcfs_debug_mb, debugmb, 0644)
> >
> > which converts to module_param_cb(libcfs_debug_mb, &param_ops_debugmb,
> > 				  &libcfs_debug_mb, 0644)
> >
> > which converts to __module_param_call(MODULE_PARAM_PREFIX, libcfs_debug_mb,
> > 				      &param_ops_debugmb, &libcfs_debug_mb,
> > 				      0644, -1, 0)
> >
> > Now this translates to declaration of a variable of type struct kernel_param
> > whose field const struct kernel_param_ops* is assigned the value of
> > &param_ops_debugmb. Since this field is const, we can originally declare
> > struct kernel_param_ops param_ops_debugmb as const as well.
> >
> > A small doubt:
> > Is a successful build after this constification change not sufficient to
> > verify its correctness? Since the preprocessor translates the macros before
> > compilation anyways.
> 
> Yes, but there are always small details.  For example, something could be
> done differently for a different architecture.  Or there could be a cast
> that does not contain the const annotation.  In that case the compiler
> will not complain, but there will be an error at run time if the value
> turns out to be updated.
> 

Alright. Thanks
I'll send a v2 correcting the commit message.

> Thanks for the analysis.  Everything seems fine.
> 
> julia
> 
> >
> > Thanks,
> > Rehas
> >
> > On Sun, Oct 02, 2016 at 12:28:17PM +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Oct 01, 2016 at 07:13:16PM +0200, Julia Lawall wrote:
> > > >
> > > >
> > > > On Sat, 1 Oct 2016, Rehas Sachdeva wrote:
> > > >
> > > > > Check for structs which can are not modified and hence can be declared as
> > > > > const. Done using Coccinelle:
> > > >
> > > > "can are" -> "are"
> > > >
> > > > This merits a little more discussion, though.  I have the impression that
> > > > these structures are used via the module_param macro.  It would be useful
> > > > to look at the expansion of the macros and see what exactly is going on.
> > >
> > > Based on other uses of this structure in the kernel, I think this is
> > > safe.  But verification would be good...
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > --
> > 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/20161002125804.GA22828%40toblerone.
> > For more options, visit https://groups.google.com/d/optout.
> >