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]

Message-ID: <20171205130308.ifapjzngjisfx4p2@mwanda>
Date:   Tue, 5 Dec 2017 16:03:08 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Marcus Wolf <marcus.wolf@...rthome-wolf.de>
Cc:     devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
        linux@...f-Entwicklungen.de, linux-kernel@...r.kernel.org,
        Simon Sandström <simon@...anor.nu>
Subject: Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in
 rf69_enum.h

On Tue, Dec 05, 2017 at 01:40:02PM +0100, Marcus Wolf wrote:
> > It's not the greatest, but it's not the worst...  The configuration for
> > ->enable_sync is a bit spread out and it might be nice to move it all to
> > one function?
> > 
> > I liked Simon's naming scheme and I thought it was clear what the
> > rf69_set_sync(spi, false) function would do.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^
> > 

Simon's liked splitting it up but he also proposed this alternative:

rf69_set_sync_operation(spi, true/false);

but I removed the "_operation" because I think it doesn't add anything.

> > Simon, it seems like Marcus and I both are Ok with your style choices.
> > Do whatever seems best when you implement the code.  If it's awkward to
> > break up the functions then don't.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan,
> 
> now I am a bit confused.
> 
> My favourit:
> ------------
> rf69_set_sync_enable(spi, false)
> rf69_set_amp_enable(spi, false)
> rf69_set_crc_enable(spi, false)
> 
> I prefer to keep the enable (or comparable), because it shows, what the
> function is doing. For sync, for example, there are several setter:
> size, tolerance, values ... AND enable (or comparable).

To me it's just weird that "_enable" disables anything.  I really
prefer just splitting it up.  I don't think it will bloat the code.
But I'm also fine with:

rf69_set_sync(spi, true/false)
rf69_set_amp(spi, true/false)
rf69_set_crc(spi, true/false)

Anyway, I feel like I'll like whatever Simon does.  Some of these
things, you can't tell how they'll look until the end until you try.
Let's wait until we see a patch before we debate any more.

regards,
dan carpenter