On 09/07/2015 01:34 AM, Fam Zheng wrote: > From: Stefan Hajnoczi <stefa...@redhat.com> > > Join the transaction when the 'transactional-cancel' QMP argument is > true. > > 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. > > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > Signed-off-by: Fam Zheng <f...@redhat.com> > ---
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 as:
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. :)
...]
> +++ b/qapi/block-core.json
> @@ -736,6 +736,11 @@
> # default 'report' (no limitations, since this applies 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 background 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
