| Message ID | a5225eb8ea19c882ef1eeba18f7d8c7e20b1fef5.1401200583.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jcody@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.
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, + Error **errp) +{ + BlockDriverState *bs = NULL; + BlockDriverState *image_bs = NULL; + Error *local_err = NULL; + bool ro; + int open_flags; + int ret; + + /* validate argument combinations */ + if (has_image && has_image_node_name) { + error_setg(errp, "'image' and 'image-node-name' " + "are mutually exclusive"); + return; + } + + if (has_image && !has_device) { + error_setg(errp, "'image' specified, but not 'device'"); + return; + } + + if (!has_image && !has_image_node_name) { + error_setg(errp, "either 'image' or 'image-node-name' must be " + "specified"); + return; + } + + /* find the BDS to operate on */ + if (has_device) { + bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + } + } + + if (has_image_node_name) { + image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + } + + if (bs && has_image) { + if (!strcmp(bs->filename, image)) { + image_bs = bs; + } else { + image_bs = bdrv_find_backing_image(bs, image); + } + } + + if (!image_bs) { + error_setg(errp, "image file not found"); + return; + } + + bs = bs ?: bdrv_find_active(image_bs); + if (!bs) { + error_setg(errp, "could not find active layer for '%s'", + image_bs->filename); + return; + } + + if (bdrv_find_base(image_bs) == image_bs) { + error_setg(errp, "not allowing backing file change on an image " + "without a backing file"); + return; + } + + /* even though we are not operating on bs, we need it to + * determine if block ops are currently prohibited on the chain */ + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) { + return; + } + + /* final sanity check */ + if (!bdrv_chain_contains(bs, image_bs)) { + error_setg(errp, "'%s' and image file are not in the same chain", + device); + return; + } + + /* if not r/w, reopen to make r/w */ + open_flags = image_bs->open_flags; + ro = bdrv_is_read_only(image_bs); + + if (ro) { + bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + } + + ret = bdrv_change_backing_file(image_bs, backing_file, + image_bs->drv ? image_bs->drv->format_name : ""); + + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not change backing file to '%s'", + backing_file); + /* don't exit here, so we can try to restore open flags if + * appropriate */ + } + + if (ro) { + bdrv_reopen(image_bs, open_flags, &local_err); + if (local_err) { + error_propagate(errp, local_err); /* will preserve prior errp */ + } + } +} + void qmp_blockdev_add(BlockdevOptions *options, Error **errp) { QmpOutputVisitor *ov = qmp_output_visitor_new(); diff --git a/hmp-commands.hx b/hmp-commands.hx index 5759252..34a561c 100644 --- a/hmp-commands.hx +++ 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", + .params = "[device image] [image_node_name] backing_file", + .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 +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"); + const char *backing_file = qdict_get_str(qdict, "backing_file"); + + qmp_change_backing_file(device != NULL, device, + image != NULL, image, + image_node_name != NULL, image_node_name, + backing_file, &error); + + hmp_handle_error(mon, &error); +} + void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict) { Error *error = NULL; diff --git a/hmp.h b/hmp.h index aba59e9..79b34f4 100644 --- a/hmp.h +++ b/hmp.h @@ -70,6 +70,7 @@ void hmp_eject(Monitor *mon, const QDict *qdict); void hmp_change(Monitor *mon, const QDict *qdict); void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict); void hmp_block_stream(Monitor *mon, const QDict *qdict); +void hmp_change_backing_file(Monitor *mon, const QDict *qdict); void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict); void hmp_block_job_cancel(Monitor *mon, const QDict *qdict); void hmp_block_job_pause(Monitor *mon, const QDict *qdict); diff --git a/qapi-schema.json b/qapi-schema.json index 10be371..ac3fa0b 100644 --- a/qapi-schema.json +++ 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. +# +# 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 +# +# 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', + 'data': { '*device': 'str', '*image': 'str', '*image-node-name': 'str', + 'backing-file': 'str' } } + +## # @block-commit # # Live commit of data from overlay image nodes into backing nodes - i.e., diff --git a/qmp-commands.hx b/qmp-commands.hx index 837195d..4538c12 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1382,6 +1382,76 @@ Example: EQMP { + .name = "change-backing-file", + .args_type = "device:s?,image:s?,image-node-name:s?,backing-file:s", + .mhandler.cmd_new = qmp_marshal_input_change_backing_file, + }, + +SQMP +change-backing-file +------------------- +Since: 2.1 + +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. + +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" + + Method 2: Supply the node-name of the image to modify, via "image-node-name" + +Arguments: + +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": 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" + (json-string, optional) + +- "image": The file name of the image to modify + (json-string, optional) + + +Method 2 interface +-------------------- +- "image-node-name": The name of the block driver state node of the + image to modify. + (json-string, optional) + + +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. + (json-string) + + 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 + + +EQMP + + { .name = "balloon", .args_type = "value:M", .mhandler.cmd_new = qmp_marshal_input_balloon,
This allows a user to change the backing file live, of 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. Signed-off-by: Jeff Cody <jcody@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(+)