Allow custom backing-file string in commit/stream
v7 cover Allow custom backing-file string in commit/stream
This is part 2 of a split of "Modify block jobs to use node-names".
These patches add a helper function, allow block-commit and block-stream to change backing files, and adds a standalone API to change backing files.
The api to change backing files relies still on the "check active layer blocker" workaround on blockers.
Jeff Cody (4):
block: add helper function to determine if a BDS is in a chain
block: extend block-commit to accept a string for the backing file
block: add backing-file option to block-stream
block: add QAPI command to allow live backing file change
block.c | 19 ++++++- block/commit.c | 9 ++-- block/stream.c | 11 ++-- blockdev.c | 133 ++++++++++++++++++++++++++++++++++++++++++++-- hmp.c | 2 +- include/block/block.h | 4 +- include/block/block_int.h | 3 +- qapi/block-core.json | 99 ++++++++++++++++++++++++++++++++-- qmp-commands.hx | 95 ++++++++++++++++++++++++++++++++- 9 files changed, 350 insertions(+), 25 deletions(-)
Signature · 1 line
1.9.3
PATCH v7 1/4 block: add helper function to determine if a BDS is in a chain
This is a small helper function, to determine if 'base' is in the chain of BlockDriverState 'top'. It returns true if it is in the chain, and false otherwise.
If either argument is NULL, it will also return false.
--- block.c | 11 +++++++++++ include/block/block.h | 1 + 2 files changed, 12 insertions(+)
block.c +11 -0
@@ -3781,6 +3781,17 @@ BlockDriverState *bdrv_lookup_bs(const char *device,return NULL ; } +/* If 'base' is in the same chain as 'top', return true. Otherwise, +* return false. If either argument is NULL, return false. */ +bool bdrv_chain_contains (BlockDriverState *top,BlockDriverState *base) +{ +while (top && top != base) { + top = top->backing_hd; + } + +return top !=NULL ; +} +BlockDriverState *bdrv_next (BlockDriverState *bs) {if (!bs) {
include/block/block.h +1 -0
@@ -399,6 +399,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);BlockDriverState *bdrv_lookup_bs (const char *device,const char *node_name,Error **errp); +bool bdrv_chain_contains (BlockDriverState *top,BlockDriverState *base);BlockDriverState *bdrv_next (BlockDriverState *bs);void bdrv_iterate (void (*it)(void *opaque,BlockDriverState *bs),void *opaque);
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH v7 for 2.1 1/4] block: add helper function to determine if a BDS is in a chain
Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:
Quoted 9 lines
This is a small helper function, to determine if 'base' is in the chain of BlockDriverState 'top'. It returns true if it is in the chain, and false otherwise.
If either argument is NULL, it will also return false.
Reviewed-by Benoit Canet <[email protected]>Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>
PATCH v7 2/4 block: extend block-commit to accept a string for the backing file
In reply to Jeff CodyOn some image chains, QEMU may not always be able to resolve the filenames properly, when updating the backing file of an image after a block commit.
For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g. /dev/fd/???), or a relative protocol pathname may have been used.
In these instances, QEMU may lack the information to be able to make the correct choice, but the user or management layer most likely does have that knowledge.
With this extension to the block-commit api, the user is able to change the backing file of the overlay image as part of the block-commit operation.
This allows the change to be 'safe', in the sense that if the attempt to write the overlay image metadata fails, then the block-commit operation returns failure, without disrupting the guest.
If the commit top is the active layer, then specifying the backing file string will be treated as an error (there is no overlay image to modify in that case).
If a backing file string is not specified in the command, the backing file string to use is determined in the same manner as it was previously.
--- block.c | 8 ++++++-- block/commit.c | 9 ++++++--- blockdev.c | 8 +++++++- include/block/block.h | 3 ++- include/block/block_int.h | 3 ++- qapi/block-core.json | 20 ++++++++++++++++++-- qmp-commands.hx | 19 ++++++++++++++++++- 7 files changed, 59 insertions(+), 11 deletions(-)
block.c +6 -2
@@ -2560,12 +2560,15 @@ typedef struct BlkIntermediateStates {* * base <- active * +* If backing_file_str is non-NULL, it will be used when modifying top's +* overlay image metadata. +* * Error conditions: * if active == top, that is considered an error * */ int bdrv_drop_intermediate (BlockDriverState *active,BlockDriverState *top, -BlockDriverState *base) +BlockDriverState *base,const char *backing_file_str) {BlockDriverState *intermediate;BlockDriverState *base_bs =NULL ;
@@ -2617,7 +2620,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, }/* success - we can delete the intermediate states, and link top->base */ - ret =bdrv_change_backing_file (new_top_bs, base_bs->filename, + backing_file_str = backing_file_str ? backing_file_str : base_bs->filename; + ret =bdrv_change_backing_file (new_top_bs, backing_file_str, base_bs->drv ? base_bs->drv->format_name :"" );if (ret) { goto exit;
block/commit.c +6 -3
@@ -37,6 +37,7 @@ typedef struct CommitBlockJob {BlockdevOnError on_error;int base_flags;int orig_overlay_flags; +char *backing_file_str; } CommitBlockJob; static int coroutine_fn commit_populate(BlockDriverState *bs,
@@ -141,7 +142,7 @@ wait:if (!block_job_is_cancelled (&s->common) && sector_num == end) {/* success */ - ret =bdrv_drop_intermediate (active, top, base); + ret =bdrv_drop_intermediate (active, top, base, s->backing_file_str); } exit_free_buf:
@@ -158,7 +159,7 @@ exit_restore_reopen:if (overlay_bs && s->orig_overlay_flags !=bdrv_get_flags (overlay_bs)) {bdrv_reopen (overlay_bs, s->orig_overlay_flags,NULL ); } - +g_free (s->backing_file_str);block_job_completed (&s->common, ret); }
@@ -182,7 +183,7 @@ static const BlockJobDriver commit_job_driver = {void commit_start (BlockDriverState *bs,BlockDriverState *base,BlockDriverState *top,int64_t speed,BlockdevOnError on_error,BlockDriverCompletionFunc *cb, -void *opaque,Error **errp) +void *opaque,const char *backing_file_str,Error **errp) {CommitBlockJob *s;BlockReopenQueue *reopen_queue =NULL ;
@@ -244,6 +245,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base, s->base_flags = orig_base_flags; s->orig_overlay_flags = orig_overlay_flags; + s->backing_file_str =g_strdup (backing_file_str); + s->on_error = on_error; s->common.co =qemu_coroutine_create (commit_run);
blockdev.c +7 -1
@@ -1910,6 +1910,7 @@ void qmp_block_stream(const char *device, bool has_base,void qmp_block_commit (const char *device,bool has_base,const char *base,bool has_top,const char *top, +bool has_backing_file,const char *backing_file,bool has_speed,int64_t speed,Error **errp) {
@@ -1975,11 +1976,16 @@ void qmp_block_commit(const char *device, }if (top_bs == bs) { +if (has_backing_file) { +error_setg (errp,"'backing-file' specified," +" but 'top' is the active layer" ); +return ; + }commit_active_start (bs, base_bs, speed, on_error, block_job_cb, bs, &local_err); }else {commit_start (bs, base_bs, top_bs, speed, on_error, block_job_cb, bs, - &local_err); + has_backing_file ? backing_file :NULL , &local_err); }if (local_err !=NULL ) {error_propagate (errp, local_err);
include/block/block.h +2 -1
@@ -284,7 +284,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, const char *backing_fmt);void bdrv_register (BlockDriver *bdrv);int bdrv_drop_intermediate (BlockDriverState *active,BlockDriverState *top, -BlockDriverState *base); +BlockDriverState *base, +const char *backing_file_str);BlockDriverState *bdrv_find_overlay (BlockDriverState *active,BlockDriverState *bs);BlockDriverState *bdrv_find_base (BlockDriverState *bs);
include/block/block_int.h +2 -1
@@ -459,13 +459,14 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,* @on_error: The action to take upon error. * @cb: Completion function for the job. * @opaque: Opaque pointer value passed to @cb. +* @backing_file_str: String to use as the backing file in @top's overlay * @errp: Error object. * */ void commit_start (BlockDriverState *bs,BlockDriverState *base,BlockDriverState *top,int64_t speed,BlockdevOnError on_error,BlockDriverCompletionFunc *cb, -void *opaque,Error **errp); +void *opaque,const char *backing_file_str,Error **errp);/** * commit_active_start: * @bs: Active block device to be committed.
qapi/block-core.json +18 -2
@@ -694,6 +694,23 @@ # which contains the topmost data to be committed down. If # not specified, this is the active layer. # +# @backing-file: #optional The backing file string to write into the overlay +# image of 'top'. If 'top' is the active layer, +# specifying a backing file string is an error. This +# filename is not validated. +# +# If a pathname string is such that it cannot be +# resolved by QEMU, that means that subsequent QMP or +# HMP commands must use node-names for the image in +# question, as filename lookup methods will fail. +# +# If not specified, QEMU will automatically determine +# the backing file string to use, or error out if +# there is no obvious choice. Care should be taken +# when specifying the string, to specify a valid +# filename or protocol. +# (Since 2.1) +# # If top == base, that is an error. # If top == active, the job will not be completed by itself, # user needs to complete the job with the block-job-complete
@@ -706,7 +723,6 @@ # size of the smaller top, you can safely truncate it # yourself once the commit operation successfully completes. # -# # @speed: #optional the maximum speed, in bytes per second # # Returns: Nothing on success
@@ -721,7 +737,7 @@ ## { 'command': 'block-commit', 'data': { 'device': 'str', '*base': 'str', '*top': 'str', - '*speed': 'int' } } + '*backing-file': 'str', '*speed': 'int' } } ## # @drive-backup
qmp-commands.hx +18 -1
@@ -985,7 +985,7 @@ EQMP { .name = "block-commit", - .args_type = "device:B,base:s?,top:s?,speed:o?", + .args_type = "device:B,base:s?,top:s?,backing-file:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_input_block_commit, },
@@ -1006,6 +1006,23 @@ Arguments: which contains the topmost data to be committed down. If not specified, this is the active layer. (json-string, optional) +- backing-file: The backing file string to write into the overlay + image of 'top'. If 'top' is the active layer, + specifying a backing file string is an error. This + filename is not validated. + + If a pathname string is such that it cannot be + resolved by QEMU, that means that subsequent QMP or + HMP commands must use node-names for the image in + question, as filename lookup methods will fail. + + If not specified, QEMU will automatically determine + the backing file string to use, or error out if + there is no obvious choice. Care should be taken + when specifying the string, to specify a valid + filename or protocol. + (json-string, optional) (Since 2.1) + If top == base, that is an error. If top == active, the job will not be completed by itself, user needs to complete the job with the block-job-complete
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH v7 for 2.1 2/4] block: extend block-commit to accept a string for the backing file
Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:
Quoted 24 lines
On some image chains, QEMU may not always be able to resolve the filenames properly, when updating the backing file of an image after a block commit.
For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g.
/dev/fd/???), or a relative protocol pathname may have been used.In these instances, QEMU may lack the information to be able to make the correct choice, but the user or management layer most likely does have that knowledge.
With this extension to the block-commit api, the user is able to change the backing file of the overlay image as part of the block-commit operation.
This allows the change to be 'safe', in the sense that if the attempt to write the overlay image metadata fails, then the block-commit operation returns failure, without disrupting the guest.
If the commit top is the active layer, then specifying the backing file string will be treated as an error (there is no overlay image to modify in that case).
Quoted 6 lines
If a backing file string is not specified in the command, the backing file string to use is determined in the same manner as it was previously.
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>
PATCH v7 3/4 block: add backing-file option to block-stream
In reply to Jeff CodyOn some image chains, QEMU may not always be able to resolve the filenames properly, when updating the backing file of an image after a block job.
For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g. /dev/fd/???), or a relative protocol pathname may have been used.
In these instances, QEMU may lack the information to be able to make the correct choice, but the user or management layer most likely does have that knowledge.
With this extension to the block-stream api, the user is able to change the backing file of the active layer as part of the block-stream operation.
This allows the change to be 'safe', in the sense that if the attempt to write the active image metadata fails, then the block-stream operation returns failure, without disrupting the guest.
If a backing file string is not specified in the command, the backing file string to use is determined in the same manner as it was previously.
--- block/stream.c | 11 +++++------ blockdev.c | 23 +++++++++++++++++++---- hmp.c | 2 +- qapi/block-core.json | 19 +++++++++++++++++-- qmp-commands.hx | 2 +- 5 files changed, 43 insertions(+), 14 deletions(-)
block/stream.c +5 -6
@@ -32,7 +32,7 @@ typedef struct StreamBlockJob {RateLimit limit;BlockDriverState *base;BlockdevOnError on_error; -char backing_file_id[1024 ]; +char *backing_file_str; } StreamBlockJob; static int coroutine_fn stream_populate(BlockDriverState *bs,
@@ -186,7 +186,7 @@ wait:if (!block_job_is_cancelled (&s->common) && sector_num == end && ret ==0 ) {const char *base_id =NULL , *base_fmt =NULL ;if (base) { - base_id = s->backing_file_id; + base_id = s->backing_file_str;if (base->drv) { base_fmt = base->drv->format_name; }
@@ -196,6 +196,7 @@ wait: }qemu_vfree (buf); +g_free (s->backing_file_str);block_job_completed (&s->common, ret); }
@@ -217,7 +218,7 @@ static const BlockJobDriver stream_job_driver = { };void stream_start (BlockDriverState *bs,BlockDriverState *base, -const char *base_id,int64_t speed, +const char *backing_file_str,int64_t speed,BlockdevOnError on_error,BlockDriverCompletionFunc *cb,void *opaque,Error **errp)
@@ -237,9 +238,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, } s->base = base; -if (base_id) { -pstrcpy (s->backing_file_id,sizeof (s->backing_file_id), base_id); - } + s->backing_file_str =g_strdup (backing_file_str); s->on_error = on_error; s->common.co =qemu_coroutine_create (stream_run);
blockdev.c +19 -4
@@ -1866,14 +1866,17 @@ static void block_job_cb(void *opaque, int ret)bdrv_put_ref_bh_schedule (bs); } -void qmp_block_stream (const char *device,bool has_base, -const char *base,bool has_speed,int64_t speed, +void qmp_block_stream (const char *device, +bool has_base,const char *base, +bool has_backing_file,const char *backing_file, +bool has_speed,int64_t speed,bool has_on_error,BlockdevOnError on_error,Error **errp) {BlockDriverState *bs;BlockDriverState *base_bs =NULL ;Error *local_err =NULL ; +const char *base_name =NULL ;if (!has_on_error) { on_error =BLOCKDEV_ON_ERROR_REPORT ;
@@ -1889,15 +1892,27 @@ void qmp_block_stream(const char *device, bool has_base,return ; } -if (base) { +if (has_base) { base_bs =bdrv_find_backing_image (bs, base);if (base_bs ==NULL ) {error_set (errp,QERR_BASE_NOT_FOUND , base);return ; } + base_name = base; } -stream_start (bs, base_bs, base, has_speed ? speed :0 , +/* if we are streaming the entire chain, the result will have no backing +* file, and specifying one is therefore an error */ +if (base_bs ==NULL && has_backing_file) { +error_setg (errp,"backing file specified, but streaming the " +"entire chain" ); +return ; + } + +/* backing_file string overrides base bs filename */ + base_name = has_backing_file ? backing_file : base_name; + +stream_start (bs, base_bs, base_name, has_speed ? speed :0 , on_error, block_job_cb, bs, &local_err);if (local_err) {error_propagate (errp, local_err);
hmp.c +1 -1
@@ -1175,7 +1175,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)const char *base =qdict_get_try_str (qdict,"base" );int64_t speed =qdict_get_try_int (qdict,"speed" ,0 ); -qmp_block_stream (device, base !=NULL , base, +qmp_block_stream (device, base !=NULL , base, false,NULL ,qdict_haskey (qdict,"speed" ), speed, true,BLOCKDEV_ON_ERROR_REPORT , &error);
qapi/block-core.json +17 -2
@@ -888,6 +888,21 @@ # # @base: #optional the common backing file name # +# @backing-file: #optional The backing file string to write into the active +# layer. This filename is not validated. +# +# If a pathname string is such that it cannot be +# resolved by QEMU, that means that subsequent QMP or +# HMP commands must use node-names for the image in +# question, as filename lookup methods will fail. +# +# If not specified, QEMU will automatically determine +# the backing file string to use, or error out if there +# is no obvious choice. Care should be taken when +# specifying the string, to specify a valid filename or +# protocol. +# (Since 2.1) +# # @speed: #optional the maximum speed, in bytes per second # # @on-error: #optional the action to take on an error (default report).
@@ -900,8 +915,8 @@ # Since: 1.1 ## { 'command': 'block-stream', - 'data': { 'device': 'str', '*base': 'str', '*speed': 'int', - '*on-error': 'BlockdevOnError' } } + 'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str', + '*speed': 'int', '*on-error': 'BlockdevOnError' } } ## # @block-job-set-speed:
qmp-commands.hx +1 -1
@@ -979,7 +979,7 @@ EQMP { .name = "block-stream", - .args_type = "device:B,base:s?,speed:o?,on-error:s?", + .args_type = "device:B,base:s?,speed:o?,backing-file:s?,on-error:s?", .mhandler.cmd_new = qmp_marshal_input_block_stream, },
Signature · 1 line
1.9.3
PATCH v7 4/4 block: add QAPI command to allow live backing file change
In reply to Jeff CodyThis 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) 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 changing the backing file string.
--- blockdev.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 60 ++++++++++++++++++++++++++++++ qmp-commands.hx | 74 +++++++++++++++++++++++++++++++++++++ 3 files changed, 236 insertions(+)
blockdev.c +102 -0
@@ -2347,6 +2347,108 @@ void qmp_block_job_complete(const char *device, Error **errp)block_job_complete (job, errp); } +void qmp_change_backing_file (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 ; + } + +/* find the top layer BDS of the chain */ + 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 (has_image) { +if (!strcmp (bs->filename, image)) { + image_bs = bs; + }else { + image_bs =bdrv_find_backing_image (bs, image); + } + } + +if (!has_image && !has_image_node_name) { + image_bs = bs; + } + +if (!image_bs) { +error_setg (errp,"image file not found" ); +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 necessarily 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 ();
qapi/block-core.json +60 -0
66 lines not on this page · raw message
qmp-commands.hx +74 -0
80 lines not on this page · raw message
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH v7 for 2.1 4/4] block: add QAPI command to allow live backing file change
Quoted 12 lines · Jeff Cody
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) image filename
2) image node-nameNote: 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.
Quoted 6 lines
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 changing the backing file string.
For 2.1, did we want to limit this command to only affecting the active layer (and failing if it was attempted on a backing file), in order to simplify dealing with op-blocker work? Having the command exist, even if it is limited in nature, is all the more libvirt wants right now; and it's easier to relax a constraint in a future release than it is to release something now and have to keep it working even after we rework op-blockers in the future.
Quoted 3 lines
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>
I'm still okay with this patch as-is, if we are okay with the ability to rename the backing file of a non-active node.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v7 for 2.1 4/4] block: add QAPI command to allow live backing file change
Quoted 20 lines · Eric Blake
Quoted 12 lines · Jeff Cody
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) image filename
2) image node-nameNote: 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.
Quoted 6 lines
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 changing the backing file string.
Quoted 8 lines
For 2.1, did we want to limit this command to only affecting the active layer (and failing if it was attempted on a backing file), in order to simplify dealing with op-blocker work? Having the command exist, even if it is limited in nature, is all the more libvirt wants right now; and it's easier to relax a constraint in a future release than it is to release something now and have to keep it working even after we rework op-blockers in the future.
I left this as-is, because the blocker issue needs to be addressed somehow for 2.1. And once we know in what manner, I'll just update this series to use that methodology (if necessary). Unless Stefan or Kevin would prefer that I change this to only operate on the active layer, of course.
Quoted 11 lines
Quoted 3 lines
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>I'm still okay with this patch as-is, if we are okay with the ability to rename the backing file of a non-active node.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v7 for 2.1 4/4] block: add QAPI command to allow live backing file change
In reply to Jeff CodyAm 25.06.2014 um 21:40 hat Jeff Cody geschrieben:
Quoted 16 lines
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) image filename
2) image node-nameNote: 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.
Quoted 5 lines
A good analogy for this command is that it is a live version of 'qemu-img rebase -u', with respect to changing the backing file string.
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>
I'm not a huge fan of adding two different addressing modes to a new QMP command. I consider using device_name/filename as deprecated and expect that management tools use node-name for new commands.
Also there's still Eric's reply and Jeff's promise to update the patch once blockers were sorted out for 2.1.
I'm leaning towards declaring this patch not ready.
Kevin
Re: [Qemu-devel] [PATCH v7 for 2.1 4/4] block: add QAPI command to allow live backing file change
Quoted 6 lines · Kevin Wolf
Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:
Quoted 5 lines
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:
Quoted 7 lines
Quoted 3 lines
1) image filename
2) image node-nameI'm not a huge fan of adding two different addressing modes to a new QMP command. I consider using device_name/filename as deprecated and expect that management tools use node-name for new commands.
Good point. Libvirt does not plan on using this command directly (that is, right now, we don't see a need to change the backing file of an image except as part of a bigger operation; but the work with block-commit and block-stream earlier in this series already adds that in the bigger operation); but we DO want to have a witness that the other commands have support for relative backing name selection. So it is easiest if this new command exists, in ANY form, as the witness that the overall series is in place, without regards to the parameters in this particular command.
Quoted 5 lines
Also there's still Eric's reply and Jeff's promise to update the patch once blockers were sorted out for 2.1.
I'm leaning towards declaring this patch not ready.
How about a compromise - simplify the command to always require a node-name and device for 2.1. For 2.2, we may later add optional arguments (for example, making @device optional instead of mandatory), and/or provide alternative interfaces (if it turns out we wanted filename addressing after all), but the immediate concern is getting the command installed, with the bare minimum interface. That is, my proposal is to completely drop the addressing by filename, and only allow addressing by node-name.
##
# @change-backing-file
#
# Change the backing file in the image file metadata. This does not
# cause QEMU to reopen the image file to reparse the backing filename
# (it may, however, perform a reopen to change permissions from
# r/o -> r/w -> r/o, if needed). The new backing file string is written
# into the image file metadata, and the QEMU internal strings are
# updated.
#
# @image-node-name: The name of the block driver state node of the
# image to modify.
#
# @device: The name of the device that owns image-node-name.
#
# @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.
#
# Since: 2.1
##
{ 'command': 'change-backing-file',
'data': { 'device': 'str', 'image-node-name': 'str',
'backing-file': 'str' } }Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH v8 4/4 block: add QAPI command to allow live backing file change
In reply to Jeff CodyFrom: Jeff Cody <[email protected]>
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) 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 changing the backing file string.
[Jeff is offline so I respun this patch in his absence. Dropped image filename since using node-name is preferred and this is a new command. No need to introduce the limitations of finding images by filename. --Stefan]
--- blockdev.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 26 +++++++++++++++++ qmp-commands.hx | 39 ++++++++++++++++++++++++++ 3 files changed, 144 insertions(+)
blockdev.c +79 -0
@@ -2388,6 +2388,85 @@ void qmp_block_job_complete(const char *device, Error **errp)block_job_complete (job, errp); } +void qmp_change_backing_file (const char *device, +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; + +/* find the top layer BDS of the chain */ + bs =bdrv_find (device); +if (!bs) { +error_set (errp,QERR_DEVICE_NOT_FOUND , device); +return ; + } + + image_bs =bdrv_lookup_bs (NULL , image_node_name, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } + +if (!image_bs) { +error_setg (errp,"image file not found" ); +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 necessarily 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 ();
qapi/block-core.json +26 -0
@@ -680,6 +680,32 @@ 'data': 'BlockdevSnapshot' } ## +# @change-backing-file +# +# Change the backing file in the image file metadata. This does not +# cause QEMU to reopen the image file to reparse the backing filename +# (it may, however, perform a reopen to change permissions from +# r/o -> r/w -> r/o, if needed). The new backing file string is written +# into the image file metadata, and the QEMU internal strings are +# updated. +# +# @image-node-name: The name of the block driver state node of the +# image to modify. +# +# @device: The name of the device that owns image-node-name. +# +# @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. +# +# Since: 2.1 +## +{ 'command': 'change-backing-file', + 'data': { 'device': 'str', 'image-node-name': 'str', + 'backing-file': 'str' } } + +## # @block-commit # # Live commit of data from overlay image nodes into backing nodes - i.e.,
qmp-commands.hx +39 -0
@@ -1370,6 +1370,45 @@ Example: EQMP { + .name = "change-backing-file", + .args_type = "device: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 cause +QEMU to reopen the image file to reparse the backing filename (it may, +however, perform a reopen to change permissions from r/o -> r/w -> r/o, +if needed). The new backing file string is written into the image file +metadata, and the QEMU internal strings are updated. + +Arguments: + +- "image-node-name": The name of the block driver state node of the + image to modify. The "device" is argument is used to + verify "image-node-name" is in the chain described by + "device". + (json-string, optional) + +- "device": The name of the device. + (json-string) + +- "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) + +Returns: Nothing on success + If "device" does not exist or cannot be determined, DeviceNotFound + +EQMP + + { .name = "balloon", .args_type = "value:M", .mhandler.cmd_new = qmp_marshal_input_balloon,
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH v8 for 2.1 4/4] block: add QAPI command to allow live backing file change
Am 01.07.2014 um 09:52 hat Stefan Hajnoczi geschrieben:
Quoted 21 lines
From: Jeff Cody <[email protected]>
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) image filename
2) image node-nameNote: 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 changing the backing file string.
Quoted 8 lines
[Jeff is offline so I respun this patch in his absence. Dropped image filename since using node-name is preferred and this is a new command. No need to introduce the limitations of finding images by filename. --Stefan]
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>Signed-off-by Stefan Hajnoczi <[email protected]>
Re: [Qemu-devel] [PATCH v8 for 2.1 4/4] block: add QAPI command to allow live backing file change
In reply to Stefan HajnocziQuoted 10 lines · Stefan Hajnoczi
From: Jeff Cody <[email protected]>
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) image filename
2) image node-name
This part of the commit message is now stale, but that's okay...
Quoted 8 lines
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.
Quoted 7 lines
A good analogy for this command is that it is a live version of 'qemu-img rebase -u', with respect to changing the backing file string.
[Jeff is offline so I respun this patch in his absence. Dropped image filename since using node-name is preferred and this is a new command. No need to introduce the limitations of finding images by filename. --Stefan]
...given that you are preserving history while documenting changes.
Quoted 4 lines
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>Signed-off-by Stefan Hajnoczi <[email protected]>
This is a big enough change that I re-reviewed the patch. The reduced capability of the command is not an issue for how libvirt plans to use it (mainly as a witness that libvirt has control over backing file strings); and it is still desirable from libvirt's point of view to introduce the command.
Thanks for fixing this up.
Quoted 16 lines
--- blockdev.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 26 +++++++++++++++++ qmp-commands.hx | 39 ++++++++++++++++++++++++++ 3 files changed, 144 insertions(+)qmp-commands.hx +8
-0+SQMP +change-backing-file +------------------- +Since: 2.1 + +Change the backing file in the image file metadata. This does not cause +QEMU to reopen the image file to reparse the backing filename (it may, +however, perform a reopen to change permissions from r/o -> r/w -> r/o,
qmp-commands.hx Example: · line 1395
Quoted 10 lines
qmp-commands.hx +10
-0+if needed). The new backing file string is written into the image file +metadata, and the QEMU internal strings are updated. + +Arguments: + +- "image-node-name": The name of the block driver state node of the + image to modify. The "device" is argument is used to + verify "image-node-name" is in the chain described by + "device". + (json-string, optional)
No longer optional. If you still have time to fix it, great; if not, it's minor enough. My R-b still stands.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org