From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58208) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V06rC-0004M8-SW for qemu-devel@nongnu.org; Fri, 19 Jul 2013 05:20:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V06r6-0005Ee-PT for qemu-devel@nongnu.org; Fri, 19 Jul 2013 05:20:10 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:40114) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V06r6-0005A2-1D for qemu-devel@nongnu.org; Fri, 19 Jul 2013 05:20:04 -0400 Received: from /spool/local by e23smtp06.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 19 Jul 2013 19:12:18 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id C81842CE804D for ; Fri, 19 Jul 2013 19:20:00 +1000 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r6J94W1H3015054 for ; Fri, 19 Jul 2013 19:04:33 +1000 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r6J9Jx1k005180 for ; Fri, 19 Jul 2013 19:19:59 +1000 Message-ID: <51E904B2.8060702@linux.vnet.ibm.com> Date: Fri, 19 Jul 2013 17:19:46 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1373521624-4380-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1373521624-4380-4-git-send-email-xiawenc@linux.vnet.ibm.com> <20130718122226.GJ3582@dhcp-200-207.str.redhat.com> In-Reply-To: <20130718122226.GJ3582@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: phrdina@redhat.com, famz@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com 于 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 >> --- >> 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