linux-kernel - Re: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers

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:   Tue, 14 Mar 2023 13:36:54 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Matti Vaittinen <mazziesaccount@...il.com>
Cc:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers

On Tue, Mar 14, 2023 at 01:31:06PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 14, 2023 at 12:28:43PM +0200, Matti Vaittinen wrote:
> > On 3/13/23 16:39, Andy Shevchenko wrote:
> > > On Mon, Mar 13, 2023 at 01:31:42PM +0200, Matti Vaittinen wrote:
> > > > On 3/6/23 13:13, Andy Shevchenko wrote:
> > > > > On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote:
> > > > > > On 3/2/23 17:05, Andy Shevchenko wrote:
> > > > > > > On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

...

> > > > > > > > +			if (!diff) {
> > > > > > > 
> > > > > > > Why not positive conditional?
> > > > > > 
> > > > > > Because !diff is a special condition and we check explicitly for it.
> > > > > 
> > > > > And how my suggestion makes it different?
> > > > 
> > > > In example you gave we would be checking if the value is anything else but
> > > > the specific value we are checking for. It is counter intuitive.
> > > > 
> > > > > (Note, it's easy to miss the ! in the conditionals, that's why positive ones
> > > > >    are preferable.)
> > > > 
> > > > Thank you for explaining me the rationale behind the "positive checks". I
> > > > didn't know missing '!' was seen as a thing.
> > > > I still don't think being afraid of missing '!' is a good reason to switch
> > > > to counter intuitive checks. A check "if (!foo)" is a pattern in-kernel if
> > > > anything and in my opinion people really should be aware of it.
> > > > 
> > > > (I would much more say that having a constant value on left side of a
> > > > "equality" check is beneficial as people do really occasionally miss one '='
> > > > when meaning '=='. Still, this is not strong enough reason to make
> > > > counter-intuitive checks. In my books 'avoiding negative checks' is much
> > > > less of a reason as people (in my experience) do not really miss the '!'.)
> > > 
> > > It's not a problem when it's a common pattern (like you mentioned
> > > if (!foo) return -ENOMEM; or alike), but in your case it's not.
> > 
> > I think we can find plenty of cases where the if (!foo) is used also for
> 
> Pleading to the quantity and not quality is not an argument, right?
> 
> > other type of checks. To me the argument about people easily missing the !
> > in if () just do not sound reasonable.
> 
> You may theoretically discuss this, I'm telling from my review background
> and real cases.
> 
> > > I would rather see if (diff == 0) which definitely shows the intention
> > > and I wouldn't tell a word against it.
> > 
> > I think this depends much of the corner of the kernel you have been working
> > with. As far as I remember, in some parts the kernel the check
> > (foo == 0) was actually discouraged, and check (!foo) was preferred.
> 
> Don't you use your common sense?
> 
> > Personally I like !foo much more - but I can tolerate the (foo == 0) in
> > cases where the purpose is to really see if some measure equals to zero.
> > 
> > Other uses where I definitely don't want to use "== 0" are for example
> > checking if a flag is clear, pointer is NULL or "magic value" is zero.
> > 
> > In this case we are checking for a magic value. Having this check written
> > as: (diff == 0), would actually falsely suggest me we are checking for the
> > difference of gains being zero. That would really be a clever obfuscation
> > and I am certain the code readers would fall on that trap quite easily.
> 
> Testing with !diff sounds like it's a boolean kind and makes a false
> impression that all other values are almost the same meaning which is
> not the case. Am I right? That's why diff == 0 shows the exact intention
> here "I would like to check if diff is 0 because this is *special case*".
> 
> Making !diff creates less visibility on this.
> 
> Result: Fundamental disagreement between us.

JFYI:
$ git grep -n 'diff.* == 0[^0-9]' -- drivers/ | wc -l
45

(It happens to have same variable name, but you can imagine that there are
 much more cases with different variable names in use)

-- 
With Best Regards,
Andy Shevchenko