Re: [Qemu-devel] [PATCH v3 2/3] block: Add blockdev-backup to transaction

[Fam Zheng <famz@redhat.com>]

On Wed, 12/17 10:40, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > BTW add version info for other transaction types.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockdev.c       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json |  7 +++++++
> >  2 files changed, 55 insertions(+)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 2e5068c..6401850 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1495,6 +1495,49 @@ static void drive_backup_abort(BlkTransactionState *common)
> >      }
> >  }
> >  
> > +typedef struct BlockdevBackupState {
> > +    BlkTransactionState common;
> > +    BlockDriverState *bs;
> > +    BlockJob *job;
> > +} BlockdevBackupState;
> > +
> > +static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
> > +{
> > +    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
> > +    BlockdevBackup *backup;
> > +    Error *local_err = NULL;
> > +
> > +    assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
> > +    backup = common->action->blockdev_backup;
> > +
> > +    qmp_blockdev_backup(backup->device, backup->target,
> > +                        backup->sync,
> > +                        backup->has_speed, backup->speed,
> > +                        backup->has_on_source_error, backup->on_source_error,
> > +                        backup->has_on_target_error, backup->on_target_error,
> > +                        &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        state->bs = NULL;
> > +        state->job = NULL;
> > +        return;
> > +    }
> > +
> > +    state->bs = bdrv_find(backup->device);
> > +    state->job = state->bs->job;
> > +}
> > +
> > +static void blockdev_backup_abort(BlkTransactionState *common)
> > +{
> > +    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
> > +    BlockDriverState *bs = state->bs;
> > +
> > +    /* Only cancel if it's the job we started */
> > +    if (bs && bs->job && bs->job == state->job) {
> > +        block_job_cancel_sync(bs->job);
> > +    }
> > +}
> > +
> >  static void abort_prepare(BlkTransactionState *common, Error **errp)
> >  {
> >      error_setg(errp, "Transaction aborted using Abort action");
> > @@ -1517,6 +1560,11 @@ static const BdrvActionOps actions[] = {
> >          .prepare = drive_backup_prepare,
> >          .abort = drive_backup_abort,
> >      },
> > +    [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
> > +        .instance_size = sizeof(BlockdevBackupState),
> > +        .prepare = blockdev_backup_prepare,
> > +        .abort = blockdev_backup_abort,
> > +    },
> >      [TRANSACTION_ACTION_KIND_ABORT] = {
> >          .instance_size = sizeof(BlkTransactionState),
> >          .prepare = abort_prepare,
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 24379ab..5fc6892 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1254,11 +1254,18 @@
> >  #
> >  # A discriminated record of operations that can be performed with
> >  # @transaction.
> > +#
> > +# Since 1.1
> > +# drive-backup since 1.6
> > +# abort since 1.6
> > +# blockdev-snapshot-internal-sync since 1.7
> 
> I'd do these belated doc updates in a separate patch preceding this one.
> Up to you.

Okay, I remember one previous reviewer said this is fine, but since we are
talking about it, why not split it.

Fam

> 
> > +# blockdev-backup since 2.3
> >  ##
> >  { 'union': 'TransactionAction',
> >    'data': {
> >         'blockdev-snapshot-sync': 'BlockdevSnapshot',
> >         'drive-backup': 'DriveBackup',
> > +       'blockdev-backup': 'BlockdevBackup',
> >         'abort': 'Abort',
> >         'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> >     } }