Re: [Qemu-devel] [PATCH v2 06/12] block: Deep-clear inherits_from

[Max Reitz]

On 16.07.19 19:01, Kevin Wolf wrote:
> Am 03.07.2019 um 19:28 hat Max Reitz geschrieben:
>> BDS.inherits_from does not always point to an immediate parent node.
>> When launching a block job with a filter node, for example, the node
>> directly below the filter will not point to the filter, but keep its old
>> pointee (above the filter).
>>
>> If that pointee goes away while the job is still running, the node's
>> inherits_from will not be updated and thus point to garbage.  To fix
>> this, bdrv_unref_child() has to check not only the parent node's
>> immediate children for nodes whose inherits_from needs to be cleared,
>> but its whole subtree.
>>
>> Signed-off-by: Max Reitz <address@hidden>
> 
> Isn't the real bug that we keep pointing to a node that isn't a parent
> of the node any more? I think this will effectively disable option
> inheritance in bdrv_reopen() as long as the filter node is present,
> which is certainly not what we intended.

Well, it breaks it while a block job is running.  I don’t know whether I
would advise doing a reopen across a block job filter.  It’s a case of
“Why wouldn’t it work?”, but I’m sure there’s something that doesn’t.
(Like this here, for example, but it at least has the decency of just
letting the reopen fail.)

> The intuitive thing would be that after inserting a filter, the image
> now inherits from the filter node, and when the filter is removed, it
> inherits from the filter's bs->inherit_from if that becomes a parent of
> the node. (Though I'm not necessarily saying that my intuition is to be
> trusted here.)

I tried that first, but I couldn’t get it to work.  I don’t remember
why, though.  I suppose my problem was that removing the filter node
make inherits_from NULL.  I guess I stopped at that point and went this
route instead.

I suppose we could add a flag to the BDS that says whether an heir
node’s inherits_from should be cleared or set to the bequeather’s
inherits_from, like so:

    if (parent->inherit_inherits_from) {
        child->bs->inherits_from = parent->inherits_from;
    } else {
        child->bs->inherits_from = NULL;
    }

And then, if you insert a node between a child and its inherits_from
parent, that node copies inherits_from from the child and gets its
inherit_inherits_from set to true.

The problem I see is that it doesn’t always appear clear to me that this
intermediate node should actually copy its inherits_from from the child.

So the same question applies here, too, I guess; should the filter node
even inherit its options from the parent?

Max

signature.asc
Description: OpenPGP digital signature