linux-kernel - Re: [PATCH v2] reset: add support for non-DT systems

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:   Wed, 14 Feb 2018 13:32:26 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Philipp Zabel <p.zabel@...gutronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Sekhar Nori <nsekhar@...com>,
        Kevin Hilman <khilman@...libre.com>,
        David Lechner <david@...hnology.com>
Subject: Re: [PATCH v2] reset: add support for non-DT systems

On Wed, Feb 14, 2018 at 10:59 AM, Bartosz Golaszewski <brgl@...ev.pl> wrote:
> 2018-02-13 20:17 GMT+01:00 Andy Shevchenko <andy.shevchenko@...il.com>:
>> On Tue, Feb 13, 2018 at 8:39 PM, Bartosz Golaszewski <brgl@...ev.pl> wrote:

>>> +               for (index = 0; lookup->dev; index++, lookup++) {
>>> +                       if (strcmp(dev_id, lookup->dev))
>>> +                               continue;
>>> +
>>> +                       if ((!id && !lookup->id) ||
>>> +                           (id && lookup->id && !strcmp(id, lookup->id))) {
>>> +                               rstc = __reset_control_get_internal(rcdev,
>>> +                                                               index, shared);
>>> +                               break;
>>> +                       }
>>
>> Wouldn't be slightly more readable
>>
>>       if (id && lookup->id) {
>>         if (strcmp(id, lookup->id))
>>           continue;
>>       } else if (id || lookup->id) {
>>         continue;
>>       }
>>
>>       rstc = __reset_control_get_internal(rcdev, index, shared);
>>       break;
>>
>
> You'd get less indentations, yes but I wanted to emphasize the
> condition on which we want to stop in this line.

It doesn't matter (indentation) here, esp. taking into consideration
that you already have another condition by which you continue the
loop.
My proposal adds consistency and from my point of view makes easier to parse.

You check all false conditions in a row, end up with a true one.

>>> +       if (!rstc)
>>> +               return optional ? NULL : ERR_PTR(-ENOENT);
>>
>> Isn't simpler
>>
>> if (!optional && !rstc) // or other way around, depending which might
>> be more offten
>>  return ERR_PTR(...);
>>
>
> IMO it's just a matter of taste.

I think in my proposal it's more straight forward. Easier to parse by reader.

-- 
With Best Regards,
Andy Shevchenko