Re: [Qemu-devel] [PATCH 3/8] 9pfs: fix P9_NOTAG and P9_NOFID macros

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2618 bytes --]

On Sat, 10 Dec 2016 10:24:35 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 12/10/2016 07:57 AM, Greg Kurz wrote:
> 
> >>> -#define P9_NOTAG    (u16)(~0)
> >>> -#define P9_NOFID    (u32)(~0)
> >>> +#define P9_NOTAG    (uint16_t)(~0)
> >>> +#define P9_NOFID    (uint32_t)(~0)    
> >>
> >> Don't you want to write ((uint16_t)(~0)), to ensure that this expression
> >> can be used as a drop-in in any other syntactical situation?
> >>  
> > 
> > These defines come from the linux kernel sources and I must admit it
> > didn't cross my mind... can you share a case where this would cause
> > troubles ?  
> 
> Unlikely to occur in real code, but:
> 
>   int a[] = { -2, -3 };
>   int *b = a + 1;
>   printf("%d\n", (uint16_t)(~0)[b]); // prints 65534 - let's see why?
> 
>   // prints 65534, or the result of b[-1] cast to uint16_t
>   printf("%d\n", (uint16_t)((~0)[b]));
> 
>   // probably dumps core, as b[65535] is out of bounds
>   printf("%d\n", ((uint16_t)(~0))[b]);
> 
> that is, since [] has higher precedence than casts, failure to
> parenthesize a cast will change the interpretation of P9_NOTAG[pointer].
> 

... which is indeed very unlikely to happen even if it is legit. :)

> And yes, if you copied from the kernel, that means the kernel has a bug
> (even if it is unlikely to trip up normal code).
> 

I'll send a patch there too.

> 
> >   
> >> Or even write it as UINT16_C(~0) (using <stdint.h>), or as UINT16_MAX.
> >> (Be aware: the type of (uint16_t)(~0) is uint16_t, while the type of
> >> UINT16_MAX is int, due to the rules of integer promotion, if that matters)
> >>  
> > 
> > UINT16_C(~0) expands to ~0 and UINT16_MAX expands to (65535), at least on
> > my laptop (glibc-headers-2.23.1-11.fc24.x86_64)... doesn't that mean the
> > type of UINT16_C(~0) is also int ? Please enlighten me.  
> 
> Indeed, UINT16_C produces an int constant, not uint16_t (since there is
> no such thing as a uint16_t constant).  So the cast is the only way to
> force ~0 to be truncated to a 16-bit pattern.  But using UINT16_MAX is
> probably just fine, as it is the all-ones value with the correct integer
> promotion for use in any other arithmetic.
> 
> > 
> > The 9P spec at http://man.cat-v.org/plan_9/5/version says "(ushort)~0". My
> > understanding is 16 bits all ones. I guess I'd rather then go for
> > ((uint16_t)(~0)).  
> 
> Verbose, but works, as does UINT16_MAX.  But I stand corrected that
> UINT16_C(~0) does not work.
> 

Ok, I'll go the UINT16_MAX way then.

Thanks for the detailed explanation.

Cheers.

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]