From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36050) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V0Kkn-0005fE-A4 for qemu-devel@nongnu.org; Fri, 19 Jul 2013 20:10:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V0Kkk-0007zQ-Va for qemu-devel@nongnu.org; Fri, 19 Jul 2013 20:10:29 -0400 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:36331) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V0Kkk-0007xM-CB for qemu-devel@nongnu.org; Fri, 19 Jul 2013 20:10:26 -0400 Received: from /spool/local by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sat, 20 Jul 2013 10:00:10 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id C642F2BB004F for ; Sat, 20 Jul 2013 10:10:13 +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 r6JNsjMY6947088 for ; Sat, 20 Jul 2013 09:54:46 +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 r6K0ACx6016487 for ; Sat, 20 Jul 2013 10:10:12 +1000 Message-ID: <51E9D553.4040306@linux.vnet.ibm.com> Date: Sat, 20 Jul 2013 08:09:55 +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> <51E904B2.8060702@linux.vnet.ibm.com> <20130719101354.GG2992@dhcp-200-207.str.redhat.com> In-Reply-To: <20130719101354.GG2992@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, qemu-devel@nongnu.org, armbru@redhat.com, lcapitulino@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com 于 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 >>>> --- >>>> 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