netdev - Re: [PATCH v5] net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)

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: <52701C6B.6010900@xdin.com>
Date:	Tue, 29 Oct 2013 21:36:59 +0100
From:	Arvid Brodin <arvid.brodin@...n.com>
To:	David Laight <David.Laight@...LAB.COM>
CC:	Joe Perches <joe@...ches.com>, <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Stephen Hemminger <shemminger@...tta.com>,
	Javier Boticario <jboticario@...il.com>,
	<balferreira@...glemail.com>,
	Elías Molina Muñoz <elias.molina@....es>
Subject: Re: [PATCH v5] net/hsr: Add support for the High-availability Seamless
 Redundancy protocol (HSRv0)

On 2013-10-28 10:17, David Laight wrote:
>> Subject: Re: [PATCH v5] net/hsr: Add support for the High-availability Seamless Redundancy protocol
>> (HSRv0)
>>
>> On Fri, 2013-10-25 at 20:20 +0200, Arvid Brodin wrote:
>>> On 2013-10-23 18:52, Joe Perches wrote:
>>>> On Wed, 2013-10-23 at 18:09 +0200, Arvid Brodin wrote:
>> []
>>>>> +/* above(a, b) - return 1 if a > b, 0 otherwise.
>>>>> + */
>>>>> +static bool above(u16 a, u16 b)
>>>>> +{
>>>>> +	/* Remove inconsistency where above(a, b) == below(a, b) */
>>>>> +	if ((int) b - a == 32768)
>>>>> +		return 0;
>>>>> +
>>>>> +	return (((s16) (b - a)) < 0);
>>>>> +}
>>>>> +#define below(a, b)		above((b), (a))
>>>>> +#define above_or_eq(a, b)	(!below((a), (b)))
>>>>> +#define below_or_eq(a, b)	(!above((a), (b)))
>>>>
>>>> This looks odd.
>>>
>>> It relies on unsigned arithmetic to compare two values that may wrap. I.e.,
>>> it doesn't care about the absolute sizes, but only about the distance
>>> between the numbers.
> 
> I'd have thought it wasn't really worth worrying about what happens
> when a ^ b == 0x8000. Anything using modulo 16 distances shouldn't
> really see such values.
> Perhaps just document the effect.

Unfortunately there are circumstances where such values can be seen - 
specifically when there is a network problem. And since HSR is meant to
be a high-availability protocol, it is imperative that such cases are 
handled consistently. (That would have been the case with modulo 32 as
well, even if the occations when it happened would be a lot rarer, and
the resulting bugs would be much harder to find.)

> 
> Also, if these definitions are in a .h file they need better names

They're in a .c file, but between your's and Joe Perches' comments, I can
see that the names and description needs a face-lift.

Thanks for looking at the code!

> 
> 	David
> 

-- 
Arvid Brodin | Consultant (Linux)
XDIN AB | Knarrarnäsgatan 7 | SE-164 40 Kista | Sweden | xdin.com

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html