linux-kernel - Re: [PATCH] block: aoe: replace use of __constant

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

Message-ID: <20201221110356.6x2h2ip6o3hlwztd@archlinux>
Date:   Mon, 21 Dec 2020 16:33:56 +0530
From:   'Amey Narkhede' <ameynarkhede03@...il.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] block: aoe: replace use of __constant_htons to htons

On 20/12/21 09:23AM, David Laight wrote:
> From: 'Amey Narkhede' <ameynarkhede03@...il.com>
> > Sent: 20 December 2020 20:20
> > To: David Laight <David.Laight@...LAB.COM>
> > Subject: Re: [PATCH] block: aoe: replace use of __constant_htons to htons
> >
> > On 20/12/20 07:35PM, David Laight wrote:
> > > From: Amey Narkhede
> > > > Sent: 20 December 2020 16:46
> > > >
> > > > The macro htons expands to __swab16 which has special
> > > > case for constants in little endian case. In big
> > > > endian case both __constant_htons and htons macros
> > > > expand to the same code. So, replace __constant_htons
> > > > with htons to get rid of the definition of __constant_htons
> > > > completely.
> > > >
> > > ...
> > > >  static struct packet_type aoe_pt __read_mostly = {
> > > > -	.type = __constant_htons(ETH_P_AOE),
> > > > +	.type = htons(ETH_P_AOE),
> > > >  	.func = aoenet_rcv,
> > >
> > > Does this cause grief if someone is doing a COMPILE_TEST on LE?
> > >
> > > 	David
> > >
> > > -
> > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > Registration No: 1397386 (Wales)
> > >
> > I did COMPILE_TEST on my x86_64 machine and it compiled
> > without any problems.
> > I assume that was the point or am I missing
> > something? I'm a beginner contributor so please
> > let me know if there any mistakes.
>
> It depends on exactly how the bswap functions are defined.
>
> You can't have anything that contains a function call (or asm) for a
> static initialiser or case label.
> The bswap functions have used the builtin_constant() check to switch
> between an asm block and C expression.
> So the C expression is used for constants - so evaluated by the compiler,
> and the asm for variables.
> Last I looked this wasn't valid for initialisers - so the __constant_bswap()
> exists for these cases.
>
> Now, it might be that, on more recent gcc __builtin_bswap() is used.
> This might be valid for constants in initialisers.
> Whether the same is true for CLANG is another issue.
>
> Basically it is all complex and partially historic.
>
> So you may need to use __constant_htons() for initialisers
> and case labels.
> For constants in code it doesn't matter whether you use the __constant form.
> For variables in code the __constant form is likely to generate horrid code.
>
> FWIW you'll get more credit for finding real bugs than stylistic changes.
>
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
Thanks for thee detail explanation.
Actually I was submitting stylistic changes to get
familiar with email and patch based workflows

Amey

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)