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

[Kevin Wolf <kwolf@redhat.com>]

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 <xiawenc@linux.vnet.ibm.com>
> ---
>  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.

> +        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 {' ?

> +        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.

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?

> +    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.

> +    }
> +}

Kevin