From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaVqD-0003Bq-IQ for qemu-devel@nongnu.org; Fri, 11 Sep 2015 17:26:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZaVqA-000326-BX for qemu-devel@nongnu.org; Fri, 11 Sep 2015 17:26:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41227) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaVqA-000312-4b for qemu-devel@nongnu.org; Fri, 11 Sep 2015 17:26:38 -0400 References: <1441611277-24596-1-git-send-email-famz@redhat.com> <1441611277-24596-12-git-send-email-famz@redhat.com> From: Eric Blake Message-ID: <55F34708.5000304@redhat.com> Date: Fri, 11 Sep 2015 15:26:32 -0600 MIME-Version: 1.0 In-Reply-To: <1441611277-24596-12-git-send-email-famz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="0nQpQNacCpHroiRXtx76xqkEsmU4vsS06" Subject: Re: [Qemu-devel] [PATCH v5 11/14] block/backup: support block job transactions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Jeff Cody , Max Reitz , vsementsov@parallels.com, stefanha@redhat.com, John Snow This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --0nQpQNacCpHroiRXtx76xqkEsmU4vsS06 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/07/2015 01:34 AM, Fam Zheng wrote: > From: Stefan Hajnoczi >=20 > Join the transaction when the 'transactional-cancel' QMP argument is > true. >=20 > This ensures that the sync bitmap is not thrown away if another block > job in the transaction is cancelled or fails. This is critical so > incremental backup with multiple disks can be retried in case of > cancellation/failure. >=20 > Signed-off-by: Stefan Hajnoczi > Signed-off-by: Fam Zheng > --- Interface review: > +void qmp_blockdev_backup(const char *device, const char *target, > + enum MirrorSyncMode sync, > + bool has_speed, int64_t speed, > + bool has_on_source_error, > + BlockdevOnError on_source_error, > + bool has_on_target_error, > + BlockdevOnError on_target_error, > + bool has_transactional_cancel, > + bool transactional_cancel, > + Error **errp) > +{ > + if (has_transactional_cancel && transactional_cancel) { > + error_setg(errp, "Transactional cancel can only be used in the= " > + "'transaction' command"); > + return; > + } Hmm. It might be nicer if we had two separate qapi structs: # used in 'blockdev-backup' { 'struct':'BlockdevBackup', 'data': { device ... on-target-error } } # used in 'transaction' { 'struct':'BlockdevBackupTxn', 'base':'BlockdevBackup', 'data': { 'transactional-cancel':'bool' } } so that the user can't abuse the boolean from the wrong context. [Side notes... Furthermore, with pending changes coming down the qapi pipeline, we will generate the C struct so that you can safely do '(BlockdevBackup*)blockdev_backup_txn' to go from the child back to the base class. https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02583.html > + > + do_blockdev_backup(device, target, sync, has_speed, speed, > + has_on_source_error, on_source_error, > + has_on_target_error, on_target_error, > + NULL, errp); > +} And with other changes coming down the pipe, you could write a function a= s: void do_blockdev_backup(BlockdevBackup *args, Error **errp) by adding 'box':true' to 'blockdev-backup' and make it a bit easier to pass around all the data without breaking it into multiple parameters. https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html But we're not there yet - it may have to be a simplification we apply after the fact. :) =2E..] > +++ b/qapi/block-core.json > @@ -736,6 +736,11 @@ > # default 'report' (no limitations, since this appli= es to > # a different block device than @device). > # > +# @transactional-cancel: #optional whether failure or cancellation of = other > +# block jobs with @transactional-cancel true in= the same > +# transaction causes the whole group to cancel.= > +# (Since 2.5) Mention default false. > +# > # Note that @on-source-error and @on-target-error only affect backgrou= nd I/O. > # If an error occurs during a guest write request, the device's rerror= /werror > # actions will be used. > @@ -747,7 +752,8 @@ > 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', > '*speed': 'int', '*bitmap': 'str', > '*on-source-error': 'BlockdevOnError', > - '*on-target-error': 'BlockdevOnError' } } > + '*on-target-error': 'BlockdevOnError', > + '*transactional-cancel': 'bool' } } See my above comments about the idea of using a parent and child class, rather than exposing this outside of transaction, especially since you didn't document that it can't be used outside of a transaction. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --0nQpQNacCpHroiRXtx76xqkEsmU4vsS06 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV80cIAAoJEKeha0olJ0Nqyc4H/RQH1t3sRljGgPVAC90F8fxh 7moY7Jycwj23R+1kP29ITw1VK/1e1btme3pJnqBw3EzaVBa9kUIosCWOv6bs9BIO C0knQGUH2fb4yQvYg2MmpA4urxF2VKxe2LkRmERuWnJQT1Bw6oJ5+Nt3836C1RO8 qBtqTdU9a8ri91KlYZrjTcCGv5o4yRLfyGKIstGBy69bQZZCb3x3JsdkvgvD/B+y ErXHFJINl3CSm1TWjQRRWEHxI5dAweX3qvWvTcolDNOd0RHkm279R/egJkAhp95I dA4n0C+WSGVVZoVjzeDTf0Sko8MAgWq9EgDJitm3TUdf5iVN/iaXkGi7DPX1b6c= =5P/3 -----END PGP SIGNATURE----- --0nQpQNacCpHroiRXtx76xqkEsmU4vsS06--