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: Fri, 19 Jul 2013 17:19:46 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130620 Thunderbird/17.0.7

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

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




reply via email to

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