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

[Eric Blake]

On 10/02/2017 09:50 AM, Kevin Wolf wrote:

>> 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?

Of course - that's a good test to have.

So it turns out the reason we get in with 0 length is:

        ret = bdrv_is_allocated(bs, offset, bytes, &pnum);
        if (ret < 0) {
            goto out;
        }

        if (!ret || pnum != bytes) {
            ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov);
            goto out;
        }

because bdrv_is_allocated() returns 0 for a 0-length query.  I wonder if
we should instead assert that bdrv_is_allocated/bdrv_get_block_status()
are given a non-zero size, and fix callers to avoid a 0-length query
(after all, it's tough to argue that the read is allocated in the
current layer or defers to a backing layer, when there is nothing to be
read).

>> +++ 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.

Sure, I can take that approach instead.  v2 coming up, after a bit more
of a wait for any other review comments on the main patch rather than
just this fixup.

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

signature.asc
Description: OpenPGP digital signature