Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Mimi Zohar <zohar@linux.ibm.com>]

On Fri, 2018-09-21 at 13:13 -0700, Eric Biggers wrote:
> On Fri, Sep 21, 2018 at 03:55:33PM -0400, Mimi Zohar wrote:
> > On Fri, 2018-09-21 at 12:33 -0700, Eric Biggers wrote:
> > > On Fri, Sep 21, 2018 at 03:02:14PM -0400, Mimi Zohar wrote:
> > > > On Fri, 2018-09-21 at 11:54 -0700, Eric Biggers wrote:
> > > > > On Fri, Sep 21, 2018 at 02:42:38PM -0400, Mimi Zohar wrote:
> > > > > > On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> > > > > > > From: Eric Biggers <ebiggers@google.com>
> > > > > > > 
> > > > > > > The 'init_keyring' variable actually just gave the value of
> > > > > > > CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> > > > > > > directly instead.  No change in behavior; this just simplifies the code.
> > > > > > 
> > > > > > We try to minimize as much as possible "ifdefs" in C code.  This
> > > > > > change is moving in the wrong direction.
> > > > > 
> > > > > So your preferred approach is to store the values of Kconfig options in
> > > > > variables?  That defeats much of the point of having Kconfig options...
> > > > 
> > > > No, I prefer using "ifdefs" in include files, not C code, and defining
> > > > stub functions.
> > > > 
> > > > Mimi
> > > > 
> > > 
> > > integrity_init_keyring() is already stubbed out in a header.  What are you
> > > suggesting, exactly?
> > 
> > Refer to section "20) Conditional Compilation" of
> > Documentation/process/coding-style.rst.
> > 
> > Mimi
> > 
> 
> I'm already familiar with that.  Unfortunately, you haven't clearly indicated
> what alternative you prefer, and it's unclear whether you've even read my patch,
> given that you're apparently saying to define a stub function which actually
> already exists.  Maybe you want the 'init_keyring' bool replaced
> IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING)?  That doesn't really make sense
> though, because integrity_init_keyring() is already stubbed out in some
> configurations; it makes more sense to fix the condition for stubbing it out...

I read your patch and commented that "ifdefs" don't belong in .c
files, as the above referenced doc says.  My comment on using ifdefs
for defining stubs was simply an example of how to get around using
ifdefs in C.

The doc seems to be suggesting to use IS_ENABLED() to set a flag,
which is what the existing code does.  Replacing the existing code
with IS_ENABLE() would be fine, but seems unnecessary.

I don't see a need to change the existing code to add more ifdefs in
.c files.

Mimi