[Qemu-devel] [PATCH v2] nbd: fix uninitialized variable warning

[Qemu-devel] [PATCH v2] nbd: fix uninitialized variable warning

[Marc-André Lureau]

../block/nbd.c: In function 'nbd_co_request':
../block/nbd.c:745:8: error: 'local_reply.type' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     if (chunk->type == NBD_REPLY_TYPE_NONE) {
        ^
../block/nbd.c:710:14: note: 'local_reply.type' was declared here
     NBDReply local_reply;
              ^~~~~~~~~~~
../block/nbd.c:710:14: error: 'local_reply.flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
../block/nbd.c:738:8: error: 'local_reply.<U4be0>.magic' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     if (nbd_reply_is_simple(reply) || s->quit) {
        ^
../block/nbd.c:710:14: note: 'local_reply.<U4be0>.magic' was declared here
     NBDReply local_reply;
              ^~~~~~~~~~~
cc1: all warnings being treated as errors

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nbd.c b/block/nbd.c
index 81edabbf35..0f74c1e455 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -707,7 +707,7 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
                                          void **payload)
 {
     int ret, request_ret;
-    NBDReply local_reply;
+    NBDReply local_reply = {};
     NBDStructuredReplyChunk *chunk;
     Error *local_err = NULL;
     if (s->quit) {
-- 
2.22.0.428.g6d5b264208

Re: [Qemu-devel] [PATCH v2] nbd: fix uninitialized variable warning

[Philippe Mathieu-Daudé]

I was writing on your v1 "it might be worth commenting this is when
building with -O3"

This might be as easy as having the subject modified (by maintainer
taking this patch?) as:

"fix uninitialized variable warning [when building with -O3]"

On 7/16/19 12:07 PM, Marc-André Lureau wrote:
> ../block/nbd.c: In function 'nbd_co_request':
> ../block/nbd.c:745:8: error: 'local_reply.type' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      if (chunk->type == NBD_REPLY_TYPE_NONE) {
>         ^
> ../block/nbd.c:710:14: note: 'local_reply.type' was declared here
>      NBDReply local_reply;
>               ^~~~~~~~~~~
> ../block/nbd.c:710:14: error: 'local_reply.flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> ../block/nbd.c:738:8: error: 'local_reply.<U4be0>.magic' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      if (nbd_reply_is_simple(reply) || s->quit) {
>         ^
> ../block/nbd.c:710:14: note: 'local_reply.<U4be0>.magic' was declared here
>      NBDReply local_reply;
>               ^~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 81edabbf35..0f74c1e455 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -707,7 +707,7 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
>                                           void **payload)
>  {
>      int ret, request_ret;
> -    NBDReply local_reply;
> +    NBDReply local_reply = {};
>      NBDStructuredReplyChunk *chunk;
>      Error *local_err = NULL;
>      if (s->quit) {
> 

Re: [Qemu-devel] [PATCH v2] nbd: fix uninitialized variable warning

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1494 bytes --]

On 7/16/19 5:21 AM, Philippe Mathieu-Daudé wrote:
> I was writing on your v1 "it might be worth commenting this is when
> building with -O3"
> 
> This might be as easy as having the subject modified (by maintainer
> taking this patch?) as:
> 
> "fix uninitialized variable warning [when building with -O3]"
> 

I can do a touchup (probably to the commit body, rather than the subject).


>> cc1: all warnings being treated as errors
>>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks; will queue through my NBD tree.

>> ---
>>  block/nbd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 81edabbf35..0f74c1e455 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -707,7 +707,7 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
>>                                           void **payload)
>>  {
>>      int ret, request_ret;
>> -    NBDReply local_reply;
>> +    NBDReply local_reply = {};

Relies on the gcc extension rather than the idiomatic C spelling of {0}
(but we rely on it elsewhere, in part because some versions of clang are
buggy on incorrectly warning about the idiomatic spelling, so nothing new)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2] nbd: fix uninitialized variable warning

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2096 bytes --]

On 7/16/19 5:07 AM, Marc-André Lureau wrote:
> ../block/nbd.c: In function 'nbd_co_request':
> ../block/nbd.c:745:8: error: 'local_reply.type' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      if (chunk->type == NBD_REPLY_TYPE_NONE) {
>         ^
> ../block/nbd.c:710:14: note: 'local_reply.type' was declared here
>      NBDReply local_reply;
>               ^~~~~~~~~~~
> ../block/nbd.c:710:14: error: 'local_reply.flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> ../block/nbd.c:738:8: error: 'local_reply.<U4be0>.magic' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      if (nbd_reply_is_simple(reply) || s->quit) {
>         ^
> ../block/nbd.c:710:14: note: 'local_reply.<U4be0>.magic' was declared here
>      NBDReply local_reply;
>               ^~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Actually, since this patch didn't fix all the cases of use of an uninit
variable, I'm wondering if this one-liner would be a better patch for
the issue (that is, fix it so that nbd_co_receive_one_chunk never leaves
reply uninit, rather than chasing down which callers might need init),
and with a perk of not relying on a gcc extension:

diff --git i/block/nbd.c w/block/nbd.c
index 8d565cc624ec..f751a8e633e5 100644
--- i/block/nbd.c
+++ w/block/nbd.c
@@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk(
                                           request_ret, qiov, payload,
errp);

     if (ret < 0) {
+        memset(reply, 0, sizeof *reply);
         s->quit = true;
     } else {
         /* For assert at loop start in nbd_connection_entry */

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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