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

[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 <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);

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