Re: [Qemu-devel] [PATCH 0/4] block: Avoid copy-on-read assertions

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]

Am 01.10.2017 um 00:05 hat Eric Blake geschrieben:
> On 09/30/2017 04:19 PM, no-reply@patchew.org wrote:
> > Hi,
> > 
> > This series failed build test on s390x host. Please find the details below.
> > 
> 
> > /var/tmp/patchew-tester-tmp-a2p2tpcc/src/block/io.c: In function ‘bdrv_aligned_preadv’:
> > /var/tmp/patchew-tester-tmp-a2p2tpcc/src/block/io.c:955:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >      int ret;
> >          ^~~
> 
> Blah - I compiled with -g instead of -O2, which masks this warning in my
> setup.
> 
> The warning is a false negative (the error message is actually pointing
> to a line in bdrv_co_do_copy_on_readv - but the compiler must have
> inlined it into bdrv_aligned_preadv) - the function is only ever called
> with non-zero bytes, and therefore the 'while (cluster_bytes)' loop will
> execute at least once, and ret always gets assigned.  But the compiler
> can't see that, so I'll squash this in:

Well, you could help the compiler with this:

    assert(cluster_bytes > 0);

Then it compiles. Unfortunately, the compiler was right and you weren't:

    $ ./qemu-io -C -c 'read 0 0' /tmp/test.qcow2
    qemu-io: block/io.c:988: bdrv_co_do_copy_on_readv: Assertion `cluster_bytes > 0' failed.
    Abgebrochen (Speicherabzug geschrieben)

Maybe a case to add to the test?

> commit a201636c3133827bd632d5fdd9eb1f5df81d0e0e
> Author: Eric Blake <eblake@redhat.com>
> Date:   Sat Sep 30 14:27:51 2017 -0500
> 
>     fixup! block: Perform copy-on-read in loop
> 
>     Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> diff --git a/block/io.c b/block/io.c
> index 5ef5adc7a7..e7519464bb 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -952,7 +952,7 @@ static int coroutine_fn
> bdrv_co_do_copy_on_readv(BdrvChild *child,
>      int64_t cluster_offset;
>      unsigned int cluster_bytes;
>      size_t skip_bytes;
> -    int ret;
> +    int ret = 0;
>      int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
>                                      BDRV_REQUEST_MAX_BYTES);
>      unsigned int progress = 0;

I would prefer a ret = 0 immediately before err: so that we'll still get
warning if we forget assigning ret in any future error path.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]