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

[Andrey Shinkevich]

On 01/04/2019 18:44, Alberto Garcia wrote:
> On Fri 29 Mar 2019 05:15:43 PM CET, Vladimir Sementsov-Ogievskiy 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.
> 
> But you can do it at the beginning of stream_start() without exposing
> 'bottom' to the callers which, again, is an implementation detail.
> 
> Berto
> 

We have got a latent trouble with the base involved into the process.
For instance, if we encounter a filter while searching for the base,
we will want to manage it somehow. If the base is identified by the
node name rather than by file name, it would be easier. So, we would
assign the node name to the base earliest as possible. Another approach
suggested by Vladimir supposed to eliminate the base from the interface.
It is a sensitive change and we all need to come to an agreement.
-- 
With the best regards,
Andrey Shinkevich