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 <xiaw...@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
