Re: [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction

[Wenchao Xia]

于 2013-7-18 20:22, Kevin Wolf 写道:
> Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
> > Unlike savevm, the qmp_transaction interface will not generate
> > snapshot name automatically, saving trouble to return information
> > of the new created snapshot.
> >
> > Although qcow2 support storing multiple snapshots with same name
> > but different ID, here it will fail when an snapshot with that name
> > already exist before the operation. Format such as rbd do not support
> > ID at all, and in most case, it means trouble to user when he faces
> > multiple snapshots with same name, so ban that case. Request with
> > empty name will be rejected.
> >
> > Snapshot ID can't be specified in this interface.
> >
> > Signed-off-by: Wenchao Xia <address@hidden>
> > ---
> >   blockdev.c       |  117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   qapi-schema.json |   18 ++++++++-
> >   qmp-commands.hx  |   34 ++++++++++++----
> >   3 files changed, 160 insertions(+), 9 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index b3a57e0..6554768 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -808,6 +808,118 @@ struct BlkTransactionState {
> >       QSIMPLEQ_ENTRY(BlkTransactionState) entry;
> >   };
> >
> > +/* internal snapshot private data */
> > +typedef struct InternalSnapshotState {
> > +    BlkTransactionState common;
> > +    BlockDriverState *bs;
> > +    QEMUSnapshotInfo sn;
> > +} InternalSnapshotState;
> > +
> > +static void internal_snapshot_prepare(BlkTransactionState *common,
> > +                                      Error **errp)
> > +{
> > +    const char *device;
> > +    const char *name;
> > +    BlockDriverState *bs;
> > +    QEMUSnapshotInfo sn, *sn1;
> > +    bool ret;
> > +    qemu_timeval tv;
> > +    BlockdevSnapshotInternal *internal;
> > +    InternalSnapshotState *state;
> > +    int ret1;
> > +
> > +    g_assert(common->action->kind ==
> > +             TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
> > +    internal = common->action->blockdev_snapshot_internal_sync;
> > +    state = DO_UPCAST(InternalSnapshotState, common, common);
> > +
> > +    /* 1. parse input */
> > +    device = internal->device;
> > +    name = internal->name;
> > +
> > +    /* 2. check for validation */
> > +    bs = bdrv_find(device);
> > +    if (!bs) {
> > +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > +        return;
> > +    }
> > +
> > +    if (!bdrv_is_inserted(bs)) {
> > +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> > +        return;
> > +    }
> > +
> > +    if (bdrv_is_read_only(bs)) {
> > +        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
> > +        return;
> > +    }
> > +
> > +    if (!bdrv_can_snapshot(bs)) {
> > +        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> > +                  bs->drv->format_name, device, "internal snapshot");
> > +        return;
> > +    }
> > +
> > +    if (!strlen(name)) {
> > +        error_setg(errp, "Name is empty on device '%s'", device);
>
> Name is empty. This has nothing to do with the device.
  OK.

> > +        return;
> > +    }
> > +
> > +    /* check whether a snapshot with name exist */
> > +    ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
> > +    if (error_is_set(errp)) {
> > +        return;
> > +    }
> > +    if (ret) {
>
> Save one line with '} else if {' ?
  will change, thanks.

> > +        error_setg(errp,
> > +                   "Snapshot with name '%s' already exists on device '%s'",
> > +                   name, device);
> > +        return;
> > +    }
> > +
> > +    /* 3. take the snapshot */
> > +    sn1 = &state->sn;
>
> do_savevm() has a memset() to clear all of sn1 before it starts filling
> it in. Should we do the same here? For example sn1.vm_state_size looks
> undefined.
  I think state->sn is set to zero by
qmp_transaction():
state = g_malloc0(ops->instance_size);

> It also stops the VM before saving the snapshot. I don't think this is
> necessary here because we don't save the VM state, but do we need at
> least the bdrv_flush/bdrv_drain_all part of it?
  bdrv_drain_all() is called already in qmp_transaction(). But I plan
add an explicit API later for bdrv_flush/bdrv_drain, which ensure data
is going to underlining storage.

> > +    pstrcpy(sn1->name, sizeof(sn1->name), name);
> > +    qemu_gettimeofday(&tv);
> > +    sn1->date_sec = tv.tv_sec;
> > +    sn1->date_nsec = tv.tv_usec * 1000;
> > +    sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
> > +
> > +    ret1 = bdrv_snapshot_create(bs, sn1);
> > +    if (ret1 < 0) {
> > +        error_setg_errno(errp, -ret1,
> > +                         "Failed to create snapshot '%s' on device '%s'",
> > +                         name, device);
> > +        return;
> > +    }
> > +
> > +    /* 4. succeed, mark a snapshot is created */
> > +    state->bs = bs;
> > +}
> > +
> > +static void internal_snapshot_abort(BlkTransactionState *common)
> > +{
> > +    InternalSnapshotState *state =
> > +                             DO_UPCAST(InternalSnapshotState, common, common);
> > +    BlockDriverState *bs = state->bs;
> > +    QEMUSnapshotInfo *sn = &state->sn;
> > +    Error *local_error = NULL;
> > +
> > +    if (!bs) {
> > +        return;
> > +    }
> > +
> > +    if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
> > +        error_report("Failed to delete snapshot with id '%s' and name '%s' on "
> > +                     "device '%s' in abort, reason is: '%s'",
> > +                     sn->id_str,
> > +                     sn->name,
> > +                     bdrv_get_device_name(bs),
> > +                     error_get_pretty(local_error));
> > +        error_free(local_error);
>
> See, here you're doing the right thing if bdrv_snapshot_delete() returns
> simple errors like "Failed to remove from snapshot list". With the
> changes the earlier patch made to qcow2, you end up with this, though:
>
>      Failed to delete snapshot with id 'uninitialised' and name 'test' on
>      device 'ide0-hd0' in abort, reason is: 'Failed to remove snapshot
>      with ID 'uninitialised' and name 'test' from the snapshot list on
>      device 'ide0-hd0''
>
> We need to standardise on the minimal error information that makes the
> error unambiguous in order to avoid such duplication.
>
> To sum up: Leave this code as it is, but change qcow2 etc. to remove ID,
> name and device from their messages.
  OK, will remove them in the implementions.

> > +    }
> > +}
>
> Kevin


--
Best Regards

Wenchao Xia