Re: [Qemu-block] bdrv_reopen() and backing images

[Alberto Garcia]

On Wed 17 Aug 2016 05:19:06 PM CEST, Kevin Wolf <address@hidden> wrote:

>> But I would like to fix bdrv_reopen() properly. From what I can see
>> the situation is that bdrv_backing_options() forces all backing BDSs
>> to be opened without the BDRV_O_RDWR flag, which makes sense when you
>> open an image for the first time, but not so much when you reopen it.
>
> The wanted behaviour is, like with all other options, that the option
> stays as it is if it was set explicitly on the node, and to be updated
> if it was inherited from the parent node.
>
> I know that we do get this behaviour e.g. for cache modes. It might be
> that the (hopefully only) reason why we can't have it for read-only
> yet is that the read-only option is still passed in flags rather than
> the QDict.
>
> Converting it to be a "normal" option using the QDict should fix the
> problem then.

Status update: I've been taking a look at this during the past days.

Although I managed to make it work, at least for the specific problem
that I was having, I'm not done yet.

First I looked into removing the BDRV_O_RDWR flag completely. The
problem is that that flag is used in a lot of places all over the QEMU
code. The way to deal with its removal varies depending on the case:

  1. If we're setting the flag, it needs to be converted into a QBool
     and stored in a (possibly new) QDict in order to be passed to
     e.g. bdrv_open(). A bit more work, some APIs need to be adjusted,
     but else it's relatively straightforward.

  2. It can be replaced with bs->read_only. This is the simplest one.

  3. It can be replaced with qemu_opt_get_bool(). This is also simple,
     but it may involve creating the QemuOpts object earlier if it
     doesn't exist yet. It shouldn't be a big deal.

  4. Sometimes we cannot create a QemuOpts object and the only way to
     get that value is from a QDict, which can contain a QBool or a
     QString with "on" / "off". So we'd need additional code to parse
     it. This is rather tedious and I don't like it.

Even if we can get rid of all instances of BDRV_O_RDWR, the problem
with all this is that we'd be replacing very simple operations to
check/set flags with code that is significantly less efficient and
more complicated. There are some functions that nicely transform QEMU
flags into standard POSIX flags that would become more complicated for
no additional benefit (qemu_gluster_parse_flags(), raw_parse_flags()),
and in general I think flags are a better solution for this kind of
boolean properties.

So the other alternative is to add "read-only" as an option and keep
it in sync with the BDRV_O_RDWR flag. This is similar to what commit
91a097e7478940483 did with the cache options.

There's a problem here, mostly related to the bdrv_open_inherit()
code. The goal of this change is to inherit the "read-only" option only
if it hasn't been explicitly set, e.g. with qdict_set_default_str().
However that means that we don't know the new value unless we parse the
QDict (scenario 4 above), and if we don't do it the flags and options
will be (temporarily) out of sync. I think we can deal with that, and
and that's what I'm trying at the moment. It's significantly less code,
although I think it requires moving things from bdrv_open_inherit() to
bdrv_open_common() (so we can use the QemuOpts object).

I'm also wondering: if the only reason why we need to do all this is
to distinguish the flags that were set explicitly from the ones that
were inherited, isn't it simpler and more efficient to use a mask
instead?

    struct BlockDriverState {
        /* ... */
        int open_flags;
        int open_flags_mask;
        /* ... */
    };

I haven't explored this possibility much yet, but intuitively it
doesn't look bad. It would even allow us to tell bdrv_reopen() which
flags we want to alter and which ones we don't want to touch.

Thoughts, comments, ...?

Berto