Re: [Qemu-devel] [PATCH 3/3] block/stream: introduce a bottom node

[Vladimir Sementsov-Ogievskiy]

29.03.2019 19:07, Alberto Garcia wrote:
> On Fri 29 Mar 2019 02:29:14 PM CET, Andrey Shinkevich wrote:
>> @@ -3237,7 +3238,14 @@ void qmp_block_stream(bool has_job_id, const char 
>> *job_id, const char *device,
>>           job_flags |= JOB_MANUAL_DISMISS;
>>       }
>>   
>> -    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
>> +    /* Find the bottom node that has the base as its backing image */
>> +    bottom_node = bs;
>> +    while ((iter = backing_bs(bottom_node)) != base_bs) {
>> +        bottom_node = iter;
>> +    }
>> +    assert(bottom_node);
>> +
>> +    stream_start(has_job_id ? job_id : NULL, bs, bottom_node, base_name,
>>                    job_flags, has_speed ? speed : 0, on_error, &local_err);
> 
> Isn't it simpler to pass 'base' to stream_start() and find the bottom
> node there? (with bdrv_find_overlay()).
> 
> I think bottom should be an internal implementation detail of the
> block-stream driver, callers don't need to know about it, or do they?
> 

My idea is that we should get rid of base before any yield, and better do it as
soon as possible.
Better to get rid of it in qapi interface, but we can't do it due to backward
compatibility.
So, I'd prefer to not deal with base in block/stream.c code at all, and only
qmp_block_stream is responsible to recalculate it's parameters to something more
meaningful before any context switch.


-- 
Best regards,
Vladimir