netdev - Re: [PATCH net-next 0/4] mlx4 misc for 4.16

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]

Message-ID: <c579aff0-fbe5-bfde-4fb3-6af50a80ec59@mellanox.com>
Date:   Sun, 31 Dec 2017 10:29:02 +0200
From:   Tariq Toukan <tariqt@...lanox.com>
To:     David Miller <davem@...emloft.net>, tariqt@...lanox.com
Cc:     netdev@...r.kernel.org, eranbe@...lanox.com
Subject: Re: [PATCH net-next 0/4] mlx4 misc for 4.16



On 28/12/2017 7:28 PM, David Miller wrote:
> From: Tariq Toukan <tariqt@...lanox.com>
> Date: Thu, 28 Dec 2017 16:26:07 +0200
> 
>> This patchset contains misc cleanups and improvements
>> to the mlx4 Core and Eth drivers.
>>
>> In patches 1 and 2 I reduce and reorder the branches in the RX csum flow.
>> In patch 3 I align the FMR unmapping flow with the device spec, to allow
>>    a remapping afterwards.
>> Patch 4 by Moni changes the default QoS settings so that a pause
>>    frame stops all traffic regardless of its prio.
>>
>> Series generated against net-next commit:
>> 836df24a7062 net: hns3: hns3_get_channels() can be static
> 
> Series applied, thanks Tariq.
> 
> I can't say that that ipv6 ifdef you added in patch #1 is the nicest.
> 
> It's one thing to ifdef control entire blocks of code, or individual
> values.
> 
> It's another thing to partially include pieces of code structure
> such as closing parenthesis, arithmetic operators, and curly braces.
> Two of those were included in the ifdef section.
> 
> For example, if we have:
> 
> 	if (x & (IPV4_THING | IPV6_THING)) {
> 
> I don't want to see:
> 
> 	if (x & (IPV4_THING |
> #ifdef IPV6_TEST
> 		 IPV6_THING)) {
> #else
> 		 0)) {
> #endif
> 
> Those closing parenthesis and the openning curly brace are there
> regardless of the CPP test, and duplicating them in both arms of
> the CPP test only makes the code more confusing to read.
> 
> I can say I would prefer:
> 
> 	if (x & (IPV4_THING |
> #ifdef IPV6_TEST
> 		 IPV6_THING
> #else
> 		 0
> #endif
> 		 )) {
> 
> which is better.  However, best would be:
> 
> #ifdef IPV6_TEST
> #define THING_MASK	IPV4_THING | IPV6_THING
> #else
> #define THING_MASK	IPV4_THING
> #endif
> 
> 	if (x & THING_MASK) {
> 
> is the best.
> 
Hi Dave,
Thanks for accepting the series.
Your suggestions look better indeed, I will prepare a followup patch and 
submit soon.

Regards,
Tariq