linux-kernel - Re: [PATCH 01/12] staging: ccree: correct coding style violations

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] [day] [month] [year] [list]

Date:   Mon, 29 May 2017 20:51:24 +0300
From:   Gilad Ben-Yossef <gilad@...yossef.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-crypto@...r.kernel.org, devel@...verdev.osuosl.org,
        driverdev-devel@...uxdriverproject.org,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        Ofir Drang <ofir.drang@....com>
Subject: Re: [PATCH 01/12] staging: ccree: correct coding style violations

On Mon, May 29, 2017 at 8:36 PM, Joe Perches <joe@...ches.com> wrote:
> On Mon, 2017-05-29 at 20:11 +0300, Gilad Ben-Yossef wrote:
>> On Mon, May 29, 2017 at 7:57 PM, Joe Perches <joe@...ches.com> wrote:
>> > On Mon, 2017-05-29 at 16:37 +0200, Greg Kroah-Hartman wrote:
>> > > On Sun, May 28, 2017 at 05:40:26PM +0300, Gilad Ben-Yossef wrote:
>> > > > cc_crypto_ctx.h had multiple coding style violations reported by
>> > > > checkpatch. Fix them all.
>> > >
>> > > Sorry, no.  You need to do only one-thing-per-patch, and "fix all coding
>> > > style issues is not "one thing".  I wouldn't take this kind of patch
>> > > from anyone else, so why should I take it from you?  :)
>> >
>> > Because he's the named MAINTAINER of the subsystem
>> > and you are acting as an upstream conduit.
>> >
>>
>> LOL. Thank you Joe, but I have opted to upstream via staging to get the guidance
>> and review of Greg and other developers kind enough to offer it, and I'd be a
>> fool not to listen to them.
>
> For reviews of technical merit, true.
>
> For reviews of commit log wording of whitespace
> changes, where git diff -w shows no difference,
> less true.
>
> This patch seems almost entirely whitespace except
> one bit reformatting a comment block.
>
> Breaking those down into changes like:
>         added space after commas
>         added spaces around bit shifts
>         added spaces around arithmetic
> is simply excessive.

I'll admit that this was my line of reasoning as well for including it
in a single patch but I if Greg finds it easier to review broken
down I'm fine with that. Something tells me he sees a lot of patches... :-)

> The only comment I would have given would be to
> change the patch context that added line comment
> initiators to use the /** kernel-doc style.
>
> And maybe a style change of
>
> #define CC_MULTI2_SYSTEM_N_DATA_KEY_SIZE (CC_MULTI2_SYSTEM_KEY_SIZE + \
>                                           CC_MULTI2_DATA_KEY_SIZE)
>
> to
>
> #define CC_MULTI2_SYSTEM_N_DATA_KEY_SIZE \
>         (CC_MULTI2_SYSTEM_KEY_SIZE + CC_MULTI2_DATA_KEY_SIZE)
>
> as it's sometimes easier to scan arithmetic on a single line.
>

Thanks for the feedback. I will include it in the revised series.

> btw: this #define seems misleading as it's used in both .min_keysize
> and .max_keysize with "+ 1" and key[CC_MULTI2_SYSTEM_N_DATA_KEY_SIZE]
> is also used.
>

 I'll look into that. There are still areas in this driver I've
inherited that I find... intriguing :-)

Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru