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 21:43:54 -0400
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	James Bottomley <James.Bottomley@...senpartnership.com>
Cc:	Stephen Cameron <stephenmcameron@...il.com>,
	"Stephen M. Cameron" <scameron@...rdog.cce.hp.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 Mon, Apr 23, 2012 at 10:56 AM, James Bottomley
<James.Bottomley@...senpartnership.com> wrote:
> On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote:
>> 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.
>
> I don't think you need to change the driver unless there's a good
> reason.  In kernel terms, it's usually better to wait a bit to see if
> the wheels fall off any particular bandwagon before leaping on it ...
>
>> 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.
>
> To be honest, I think we want to use #if IS_ENABLED() for all cases
> going forwards, including the boolean case.

I guess we will have to agree to disagree then.

>
> The reason is that it's an easier design pattern.  If we have the #ifdef
> CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can
> be modular or not it's just creating pointless rules that someone will
> annoy us all by enforcing in a checkpatch or some other script.  Whereas
> #if IS_ENABLED(CONFIG_X) is obvious and simple.

What exactly is an "easier design pattern", and what are the gains?

To me, it has nothing to do with rules, and what the checkpatch folks do
or do not do.  The line "#ifdef CONFIG_FOO" conveys one specific piece
of information.  The line "#if IS_ENABLED(CONFIG_FOO) conveys
a different piece of information, which is a superset of the former.

If you blindly convert all of them, you throw away information that would
otherwise be available to the code reader.  I would not support that.

Thanks,
Paul.
--

>
> James
>
>
> --
> 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/
--
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/