qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction
Date: Sat, 20 Jul 2013 08:09:55 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130620 Thunderbird/17.0.7

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

  sure, will rename local sn to sn_old.

--
Best Regards

Wenchao Xia




reply via email to

[Prev in Thread] Current Thread [Next in Thread]