Re: [Qemu-block] [PATCH v3 04/20] stream: Switch stream_run() to byte-based

[Eric Blake]

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 <address@hidden>
>> Reviewed-by: John Snow <address@hidden>
>>
>> ---
>> 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