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 <jc...@redhat.com> > --- > 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 > +@item change_backing_file > +@findex 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