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