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

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

于 2013-7-19 18:13, Kevin Wolf 写道:
> Am 19.07.2013 um 11:19 hat Wenchao Xia geschrieben:
>> 于 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);
>
> Oh, yes. I was confused by the fact that there is a local sn, which
> isn't related to sn1 at all. Perhaps some renaming to make things
> clearer?
>
> Kevin
>
   sure, will rename local sn to sn_old.

-- 
Best Regards

Wenchao Xia