Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file

[Jeff Cody <jcody@redhat.com>]

On Thu, May 15, 2014 at 10:06:58AM -0600, Eric Blake wrote:
> On 05/14/2014 09:20 PM, 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 commit.
> 
> Do we want to allow qemu to error out in situations where it might get
> things wrong (such as if a gluster protocol is backed by a local
> filename) - and mandate that the user supply a backing string in those
> situations?  But this patch is a strict improvement even if you don't go
> that far, because it gives libvirt the flexibility to shift the burden
> of name generation up the stack rather than sticking qemu with that task.
> 

Yes, I think it would be a good idea to change the language here to
allow us to do that as needed.  I like what you wrote at the end of
your comments.


> Hmm - how will this be discoverable by libvirt? Maybe when libvirt is
> doing the 'qemu -m none' probing, it can hotplug a device pointing to
> /dev/null (libvirt _already_ does that to test if add-fd works), and
> intentionally omit a node name.  If libvirt then queries the device, and
> sees that the __qemu##000NNNN node-name was auto-assigned, then it can
> be assumed that this qemu is new enough to provide node-names for ALL
> operations (but that means this series is incomplete unless we add
> node-name support to all remaining block commands, such as block-stream,
> drive-mirror, and drive-backup).  This part is where I wonder if patch
> 1/5 should be rebased to be last in the series.
> 

Ah... I had originally planned on submitting separate patches for each
of the block jobs, to make reviewing easier.  But your idea on how
libvirt can discover this is a good one, and would mandate changing
those commands all in one series to be effective.  So this series will
grow by a few patches.  :)

If libvirt is going to use the autogenerated string format for
decisions, we should also document the string format in the QAPI docs.

> > 
> > For instance, certain relative pathnames may fail, or drives may
> > have been specified originally by file descriptor (e.g. /dev/fd/???),
> 
> I'd document this as /dev/fdset/???, which is the magic string QMP uses
> with its add-fd command (/dev/fd/??? is platform-specific whether it
> will work, /dev/fdset/??? is guaranteed to work in all builds of qemu).
> 

Sure, best to be consistent.

> > 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.
> 
> In short, this new command option allows the equivalent of 'qemu-img
> rebase -u' on a live image.  Definitely a needed functionality.
> 

Would it be useful to have a stand-alone QMP command to change the
backing-file, as well?  As this stands, it will only change the
backing file if you are also merging data down the chain. 

If you want/need the ability to do a true 'qemu-img rebase -u' on any
given image without other chain modification, that needs a new
command.

> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c                   |  8 ++++++--
> >  block/commit.c            |  9 ++++++---
> >  blockdev.c                |  8 +++++++-
> >  include/block/block.h     |  3 ++-
> >  include/block/block_int.h |  3 ++-
> >  qapi-schema.json          | 18 ++++++++++++++++--
> >  qmp-commands.hx           | 14 +++++++++++++-
> >  7 files changed, 52 insertions(+), 11 deletions(-)
> > 
> 
> >      } else {
> >          commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
> > -                    &local_err);
> > +                     backing_file, &local_err);
> 
> See the rest of the thread about using 'has_backing_file ? backing_file
> : NULL' here,
> 
> 
> > +# @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
> > +#                          backing file string is only written into the the
> > +#                          image file metadata - internal structures inside
> > +#                          QEMU are not updated,  and the string is not validated.
> > +#                          If not specified, QEMU will automatically determine
> > +#                          the backing file string to use.  Care should be taken
> 
> Maybe "If not specified, QEMU will automatically determine a backing
> file string to use, or error out if there is no obvious choice", to
> allow us flexibility in erroring out on corner cases such as mixing
> gluster with local files.
> 

+1