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
