linux-kernel - Re: [PATCH 14/17] hpsa: use new IS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 Apr 2012 09:56:08 -0400
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	Stephen Cameron <stephenmcameron@...il.com>
CC:	"Stephen M. Cameron" <scameron@...rdog.cce.hp.com>,
	<james.bottomley@...senpartnership.com>,
	<linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<matthew.gates@...com>, <thenzl@...hat.com>,
	<akpm@...ux-foundation.org>, <mikem@...rdog.cce.hp.com>
Subject: Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

On 12-04-22 10:20 PM, Stephen Cameron wrote:
> On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
> <paul.gortmaker@...driver.com> wrote:
>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
>> <scameron@...rdog.cce.hp.com> wrote:
>>> From: Stephen M. Cameron <scameron@...rdog.cce.hp.com>
>>>
>>> Signed-off-by: Stephen M. Cameron <scameron@...rdog.cce.hp.com>
>>
>> You've not written a commit log, so I'm left guessing what the
>> intended rationale is here.  COMPAT, X86 and PCI_MSI are
>> I believe all bool, so why make this change?  To me it gives
>> a misleading message that some level of modular awareness
>> is needed here, when there really isn't any such need.
> 
> Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
> 
>    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
>    current form requires us to actually define every possible bool/tristate
>    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
>    [...]
> 
> and so I kind of thought IS_ENABLED is the new way to do that sort of thing.

In my opinion, IS_ENABLED can/should be used when you have:

#if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)

i.e. "is this driver built in, or can it be loaded as a module?"
The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
generated by Kbuild infrastructure.

It will do the Right Thing even for cases where CONFIG_FOO_MODULE
is impossible, but it does as I've said, give the wrong impression
that there is some possibility of modular presence.  At least to
those who are familiar with the implementation and why it exists.
[I'll grant you that the name doesn't convey the use case too well.]

> 
> Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
> things to work.

I don't think we want to go and mass replace all existing cases of

   #ifdef CONFIG_SOME_BOOL

with:

   #if IS_ENABLED(CONFIG_SOME_BOOL)

There is no value add.  However, replacing instances of:

   #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)

with:

   #if IS_ENABLED(CONFIG_FOO)

is in my opinion, a reasonable thing to do.  It is shorter, and
it does hide the internally generated _MODULE suffixed "shadow"
variables from appearing directly in the main source files.  And
it tells you that the author was thinking about both the built
in and the modular cases when they wrote it.

Paul.
--

> 
> -- steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/