Re: [Qemu-devel] [PATCH 5/5] block: Simplify append_open_options()

[Alberto Garcia]

On Tue 14 Aug 2018 01:14:42 PM CEST, Kevin Wolf wrote:
>> The main difference is that if you set backing.node-name=foo then it
>> means that 'node-name=foo' is an option of the child, the option
>> doesn't belong in the parent at all. So once it's transferred to the
>> child there's no reason why it shoud remain in the parent. It's
>> redundant and can interfere with the reopen operation (e.g. you
>> change the option in the child but when you reopen the parent it will
>> try to revert the child option to the previous value).
>
> That's a rather reopen-centric point of view, though.
>
> Redundant information isn't nice already, but what really makes it a
> problem is that it's potentially incorrect information because it
> isn't kept up to date. There is no point in keeping incorrect
> information, so I agree that deleting it is best.

Yes, it's indeed reopen-centric :-) I agree with you though, the reopen
problem is a practical example of the consequences of keeping both
options, but the main problem is that they can be different and
therefore wrong.

>> In the case of 'backing=foo' that's clearly an option of the parent,
>> there's no other place to put it. We could probably remove it as
>> well, but we have to see carefully that no parent needs to keep that
>> information. I think we want to keep child references because in
>> general we don't allow them to be changed in reopen.
>> 
>> Example: you cannot pass 'file=bar' on reopen unless 'bar' was
>> already the existing value of 'file' (i.e. you're not changing
>> anything). We need to look for the previous value in bs->options in
>> order to know that.
>
> My point is the backing=foo has the same problem: Storing it in
> bs->options is not only redundant, but potentially incorrect because
> we never update it when we modify the graph. There is no point in
> keeping potentially incorrect information.

I tend to agree, but there's one reason why it's not exactly the same
case: with "backing=foo" we know not only that the backing node name is
'foo', but that it was added using a reference rather than with
backing.node-name=foo. I'm not sure if that's information that we need
for anything, however (probably not).

> When you actually use that incorrect information, you've got a bug.
> Reopen with file=bar doesn't have to check whether 'file=bar' is in
> bs->options (because that may be outdated information), but whether
> the BdrvChild with the name 'file' points to a node called 'bar'.

Again, this is correct if we open the BDS with

  file.node-name=bar

and then we allow reopening it with

  file=bar

Different set of options, but the result still makes sense. For this we
need a specific check to verify that this is correct. For the current
behavior we don't need anything now: we only allow the exact same
option.

>> After x-blockdev-reopen is ready, 'backing' will perhaps be the
>> first/only exception, because we'll be able to change it and the
>> previous value doesn't matter.
>
> I certainly hope that it will not be the only kind of node references
> that you can change. Adding/removing filter nodes requires to be able
> to change more or less any kind of node references. But we'll see.

No, but anything that we allow changing has to be done on a case-by-case
basis. You can't simply allow changing any child reference, the specific
driver has to be modified in order to allow that.

Until the driver is changed, the current behavior prevails: if an option
was modified, we return an error.

Berto