On 07/04/2017 10:00 AM, Kevin Wolf wrote: > Am 27.06.2017 um 21:24 hat Eric Blake geschrieben: >> We are gradually converting to byte-based interfaces, as they are >> easier to reason about than sector-based. Change the internal >> loop iteration of streaming to track by bytes instead of sectors >> (although we are still guaranteed that we iterate by steps that >> are sector-aligned). >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >> Reviewed-by: John Snow <js...@redhat.com> >> >> --- >> v2: no change >> --- >> block/stream.c | 24 ++++++++++-------------- >> 1 file changed, 10 insertions(+), 14 deletions(-) >> >> diff --git a/block/stream.c b/block/stream.c >> index 746d525..2f9618b 100644 >> --- a/block/stream.c >> +++ b/block/stream.c >> @@ -108,12 +108,11 @@ static void coroutine_fn stream_run(void *opaque) >> BlockBackend *blk = s->common.blk; >> BlockDriverState *bs = blk_bs(blk); >> BlockDriverState *base = s->base; >> - int64_t sector_num = 0; >> - int64_t end = -1; > > Here, end was initialised to -1. This made a differnce for early 'goto > out' paths because otherwise data->reached_end would incorrectly be true > in stream_complete.
Oh, good call. > > Because we also check data->ret, I think the only case where it actually > makes a difference is for the !bs->backing case: This used to result in > data->reached_end == false, but now it ends up as true. This is because > s->common.len hasn't been set yet, so it is still 0. When !bs->backing, ret is 0; so that means my commit message is wrong about there being no semantic change. But remember, when !bs->backing, stream is a no-op (there was no backing file to stream into the current layer, anyways) - so which do we want, declaring that the operation never reached the end (odd, since we did nothing), or that it is complete? In other words, is this something where I should fix the semantics (perhaps as a separate bug-fix patch), or where I need to fix this patch to preserve existing semantics? The next code reached is stream_complete(). Before my patch, it skipped the main body of the function (because !data->reached_end); with my patch, we are now calling bdrv_change_backing_file() (presumably a no-op when there is no backing?) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature