Re: [Qemu-devel] [PATCH v5 11/14] block/backup: support block job transactions

[Fam Zheng <famz@redhat.com>]

On Fri, 09/11 15:26, Eric Blake wrote:
> On 09/07/2015 01:34 AM, Fam Zheng wrote:
> > From: Stefan Hajnoczi <stefanha@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 <stefanha@redhat.com>
> > Signed-off-by: Fam Zheng <famz@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.

Very good point, will do that in v6.

Fam