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

[Rehas Sachdeva <aquannie@gmail.com>]

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.
> >