| Message ID | 148127568042.2633.5084932697061472826.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
On 12/09/2016 03:28 AM, Greg Kurz wrote: > The u16 and u32 types don't exist in QEMU common headers. It never broke > build because these two macros aren't use by the current code, but this > is about to change with the future addition of functional tests for 9P. > > This patch convert the types to uintXX_t. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h > index 3976b7fe3dcd..89c904bdb7e7 100644 > --- a/hw/9pfs/9p.h > +++ b/hw/9pfs/9p.h > @@ -99,8 +99,8 @@ enum p9_proto_version { > V9FS_PROTO_2000L = 0x02, > }; > > -#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? 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)
On Fri, 9 Dec 2016 15:25:55 -0600 Eric Blake <eblake@redhat.com> wrote: > On 12/09/2016 03:28 AM, Greg Kurz wrote: > > The u16 and u32 types don't exist in QEMU common headers. It never broke > > build because these two macros aren't use by the current code, but this > > is about to change with the future addition of functional tests for 9P. > > > > This patch convert the types to uintXX_t. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h > > index 3976b7fe3dcd..89c904bdb7e7 100644 > > --- a/hw/9pfs/9p.h > > +++ b/hw/9pfs/9p.h > > @@ -99,8 +99,8 @@ enum p9_proto_version { > > V9FS_PROTO_2000L = 0x02, > > }; > > > > -#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 ? > 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. 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)). Thanks! -- Greg
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]. 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). > >> 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.
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
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h index 3976b7fe3dcd..89c904bdb7e7 100644 --- a/hw/9pfs/9p.h +++ b/hw/9pfs/9p.h @@ -99,8 +99,8 @@ enum p9_proto_version { V9FS_PROTO_2000L = 0x02, }; -#define P9_NOTAG (u16)(~0) -#define P9_NOFID (u32)(~0) +#define P9_NOTAG (uint16_t)(~0) +#define P9_NOFID (uint32_t)(~0) #define P9_MAXWELEM 16 #define FID_REFERENCED 0x1
The u16 and u32 types don't exist in QEMU common headers. It never broke build because these two macros aren't use by the current code, but this is about to change with the future addition of functional tests for 9P. This patch convert the types to uintXX_t. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)