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 <jc...@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.