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

[Alberto Garcia]

On Tue 02 Apr 2019 10:51:06 AM CEST, 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.
>> 
>> But you can do it at the beginning of stream_start() without exposing
>> 'bottom' to the callers which, again, is an implementation detail.
>
> 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.

There's already the 'base-node' parameter, and converting 'base' into
'base-node' is already done early on by qmp_block_stream() so it doesn't
make much difference whether the user passed the former or the latter.

> 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.

Well, replacing 'base' with 'bottom' on the public API is a completely
different thing and I'm not sure if it's a good idea.

'base' is used to decide the new backing image after block-stream
completes, and its semantics are quite clear. But that does not mean
that 'base' must be used throughout the implementation of block-stream.

If it turns out that block-stream is best implemented using a 'bottom'
node istead of 'base' then the first thing that stream_start() can do is
calculate 'bottom' and discard 'base'.

Whether you do that at the beginning of stream_start() or at the end of
qmp_block_stream() does not make any difference in practice, the end
result is going to be the same.

The only reason why I'm proposing this is because I don't think 'base'
needs to be removed from the public API and therefore I see 'bottom' as
simply an implementation detail that doesn't need to be exposed outside
stream.c

Berto