[PATCH 0/2] block/block-copy: Fix uninitialized variable in block_copy_task_entry

[PATCH 0/2] block/block-copy: Fix uninitialized variable in block_copy_task_entry

[Philippe Mathieu-Daudé]

block_copy_task_entry() might be written differently to avoid
the initialization. This silents the warning and let me build.

Philippe Mathieu-Daudé (2):
  block/block-copy: Fix uninitialized variable in block_copy_task_entry
  block/block-copy: Simplify block_copy_do_copy()

 block/block-copy.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

-- 
2.21.3

[PATCH 1/2] block/block-copy: Fix uninitialized variable in block_copy_task_entry

[Philippe Mathieu-Daudé]

Fix when building with -Os:

    CC      block/block-copy.o
  block/block-copy.c: In function ‘block_copy_task_entry’:
  block/block-copy.c:428:38: error: ‘error_is_read’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
    428 |         t->call_state->error_is_read = error_is_read;
        |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/block-copy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 03500680f7..83e16c89d9 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -418,7 +418,7 @@ out:
 static coroutine_fn int block_copy_task_entry(AioTask *task)
 {
     BlockCopyTask *t = container_of(task, BlockCopyTask, task);
-    bool error_is_read;
+    bool error_is_read = false;
     int ret;
 
     ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,
-- 
2.21.3

[PATCH 2/2] block/block-copy: Simplify block_copy_do_copy()

[Philippe Mathieu-Daudé]

block_copy_do_copy() is static, only used in block_copy_task_entry
with the error_is_read argument set. No need to check for it,
simplify.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/block-copy.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 83e16c89d9..e8455b817a 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -343,9 +343,7 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
                                     ~BDRV_REQ_WRITE_COMPRESSED);
         if (ret < 0) {
             trace_block_copy_write_zeroes_fail(s, offset, ret);
-            if (error_is_read) {
-                *error_is_read = false;
-            }
+            *error_is_read = false;
         }
         return ret;
     }
@@ -393,9 +391,7 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
     ret = bdrv_co_pread(s->source, offset, nbytes, bounce_buffer, 0);
     if (ret < 0) {
         trace_block_copy_read_fail(s, offset, ret);
-        if (error_is_read) {
-            *error_is_read = true;
-        }
+        *error_is_read = true;
         goto out;
     }
 
@@ -403,9 +399,7 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
                          s->write_flags);
     if (ret < 0) {
         trace_block_copy_write_fail(s, offset, ret);
-        if (error_is_read) {
-            *error_is_read = false;
-        }
+        *error_is_read = false;
         goto out;
     }
 
-- 
2.21.3

Re: [PATCH 1/2] block/block-copy: Fix uninitialized variable in block_copy_task_entry

[Eric Blake]

On 5/7/20 7:11 AM, Philippe Mathieu-Daudé wrote:
> Fix when building with -Os:
> 
>      CC      block/block-copy.o
>    block/block-copy.c: In function ‘block_copy_task_entry’:
>    block/block-copy.c:428:38: error: ‘error_is_read’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      428 |         t->call_state->error_is_read = error_is_read;
>          |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
> 

Looks like -Os triggered different inlining of block_copy_do_copy().  I 
confirm that block_copy_do_copy does NOT initialize error_is_read except 
when returning < 0, but similarly block_copy_task_entry() does not read 
error_is_read except in the same setups.  So it looks like no actual bug 
was triggered, but we can definitely aid the compiler's analysis by 
initializing.

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   block/block-copy.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 2/2] block/block-copy: Simplify block_copy_do_copy()

[Eric Blake]

On 5/7/20 7:11 AM, Philippe Mathieu-Daudé wrote:
> block_copy_do_copy() is static, only used in block_copy_task_entry
> with the error_is_read argument set. No need to check for it,
> simplify.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   block/block-copy.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 0/2] block/block-copy: Fix uninitialized variable in block_copy_task_entry

[Philippe Mathieu-Daudé]

On 5/7/20 2:11 PM, Philippe Mathieu-Daudé wrote:
> block_copy_task_entry() might be written differently to avoid
> the initialization. This silents the warning and let me build.
> 
> Philippe Mathieu-Daudé (2):
>    block/block-copy: Fix uninitialized variable in block_copy_task_entry
>    block/block-copy: Simplify block_copy_do_copy()
> 
>   block/block-copy.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 

ping?

Re: [PATCH 0/2] block/block-copy: Fix uninitialized variable in block_copy_task_entry

[Kevin Wolf]

Am 07.05.2020 um 14:11 hat Philippe Mathieu-Daudé geschrieben:
> block_copy_task_entry() might be written differently to avoid
> the initialization. This silents the warning and let me build.

Thanks, applied to the block branch.

Kevin