Re: [Qemu-devel] [PATCH v2 11/11] block: add QAPI command to allow live backing file change

[Eric Blake]

On 05/27/2014 08:28 AM, Jeff Cody wrote:
> This allows a user to change the backing file live, of an open
> image.

Grammar; maybe:

This allows a user to make a live change to the backing file recorded in
an open image.

> 
> The image file to modify can be specified 2 ways:
> 
> 1) 'device' string, and image filename
> 2) image node-name
> 
> Note: this does not cause the backing file itself to be reopened; it
> merely changes the backing filename in the image file structure, and
> in internal BDS structures.
> 
> It is the responsibility of the user to pass a filename string that
> can be resolved when the image chain is reopened, and the filename
> string is not validated.
> 
> A good analogy for this command is that it is a live version of
> 'qemu-img rebase -u', with respect to change the backing file string.

s/change/changing/

Design question before I read the patch (I may answer myself later on):

'qemu-img rebase -u' can be used to intentionally drop a backing file.
This is a dangerous operation, because it _usually_ changes the contents
that a guest would see; but there are special cases where it works: 1.
if the backing file has no non-NUL blocks, then dropping the backing
file makes no difference; 2. if all non-NUL blocks of the backing file
have already been copied into the active layer or been overwritten with
new data by copy-on-write, then dropping the backing file makes no
difference.

Conversely, 'qemu-img rebase -u' can be used to add a backing file on an
image that used to be the base of a chain.  Special cases where it has
no guest impact are similar: basically, as long as the backing file
reads as all 0 blocks for any block where the active file has nothing
allocated.

BUT - those unsafe operations make sense in qemu-img where the file
being modified is NOT in use by a guest; the more common case is that
doing that sort of action changes the guest view of data, although the
user must have a reason for making such a change.  However, for an
active qemu process, hanging what the guest sees while the guest is
running is inappropriate.  So, I'm hoping that your patch forbids an
attempt to delete the backing file name, and conversely, that you forbid
an attempt to set a backing file name for a file that currently has no
backing image.

> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  blockdev.c       | 118 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx  |  16 ++++++++
>  hmp.c            |  16 ++++++++
>  hmp.h            |   1 +
>  qapi-schema.json |  57 +++++++++++++++++++++++++++
>  qmp-commands.hx  |  70 +++++++++++++++++++++++++++++++++
>  6 files changed, 278 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 81d1383..2885f2f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2438,6 +2438,124 @@ void qmp_block_job_complete(const char *device, Error 
> **errp)
>      block_job_complete(job, errp);
>  }
>  
> +void qmp_change_backing_file(bool has_device, const char *device,
> +                             bool has_image, const char *image,
> +                             bool has_image_node_name,
> +                             const char *image_node_name,
> +                             const char *backing_file,

Okay, answering myself, part 1 - looks like you made backing_file
mandatory (no deleting a backing file, even though qemu-img can do it).
 Good.


> +
> +    if (!has_image && !has_image_node_name) {
> +        error_setg(errp, "either 'image' or 'image-node-name' must be "
> +                         "specified");
> +        return;
> +    }

Any reason why you can't allow 'device' with missing
image/image-node-name just default to acting on the active image,
instead of being a hard error?  (similar to the implied 'top' of
block-commit)

> +
> +    if (bdrv_find_base(image_bs) == image_bs) {
> +        error_setg(errp, "not allowing backing file change on an image "
> +                         "without a backing file");
> +        return;
> +    }

Answering myself, part 2 - good, you don't allow creating a backing
chain where one did not previously exist, even though qemu-img can do it.

> +++ b/hmp-commands.hx
> @@ -89,6 +89,22 @@ Copy data from a backing file into a block device.
>  ETEXI
>  
>      {
> +        .name       = "change_backing_file",
> +        .args_type  = "device:s?,image:s?,image_node_name:s?,backing_file:s",

Umm, does HMP really allow optional arguments followed by a mandatory
argument?

> +        .params     = "[device image] [image_node_name] backing_file",

Worth writing as '[device] [image]'?

I haven't commented much on the HMP interface in the rest of this series
(because libvirt won't be using it), but wouldn't it make more sense for
HMP to have some intelligence built in, and look more like:

"[device] [image-name-or-node] backing_file"

where [image-name-or-node] can be either a file name or a node name, and
where HMP tries to look up both variants before giving up?  Just because
the QMP command has them as two separate arguments doesn't mean that HMP
can't add some syntactic sugar to just DWIM.

> +        .help       = "change the backing file of an image file",
> +        .mhandler.cmd = hmp_change_backing_file,
> +    },
> +
> +STEXI
> address@hidden change_backing_file
> address@hidden change_backing_file
> +Chaning the backing file of an image.  This will change the backing

s/Chaning/Changing/

> +file metadata in an image file.  You must specify either 'device' and
> +'image', or just 'image-node-name'.
> +ETEXI
> +
> +    {
>          .name       = "block_job_set_speed",
>          .args_type  = "device:B,speed:o",
>          .params     = "device speed",
> diff --git a/hmp.c b/hmp.c
> index 69dd4f5..154b379 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1183,6 +1183,22 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &error);
>  }
>  
> +void hmp_change_backing_file(Monitor *mon, const QDict *qdict)
> +{
> +    Error *error = NULL;
> +    const char *device          = qdict_get_str(qdict, "device");
> +    const char *image           = qdict_get_str(qdict, "image");
> +    const char *image_node_name = qdict_get_str(qdict, "image_node_name");

You listed these three as optional in the .hx file; do you need the _try
variant here?  Again, I'm wondering if HMP should be a bit smarter and
automatically determine whether a single parameter is the file name or a
node name.  I'm _also_ okay with completely skipping HMP and requiring
the use of QMP to access this functionality - changing the backing file
is potentially dangerous, and may not be something we need to expose
from HMP in the first place.

> +++ b/qapi-schema.json
> @@ -2092,6 +2092,63 @@
>    'returns': 'str' }
>  
>  ##
> +# @change-backing-file
> +#
> +# Change the backing file in the image file metadata.  This does not affect
> +# the internal data structures of the currently running QEMU instance, but
> +# writes changes the backing file string in the image file header metadata.

Grammar.  Maybe s/writes changes/writes the changed/

> +#
> +# The image file to perform the operation on can be specified by two 
> different
> +# methods:
> +#
> +#  Method 1: Supply the device name (e.g. 'virtio0'), and the image
> +#            filename.  This would use arguments @device and @image

Again, any reason the image filename can't be optional (and default to
the active image in that case)

> +#
> +#  Method 2: Supply the node-name of the image to modify, via 
> @image-node-name
> +#
> +# Either @image or @image-node-name must be set but not both.
> +#
> +# If @image is specified, @device must be specified as well.
> +#
> +# Method 1 interface
> +#---------------------
> +# @device:         #optional The name of the device.  If @image is specified,
> +#                            then @device is required.  If @image-node-name 
> is
> +#                            specified instead, @device is optional and used 
> to
> +#                            validate @image-node-name
> +#
> +# @image:          #optional The file name of the image to modify
> +#
> +# Method 2 interface
> +#---------------------
> +# @image-node-name #optional The name of the block driver state node of the
> +#                            image to modify.
> +#
> +# Common arguments
> +#---------------------
> +# @backing-file:    The string to write as the backing file.  This string is
> +#                   not validated, so care should be taken when specifying
> +#                   the string or the image chain may not be able to be
> +#                   reopened again.
> +#
> +#                   If a pathname string is such that it cannot be
> +#                   resolved be QEMU, that means that subsequent QMP or
> +#                   HMP commands must use node-names for the image in
> +#                   question, as filename lookup methods will fail.
> +#
> +#
> +# Returns: Nothing on success
> +#          If @device does not exist or cannot be determined, DeviceNotFound
> +#          If @image is specified, but not @device, GenericError
> +#          If both @image and @image-node-name are specified, GenericError
> +#
> +# Since: 2.1
> +#
> +{ 'command': 'change-backing-file',

The last line of the comment block is usually ## in this file.

> +Change the backing file in the image file metadata.  This does not affect
> +the internal data structures of the currently running QEMU instance, but
> +writes changes the backing file string in the image file header metadata.

Same copy-and-paste grammar issue.

I like that this command serves as a witness of whether block-commit and
block-stream have been enhanced to support specifying alternative
backing names.  Even though libvirt will probably prefer to change
backing names as part of other operations rather than in isolation (and
therefore never directly use this command), it is worth having because
of its use as a feature probe.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature