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.
> + 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 {' ?
> + 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.
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?
> + 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.
> + }
> +}
Kevin
