Re: [Qemu-devel] [PATCH v2 10/11] block: add backing-file option to block-stream

[Jeff Cody]

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 <address@hidden>
> > ---
> >  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.