staging: lustre: Constify kernel_param_ops structures

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 <[email protected]>

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

drivers/staging/lustre/lnet/libcfs/debug.c +4 -5

index 23b36b8..f9194f3 100644
@@ -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 [email protected]. To post to this group, send email to [email protected].
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.

Quoted 2 lines · Rehas Sachdeva

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

Quoted 98 lines

// <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 <[email protected]>

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

drivers/staging/lustre/lnet/libcfs/debug.c +4 -5

index 23b36b8..f9194f3 100644
@@ -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 [email protected]. To post to this group, send email to [email protected].
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.

On Sat, 1 Oct 2016, Rehas Sachdeva wrote:

Quoted 2 lines

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.

Quoted 4 lines · Julia Lawall

On Sat, 1 Oct 2016, Rehas Sachdeva wrote:

Quoted 8 lines

Quoted 2 lines

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

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

Quoted 14 lines · Greg Kroah-Hartman

Quoted 4 lines · Julia Lawall

On Sat, 1 Oct 2016, Rehas Sachdeva wrote:

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

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 [email protected]. To post to this group, send email to [email protected].
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.

Quoted 22 lines · 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.

Quoted 4 lines

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

Quoted 33 lines

Thanks,
Rehas

Quoted 14 lines · Greg Kroah-Hartman

Quoted 4 lines · Julia Lawall

On Sat, 1 Oct 2016, Rehas Sachdeva wrote:

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

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 [email protected]. To post to this group, send email to [email protected].
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.