| Message ID | 0a1752c77145e37744c19f88e89643c3ca99ca00.1401200583.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/27/2014 08:28 AM, Jeff Cody wrote: > 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 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. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/stream.c | 13 ++++++++----- > blockdev.c | 12 +++++++++++- > hmp-commands.hx | 2 +- > hmp.c | 2 ++ > include/block/block_int.h | 2 +- > qapi-schema.json | 18 +++++++++++++++++- > qmp-commands.hx | 2 +- > 7 files changed, 41 insertions(+), 10 deletions(-) > > @@ -220,7 +221,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, > const char *base_id, int64_t speed, > BlockdevOnError on_error, > BlockDriverCompletionFunc *cb, > - void *opaque, Error **errp) > + void *opaque, const char *backing_file_str, Error **errp) > { Umm, aren't the 'base_id' and 'backing_file_str' arguments redundant now? You only need one, not both, because there is only one backing string to be written (or cleared) into the active file. [1] > @@ -237,8 +238,10 @@ 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); > + if (backing_file_str) { > + s->backing_file_str = g_strdup(backing_file_str); > + } else { > + s->backing_file_str = g_strdup(base_id); g_strdup(NULL) is safely NULL, correct? (You hit this case when base_id is NULL, which happens when base is NULL). > @@ -1941,8 +1942,17 @@ void qmp_block_stream(bool has_device, const char *device, > return; > } > > + /* if we are streaming from the bottommost base image, then specifying > + * a backing file does not make sense, and is an error */ Misleading (back to the complaint in 9/11) - omitting base is different from using the bottommost base image as base. I'd word that more like: If we are streaming the entire chain, then the result will have no backing file and specifying a backing name is an error > + if (base_bs == NULL && has_backing_file) { > + error_setg(errp, "backing file specified, but streaming from " > + "bottommost base image"); > + return; > + } The 'if' condition is correct and necessary, but the error message could use better wording; maybe: backing file specified but no base image supplied > stream_start(bs, base_bs, base_name, has_speed ? speed : 0, > - on_error, block_job_cb, bs, &local_err); > + on_error, block_job_cb, bs, > + has_backing_file ? backing_file : NULL, &local_err); [1] Again, how do 'base_name' and 'backing_file' differ at this point in the game? Isn't it just simpler to do: has_backing_file ? backing_file : base_name and use a single parameter? > +++ b/qapi-schema.json > @@ -2611,6 +2611,21 @@ > # @base-node-name: #optional the block driver state node name of the > # common backing file. (Since 2.1) > # > +# @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 be QEMU, that means that subsequent QMP or Copy-and-paste strikes again :) s/be/by/ > +++ b/qmp-commands.hx > @@ -979,7 +979,7 @@ EQMP > > { > .name = "block-stream", > - .args_type = "device:B?,base:s?,base-node-name:s?,speed:o?,on-error:s?", > + .args_type = "device:B?,base:s?,base-node-name:s?,speed:o?,backing-file:s?,on-error:s?", > .mhandler.cmd_new = qmp_marshal_input_block_stream, > }, Missing documentation of backing-file? Oh, the entire block-stream call is missing documentation, when compared to the block-commit call. Oh well, I can't fault you for that, although it might be nice to rectify while we are improving it.
On Tue, May 27, 2014 at 02:30:02PM -0600, Eric Blake wrote: > On 05/27/2014 08:28 AM, Jeff Cody wrote: > > 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 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. > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > block/stream.c | 13 ++++++++----- > > blockdev.c | 12 +++++++++++- > > hmp-commands.hx | 2 +- > > hmp.c | 2 ++ > > include/block/block_int.h | 2 +- > > qapi-schema.json | 18 +++++++++++++++++- > > qmp-commands.hx | 2 +- > > 7 files changed, 41 insertions(+), 10 deletions(-) > > > > > @@ -220,7 +221,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, > > const char *base_id, int64_t speed, > > BlockdevOnError on_error, > > BlockDriverCompletionFunc *cb, > > - void *opaque, Error **errp) > > + void *opaque, const char *backing_file_str, Error **errp) > > { > > Umm, aren't the 'base_id' and 'backing_file_str' arguments redundant > now? You only need one, not both, because there is only one backing > string to be written (or cleared) into the active file. [1] > Yes, we can get rid of the extra argument... I had originally done things differently in stream.c (used the optional backing file string later at the end of the block job). When I realized the string wasn't used anywhere else, I moved it into the stream_start().... I should have factored out the extra parameter at that point, too. I'll remove it for v3. > > @@ -237,8 +238,10 @@ 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); > > + if (backing_file_str) { > > + s->backing_file_str = g_strdup(backing_file_str); > > + } else { > > + s->backing_file_str = g_strdup(base_id); > > g_strdup(NULL) is safely NULL, correct? (You hit this case when base_id > is NULL, which happens when base is NULL). > Yes, g_strdup(NULL) returns NULL, so I relied on that. Once we remove the extra argument, this change will become: - if (base_id) { - pstrcpy(s->backing_file_id, sizeof(s->backing_file_id), base_id); - } + s->backing_file_str = g_strdup(base_id); > > @@ -1941,8 +1942,17 @@ void qmp_block_stream(bool has_device, const char *device, > > return; > > } > > > > + /* if we are streaming from the bottommost base image, then specifying > > + * a backing file does not make sense, and is an error */ > > Misleading (back to the complaint in 9/11) - omitting base is different > from using the bottommost base image as base. I'd word that more like: > > If we are streaming the entire chain, then the result will have no > backing file and specifying a backing name is an error > Agree > > + if (base_bs == NULL && has_backing_file) { > > + error_setg(errp, "backing file specified, but streaming from " > > + "bottommost base image"); > > + return; > > + } > > The 'if' condition is correct and necessary, but the error message could > use better wording; maybe: > > backing file specified but no base image supplied > Yes, the error message isn't great - I'll use something like yours. > > stream_start(bs, base_bs, base_name, has_speed ? speed : 0, > > - on_error, block_job_cb, bs, &local_err); > > + on_error, block_job_cb, bs, > > + has_backing_file ? backing_file : NULL, &local_err); > > [1] Again, how do 'base_name' and 'backing_file' differ at this point in > the game? Isn't it just simpler to do: > has_backing_file ? backing_file : base_name > and use a single parameter? > > > +++ b/qapi-schema.json > > @@ -2611,6 +2611,21 @@ > > # @base-node-name: #optional the block driver state node name of the > > # common backing file. (Since 2.1) > > # > > +# @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 be QEMU, that means that subsequent QMP or > > Copy-and-paste strikes again :) > s/be/by/ Thanks. The funny thing is I read that sentence a few times before I noticed it, even after you pointed it out. > > > +++ b/qmp-commands.hx > > @@ -979,7 +979,7 @@ EQMP > > > > { > > .name = "block-stream", > > - .args_type = "device:B?,base:s?,base-node-name:s?,speed:o?,on-error:s?", > > + .args_type = "device:B?,base:s?,base-node-name:s?,speed:o?,backing-file:s?,on-error:s?", > > .mhandler.cmd_new = qmp_marshal_input_block_stream, > > }, > > Missing documentation of backing-file? Oh, the entire block-stream call > is missing documentation, when compared to the block-commit call. Oh > well, I can't fault you for that, although it might be nice to rectify > while we are improving it. > I actually thought about adding it in, but got lazy :). Since I'll be putting out a v3, I'll go ahead and add documentation for block-stream.
On 05/27/2014 08:28 AM, Jeff Cody wrote: > 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 job. > > +++ b/hmp-commands.hx > @@ -76,7 +76,7 @@ ETEXI > > { > .name = "block_stream", > - .args_type = "device:B?,speed:o?,base:s?,base-node-name:s?", > + .args_type = "device:B?,speed:o?,base:s?,base-node-name:s?,backing-file:s?", > .params = "device [speed [base]]", Another HMP change that is probably better with a named optional parameter (backing-file:+f or some such), rather than positional.
diff --git a/block/stream.c b/block/stream.c index 91d18a2..bf19a0e 100644 --- a/block/stream.c +++ b/block/stream.c @@ -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); } @@ -220,7 +221,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, const char *base_id, int64_t speed, BlockdevOnError on_error, BlockDriverCompletionFunc *cb, - void *opaque, Error **errp) + void *opaque, const char *backing_file_str, Error **errp) { StreamBlockJob *s; @@ -237,8 +238,10 @@ 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); + if (backing_file_str) { + s->backing_file_str = g_strdup(backing_file_str); + } else { + s->backing_file_str = g_strdup(base_id); } s->on_error = on_error; diff --git a/blockdev.c b/blockdev.c index a4468b4..81d1383 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1873,6 +1873,7 @@ static void block_job_cb(void *opaque, int ret) void qmp_block_stream(bool has_device, const char *device, bool has_base, const char *base, bool has_base_node_name, const char *base_node_name, + bool has_backing_file, const char *backing_file, bool has_speed, int64_t speed, bool has_on_error, BlockdevOnError on_error, Error **errp) @@ -1941,8 +1942,17 @@ void qmp_block_stream(bool has_device, const char *device, return; } + /* if we are streaming from the bottommost base image, then specifying + * a backing file does not make sense, and is an error */ + if (base_bs == NULL && has_backing_file) { + error_setg(errp, "backing file specified, but streaming from " + "bottommost base image"); + return; + } + stream_start(bs, base_bs, base_name, has_speed ? speed : 0, - on_error, block_job_cb, bs, &local_err); + on_error, block_job_cb, bs, + has_backing_file ? backing_file : NULL, &local_err); if (local_err) { error_propagate(errp, local_err); return; diff --git a/hmp-commands.hx b/hmp-commands.hx index 17eda87..5759252 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -76,7 +76,7 @@ ETEXI { .name = "block_stream", - .args_type = "device:B?,speed:o?,base:s?,base-node-name:s?", + .args_type = "device:B?,speed:o?,base:s?,base-node-name:s?,backing-file:s?", .params = "device [speed [base]]", .help = "copy data from a backing file into a block device", .mhandler.cmd = hmp_block_stream, diff --git a/hmp.c b/hmp.c index 122aa29..69dd4f5 100644 --- a/hmp.c +++ b/hmp.c @@ -1171,10 +1171,12 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict) const char *device = qdict_get_str(qdict, "device"); const char *base = qdict_get_try_str(qdict, "base"); const char *base_node_name = qdict_get_try_str(qdict, "base_node_name"); + const char *backing_file = qdict_get_try_str(qdict, "backing_file"); int64_t speed = qdict_get_try_int(qdict, "speed", 0); qmp_block_stream(device != NULL, device, base != NULL, base, base_node_name != NULL, base_node_name, + backing_file != NULL, backing_file, qdict_haskey(qdict, "speed"), speed, true, BLOCKDEV_ON_ERROR_REPORT, &error); diff --git a/include/block/block_int.h b/include/block/block_int.h index f5809ff..9f44c79 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -426,7 +426,7 @@ void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv, void stream_start(BlockDriverState *bs, BlockDriverState *base, const char *base_id, int64_t speed, BlockdevOnError on_error, BlockDriverCompletionFunc *cb, - void *opaque, Error **errp); + void *opaque, const char *backing_file, Error **errp); /** * commit_start: diff --git a/qapi-schema.json b/qapi-schema.json index 63e74c5..10be371 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2611,6 +2611,21 @@ # @base-node-name: #optional the block driver state node name of the # common backing file. (Since 2.1) # +# @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 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. +# +# 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). @@ -2624,7 +2639,8 @@ ## { 'command': 'block-stream', 'data': { '*device': 'str', '*base': 'str', '*base-node-name': 'str', - '*speed': 'int', '*on-error': 'BlockdevOnError' } } + '*backing-file': 'str', '*speed': 'int', + '*on-error': 'BlockdevOnError' } } ## # @block-job-set-speed: diff --git a/qmp-commands.hx b/qmp-commands.hx index 903c63a..837195d 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -979,7 +979,7 @@ EQMP { .name = "block-stream", - .args_type = "device:B?,base:s?,base-node-name:s?,speed:o?,on-error:s?", + .args_type = "device:B?,base:s?,base-node-name:s?,speed:o?,backing-file:s?,on-error:s?", .mhandler.cmd_new = qmp_marshal_input_block_stream, },
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 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. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/stream.c | 13 ++++++++----- blockdev.c | 12 +++++++++++- hmp-commands.hx | 2 +- hmp.c | 2 ++ include/block/block_int.h | 2 +- qapi-schema.json | 18 +++++++++++++++++- qmp-commands.hx | 2 +- 7 files changed, 41 insertions(+), 10 deletions(-)