From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45051) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCgpf-0001FL-IG for qemu-devel@nongnu.org; Fri, 14 Sep 2012 21:06:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TCgpd-0000V7-WA for qemu-devel@nongnu.org; Fri, 14 Sep 2012 21:06:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15318) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCgpd-0000Ux-Ne for qemu-devel@nongnu.org; Fri, 14 Sep 2012 21:06:01 -0400 Message-ID: <5053D473.9030803@redhat.com> Date: Fri, 14 Sep 2012 19:05:55 -0600 From: Eric Blake MIME-Version: 1.0 References: In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig0B0B91A50B0BE274A8856101" Subject: Re: [Qemu-devel] [PATCH 6/8] QAPI: add command for live block commit, 'block-commit' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, supriyak@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig0B0B91A50B0BE274A8856101 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 09/14/2012 07:41 AM, Jeff Cody wrote: > The command for live block commit is added, which has the following > arguments: >=20 > device: the block device to perform the commit on (mandatory) > base: the base image to commit into; optional (if not specified, > it is the underlying original image) > top: the top image of the commit - all data from inside top down > to base will be committed into base. optional (if not specified= , > it is the active image) - see note below > speed: maximum speed, in bytes/sec > on_error: action to take on error (optional - default is report) Shouldn't this be on-error, with a dash? Also, this doesn't match the actual commit, since you pulled it while waiting to rebase on Paolo's patches. >=20 > note: eventually this will support merging down the active layer, > but that code is not yet complete. If the active layer is passed= > in currently as top, or top is left to the default, then the erro= r > QERR_TOP_NOT_FOUND will be returned. I don't think we need a new error category for this particular failure (and in 4/8, you labeled the error as generic); so it is sufficient to state that 'an error will be returned'. >=20 > The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED wi= ll > be emitted. >=20 > +++ b/blockdev.c > @@ -825,7 +825,6 @@ exit: > return; > } > =20 > - > static void eject_device(BlockDriverState *bs, int force, Error **errp= ) Spurious whitespace change. > +void qmp_block_commit(const char *device, > + bool has_base, const char *base, > + bool has_top, const char *top, > + bool has_speed, int64_t speed, > + Error **errp) > +{ > + BlockDriverState *bs; > + BlockDriverState *base_bs, *top_bs; > + Error *local_err =3D NULL; > + /* This will be part of the QMP command, if/when the > + * BlockdevOnError change for blkmirror makes it in > + */ > + BlockErrorAction on_error =3D BLOCK_ERR_REPORT; > + > + /* drain all i/o before commits */ > + bdrv_drain_all(); Is this technically necessary for now? Since you are forbidding actions on the active image for now, and changing the active image (via snapshot or pull) already drains all I/O, there should be nothing remaining to drain for any of the backing files. Obviously, it will be necessary in the future when you do add support for committing the active layer. > +++ b/qapi-schema.json > @@ -1404,6 +1404,41 @@ > 'returns': 'str' } > =20 > ## > +# @block-commit > +# > +# Live commit of data from child image nodes into parent nodes - i.e.,= > +# writes data between 'top' and 'base' into 'base'. > +# > +# @device: the name of the device > +# > +# @base: #optional The file name of the parent image of the device t= o write > +# data into. If not specified, this is the origina= l parent > +# image. I though we wanted to fix the terminology to avoid 'parent'; how about: The file name of the backing image to write data into. If not specified, this is the deepest backing image. > +# > +# @top: #optional The file name of the child image, above which dat= a will > +# not be committed down. If not specified, this is= one > +# layer below the active layer (i.e. active->backin= g_hd). Again, how about: The file name of the backing image within the chain which contains the data to be committed down. If not specified... > +# > +# If top =3D=3D base, that is an error. > +# > +# > +# @speed: #optional the maximum speed, in bytes per second > +# > +# Returns: Nothing on success > +# If commit or stream is already active on this device, Devic= eInUse > +# If @device does not exist, DeviceNotFound > +# If image commit is not supported by this device, NotSupport= ed > +# If @base does not exist, BaseNotFound > +# If @top does not exist, TopNotFound BaseNotFound is a generic error, and I'm arguing that TopNotFound should be likewise. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig0B0B91A50B0BE274A8856101 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQEcBAEBCAAGBQJQU9RzAAoJEKeha0olJ0Nqu9wIAJ4jvaAPiNKk98mhjtHqsah6 z+bpmPe4w7SIwqmavYvm9/K9Y+6YOKT1IF+zeyal8gdd/uj7pXp95+89AV2YiVx6 ADYS0R2s1hSixplmOEJVe1cpVlNRC+6xN1QO8hXE4LmHaL+1vMS9ZIiY61zdEvwx 99C+Y/S7ZsQ4ypjNgSOgHPds61yHux0nu9LsaohIv7M7PW4w/Xo7HNL+8P/IcbLR Xy776crGx3HdWg32lRHDylladuu7vHUK2fua5KRxAVhJ4oKXlZjitbRqiMi02gw3 aqDJ87kHIG4bBmcPRiwBhDvodxaTC0W25XsCA66ctmgTtGXdDqPlXTE2XVMBeVw= =O9fo -----END PGP SIGNATURE----- --------------enig0B0B91A50B0BE274A8856101--