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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

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