From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35937) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wl0IJ-0007zA-Qu for qemu-devel@nongnu.org; Thu, 15 May 2014 14:22:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wl0ID-0006VE-6W for qemu-devel@nongnu.org; Thu, 15 May 2014 14:22:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17454) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wl0IC-0006VA-VJ for qemu-devel@nongnu.org; Thu, 15 May 2014 14:22:09 -0400 Date: Thu, 15 May 2014 14:22:03 -0400 From: Jeff Cody Message-ID: <20140515182203.GL8452@localhost.localdomain> References: <50765faa0041d43d22dd14a0128d3b55850571ac.1400123059.git.jcody@redhat.com> <5374E622.4000805@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5374E622.4000805@redhat.com> Subject: Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, benoit.canet@irqsave.net, pkrempa@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@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 > > --- > > 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