Re: [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command

[Eric Blake]

On 1/17/19 9:33 AM, Alberto Garcia wrote:

> 
> Changing options
> ================
> The general idea is quite straightforward, but the devil is in the
> details. Since this command tries to mimic blockdev-add, the state of
> the block device after being reopened should generally be equivalent
> to that of a block device added with the same set of options.
> 
> There are two important things with this:
> 
>   a) Some options cannot be changed (most drivers don't allow that, in
>      fact).
>   b) If an option is missing, it should be reset to its default value
>      (rather than keeping its previous value).

Could we use QAPI alternate types where we could state that:

"option":"new" - set the option
"option":null - reset the option to its default
omit option - leave the option unchanged

basically, making each of the options an Alternate with JSON null, so
that a request to reset to the default becomes an explicit action?

> 
> Example: the default value of l2-cache-size is 1MB. If we set a
> different value (2MB) on blockdev-add but then omit the option in
> x-blockdev-reopen, then it should be reset back to 1MB. The current
> bdrv_reopen() code keeps the previous value.
> 
> Trying to change an option that doesn't allow it is already being
> handled by the code. The loop at the end of bdrv_reopen_prepare()
> checks all options that were not handled by the block driver and sees
> if any of them has been modified.
> 
> However, as I explained earlier in (b), omitting an option is supposed
> to reset it to its default value, so it's also a way of changing
> it. If the option cannot be changed then we should detect it and
> return an error. What I'm doing in this series is making every driver
> publish its list of run-time options, and which ones of them can be
> modified.
> 
> Backing files
> =============
> This command allows reconfigurations in the node graph. Currently it
> only allows changing an image's backing file, but it should be
> possible to implement similar functionalities in drivers that have
> other children, like blkverify or quorum.
> 
> Let's add an image with its backing file (hd1 <- hd0) like this:
> 
>     { "execute": "blockdev-add",
>       "arguments": {
>           "driver": "qcow2",
>           "node-name": "hd0",
>           "file": {
>               "driver": "file",
>               "filename": "hd0.qcow2",
>               "node-name": "hd0f"
>           },
>           "backing": {
>               "driver": "qcow2",
>               "node-name": "hd1",
>               "file": {
>                   "driver": "file",
>                   "filename": "hd1.qcow2",
>                   "node-name": "hd1f"
>               }
>           }
>        }
>     }
> 
> Removing the backing file can be done by simply passing the option {
> ..., "backing": null } to x-blockdev-reopen.
> 
> Replacing it is not so straightforward. If we pass something like {
> ..., "backing": { "driver": "foo" ... } } then x-blockdev-reopen will
> assume that we're trying to change the options of the existing backing
> file (and it will fail in most cases because it'll likely think that
> we're trying to change the node name, among other options).
> 
> So I decided to use a reference instead: { ..., "backing": "hd2" },
> where "hd2" is of an existing node that has been added previously.
> 
> Leaving out the "backing" option can be ambiguous on some cases (what
> should it do? keep the current backing file? open the default one
> specified in the image file?) so x-blockdev-reopen requires that this
> option is present. For convenience, if the BDS doesn't have a backing
> file then we allow leaving the option out.

Hmm - that makes my proposal of "option":null as an explicit request to
the default a bit trickier, if we are already using null for a different
meaning for backing files.

> 
> As you can see in the patch series, I wrote a relatively large amount
> of tests for many different scenarios, including some tricky ones.
> 
> Perhaps the part that I'm still not convinced about is the one about
> freezing backing links to prevent them from being removed, but the
> implementation was quite simple so I decided to give it a go. As
> you'll see in the patches I chose to use a bool instead of a counter
> because I couldn't think of a case where it would make sense to have
> two users freezing the same backing link.
> 
> Thanks,
> 
> Berto
> 


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature