[PULL,00/30] KVM, build, NBD, SCSI patches for 2016-06-16

Commit Message

Paolo Bonzini June 16, 2016, 4:29 p.m. UTC

On 16/06/2016 18:02, Peter Maydell wrote:
> Hi. I'm afraid this generates format string warnings on OSX:

Interesting, I did test clang this time.  I'll fix it but really this 
is a compiler bug.  It's *impossible* to pass a short variable 
argument, hence va_arg(ap, short) *must* be the same as va_arg(ap, int).

I should start making a list of pointless clang warnings.

Paolo



> /Users/pm215/src/qemu-for-merges/nbd/server.c:580:34: warning: format
> specifies type 'unsigned short' but the argument has type 'unsigned
> int' [-Wformat]
>               client->exp->size, client->exp->nbdflags | myflags);
>                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /Users/pm215/src/qemu-for-merges/nbd/nbd-internal.h:44:21: note:
> expanded from macro 'TRACE'
>         LOG(msg, ## __VA_ARGS__); \
>                     ^
> /Users/pm215/src/qemu-for-merges/nbd/nbd-internal.h:50:50: note:
> expanded from macro 'LOG'
>             __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
>                                                  ^
> /Users/pm215/src/qemu-for-merges/nbd/server.c:611:34: warning: format
> specifies type 'unsigned short' but the argument has type 'unsigned
> int' [-Wformat]
>               client->exp->size, client->exp->nbdflags | myflags);
>                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /Users/pm215/src/qemu-for-merges/nbd/nbd-internal.h:44:21: note:
> expanded from macro 'TRACE'
>         LOG(msg, ## __VA_ARGS__); \
>                     ^
> /Users/pm215/src/qemu-for-merges/nbd/nbd-internal.h:50:50: note:
> expanded from macro 'LOG'
>             __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
>                                                  ^
>   CC    nbd/client.o
>   CC    nbd/common.o
>   CC    block/curl.o
> /Users/pm215/src/qemu-for-merges/nbd/client.c:715:57: warning: format
> specifies type 'unsigned short' but the argument has type 'uint32_t'
> (aka 'unsigned int') [-Wformat]
>           request->from, request->len, request->handle, request->type);
>                                                         ^~~~~~~~~~~~~
> /Users/pm215/src/qemu-for-merges/nbd/nbd-internal.h:44:21: note:
> expanded from macro 'TRACE'
>         LOG(msg, ## __VA_ARGS__); \
>                     ^
> /Users/pm215/src/qemu-for-merges/nbd/nbd-internal.h:50:50: note:
> expanded from macro 'LOG'
>             __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
>                                                  ^

Comments

Peter Maydell June 16, 2016, 4:55 p.m. UTC | #1

On 16 June 2016 at 17:29, Paolo Bonzini <[e-mail]> wrote:
> On 16/06/2016 18:02, Peter Maydell wrote:
>> Hi. I'm afraid this generates format string warnings on OSX:
>
> Interesting, I did test clang this time.  I'll fix it but really this
> is a compiler bug.  It's *impossible* to pass a short variable
> argument, hence va_arg(ap, short) *must* be the same as va_arg(ap, int).

This one's an OSX/Apple special -- it isn't part of the stock clang.
(It has its good points -- this is the only 64-bit platform that
will warn about format string issues that will break compile on
32-bit hosts.)

I guess the point of the warning in this particular case is to
say "you passed something that could be wider than a short but
you used a format specifier which is for a short/unsigned short"
(ie your format might be unintentionally dropping the top 16 bits
of the data).

thanks
-- PMM

Paolo Bonzini June 16, 2016, 5:01 p.m. UTC | #2

On 16/06/2016 18:55, Peter Maydell wrote:
>> > Interesting, I did test clang this time.  I'll fix it but really this
>> > is a compiler bug.  It's *impossible* to pass a short variable
>> > argument, hence va_arg(ap, short) *must* be the same as va_arg(ap, int).
> This one's an OSX/Apple special -- it isn't part of the stock clang.
> (It has its good points -- this is the only 64-bit platform that
> will warn about format string issues that will break compile on
> 32-bit hosts.)
> 
> I guess the point of the warning in this particular case is to
> say "you passed something that could be wider than a short but
> you used a format specifier which is for a short/unsigned short"
> (ie your format might be unintentionally dropping the top 16 bits
> of the data).

Does it fire even of

    void f(unsigned short x)
    {
       printf ("%hx", x | 1);
    }

?

Because technically that's an int too (and that's why I think this
particular warning is not a great thing)...

Paolo

Peter Maydell June 16, 2016, 5:08 p.m. UTC | #3

On 16 June 2016 at 18:01, Paolo Bonzini <[e-mail]> wrote:
> Does it fire even of
>
>     void f(unsigned short x)
>     {
>        printf ("%hx", x | 1);
>     }
>
> ?
>
> Because technically that's an int too (and that's why I think this
> particular warning is not a great thing)...

Yes, it does. If you want to shut it up then you can do
(unsigned short)(x | 1).

-- PMM

Patch

diff --git a/nbd/server.c b/nbd/server.c
index ba950973..a677e26 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -576,7 +576,7 @@  static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
     oldStyle = client->exp != NULL && !client->tlscreds;
     if (oldStyle) {
         assert ((client->exp->nbdflags & ~65535) == 0);
-        TRACE("advertising size %" PRIu64 " and flags %" PRIx16,
+        TRACE("advertising size %" PRIu64 " and flags %x",
               client->exp->size, client->exp->nbdflags | myflags);
         stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
         stq_be_p(buf + 16, client->exp->size);
@@ -607,7 +607,7 @@  static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
         }
 
         assert ((client->exp->nbdflags & ~65535) == 0);
-        TRACE("advertising size %" PRIu64 " and flags %" PRIx16,
+        TRACE("advertising size %" PRIu64 " and flags %x",
               client->exp->size, client->exp->nbdflags | myflags);
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);