linux-kernel - Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69

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, 5 Dec 2017 13:06:00 +0100
From:   Marcus Wolf <marcus.wolf@...rthome-wolf.de>
To:     Simon Sandström <simon@...anor.nu>,
        Marcus Wolf <marcus.wolf@...rthome-wolf.de>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
        linux@...f-Entwicklungen.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in
 rf69_enum.h

Am 04.12.2017 um 21:05 schrieb Simon Sandström:
> On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
>>
>> Hi Simon, hi Dan,
>>
>> if you both are of the same opinion, for me, it's fine, if we go with two
>> functions.
>>
>> But I don't get the advantage, if we split approx. 10 functions, to get rid
>> of enum optionOnOff.
>>
>> Keep in mind, that if you split the functions, in the interface
>> implementation you also need more code:
>>
>> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
>>
>> will have to be converted in something like
>>
>> if (rx_cfg->enable_sync)
>> 	SET_CHECKED(rf69_set_sync_enbable(dev->spi);
>> else
>> 	SET_CHECKED(rf69_set_sync_disable(dev->spi);
> 
> I think that this makes the code very clear. If the config tells us to
> enable the sync then we'll enabled it, otherwise we'll disable it.
> 

Hi Simon,

I thought about it a while.

Yes, you are right, it makes it very clear - but doesn't tell a lot
extra. The only thing, you can see extra, is, that there is the option,
to turn on and to turn off. You even can't see, whether it is turend on
or off.
The info, that there is an option of en- or disabling - at least from my
side - is visible, too, if there is just one function that takes a bool
or optionOnOff as argument.

When you 'll redesign every setter, that now deals with optionOnOff in
that way, you will introduce something like 50 extra lines of code
without bringing any extra functionality to the driver.
>From my point of view, that's bad: The longer the text, the harder to
understand everything entierly, the more chances for bugs and a lot
harder to service.

So from my point of view such a redesign is nice for optics but bad for
further development. I would really prefer a solution, like Marcin
Ciupak offered. He is not blowing up the code, but even tries to reduce
the calls to READ_REG and WRITE_REG. That increases readbility, too, but
also reduces code size and chances for bugs.

But regardless of the arguments above, I don't mind. If it's a benefit
for you and lot of others in the community, I won't blame you for such a
change.
If you would add 50 lines of code with new funcitonality (e. g. support
the AES feature of the module), I would be a lot happier - and most
useres for sure, too.

Cheers,

Marcus