[ovs-dev] [PATCH] ofp-meter: Fix ds_put_format that treats enum type as short integer

Ben Pfaff blp at ovn.org
Wed Jun 27 02:14:41 UTC 2018


Yes.  I should have said %x instead of %u.

On Tue, Jun 26, 2018 at 07:13:33PM -0700, Yifeng Sun wrote:
> Thanks for the clarification. The document about enum type
> is quite confusing and it is difficult to get it right.
> 
> How about use %x plus a cast to unsigned? I think a hex output
> is more intuitive for flags, what do you think?
> 
> Best,
> Yifeng
> 
> On Tue, Jun 26, 2018 at 6:16 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> > On Tue, Jun 26, 2018 at 10:30:30AM -0700, Yifeng Sun wrote:
> > > Travis job fails because of the below error:
> > >
> > > lib/ofp-meter.c:340:48: error: format specifies type 'unsigned short'
> > > but the argument has underlying type 'unsigned int' [-Werror,-Wformat]
> > >         ds_put_format(s, "flags:0x%"PRIx16" ", flags);
> > >
> > > This patch fixes it by treating enum type as int type.
> > > (6.7.2.2 Enumerationspecifiers)
> >
> > Thank you for the patch.
> >
> > This citation from C99 looks like a misunderstanding.  In standard C
> > (all editions), the members of an enumeration (the constants) have type
> > int, as paragraph 3 says:
> >
> >     The identifiers in an enumerator list are declared as constants that
> >     have type int and may appear wherever such are permitted.
> >
> > However, the enumeration type itself is not necessarily int.  Instead,
> > paragraph 4 says:
> >
> >     Each enumerated type shall be compatible with char, a signed integer
> >     type, or an unsigned integer type. The choice of type is
> >     implementation-defined...
> >
> > This means that, from a standards perspective, which is the correct type
> > here will differ from one implementation to another.  It's not even
> > necessarily a type that promotes to 'int' (it could be "unsigned int" or
> > even wider than int).  It's a weird choice but that's what the standard
> > says.
> >
> > The particular patch here doesn't seem to match up to the description,
> > either.  To treat them as int, one would want to use %x, since that's
> > the format specifier for (unsigned) int, but this uses PRIx32, which
> > isn't necessarily for int (it could be for long int, for example).
> >
> > >      if (flags) {
> > > -        ds_put_format(s, "flags:0x%"PRIx16" ", flags);
> > > +        ds_put_format(s, "flags:0x%"PRIx32" ", flags);
> > >      }
> >
> > I think that a correct fix here would be to use %u plus a cast to
> > unsigned.
> >
> > Thanks,
> >
> > Ben.
> >


More information about the dev mailing list