Re: [Qemu-block] [PATCH v9 07/11] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Wed 27 Apr 2016 03:34:19 PM CEST, Max Reitz <address@hidden> wrote:
>> +    /* Look for the top-level node that contains 'bs' in its chain */
>> +    active = NULL;
>> +    do {
>> +        active = bdrv_next(active);
>> +    } while (active && !bdrv_chain_contains(active, bs));
>
> Alternatively, you could iterate up directly from @bs. Just look for
> the BdrvChild in bs->parents with .role == &child_backing.

Yes, but the BdrvChild in bs->parents does not contain any pointer to
the actual parent node, so unless we want to change that structure I
wouldn't go for this solution.

> More interesting question: What happens if you have e.g. a qcow2 file
> as a quorum child, and then want to stream inside of the qcow2 backing
> chain?
>
> So maybe you should first walk up along &child_backing and then along
> &child_file/&child_format. I think a generic bdrv_get_root_bs()
> wouldn't hurt.

You're right. I'm not sure if that would have other consequences that we
should consider, so maybe we can add that later, with its own set of
tests.

Also, this is all necessary because of the problem with bdrv_reopen().
If that is ever fixed there's no need to block the active layer at all.

>> -    stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0,
>> +    stream_start(bs, active, base_bs, base_name, has_speed ? speed : 0,
>
> stream_start() behaves differently for active == NULL and active == bs.
> I think both kinds of behavior work, but it looks weird for active ==
> bs. Should we pass NULL in that case instead?

You're right, I'll change it to pass NULL and assert(active != bs).

Berto