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