From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38969) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtY0v-00066A-Qx for qemu-devel@nongnu.org; Fri, 11 Jan 2013 01:22:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TtY0u-0002Ax-1d for qemu-devel@nongnu.org; Fri, 11 Jan 2013 01:22:49 -0500 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:42782) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtY0t-000290-Az for qemu-devel@nongnu.org; Fri, 11 Jan 2013 01:22:47 -0500 Received: from /spool/local by e28smtp05.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 11 Jan 2013 11:51:38 +0530 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by d28dlp03.in.ibm.com (Postfix) with ESMTP id 7EFB3125804C for ; Fri, 11 Jan 2013 11:52:50 +0530 (IST) Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r0B6MYp746071870 for ; Fri, 11 Jan 2013 11:52:34 +0530 Received: from d28av05.in.ibm.com (loopback [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r0B6MZNF006726 for ; Fri, 11 Jan 2013 17:22:35 +1100 Message-ID: <50EFAFA4.1030705@linux.vnet.ibm.com> Date: Fri, 11 Jan 2013 14:22:28 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1357543689-11415-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1357543689-11415-9-git-send-email-xiawenc@linux.vnet.ibm.com> <20130109124433.GA391@stefanha-thinkpad.redhat.com> <50EE33B2.4040504@linux.vnet.ibm.com> <20130110124109.GD30946@stefanha-thinkpad.redhat.com> In-Reply-To: <20130110124109.GD30946@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, aliguori@us.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com 于 2013-1-10 20:41, Stefan Hajnoczi 写道: > On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote: >> 于 2013-1-9 20:44, Stefan Hajnoczi 写道: >>> On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote: >>>> This patch switch to internal common API to take group external >>>> snapshots from qmp_transaction interface. qmp layer simply does >>>> a translation from user input. >>>> >>>> Signed-off-by: Wenchao Xia >>>> --- >>>> blockdev.c | 215 ++++++++++++++++++++++++------------------------------------ >>>> 1 files changed, 87 insertions(+), 128 deletions(-) >>> >>> An internal API for snapshots is not necessary. qmp_transaction() is >>> already usable both from the monitor and C code. >>> >>> The QAPI code generator creates structs that can be accessed directly >> >from C. qmp_transaction(), BlockdevAction, and BlockdevActionList *is* >>> the snapshot API. It just doesn't support internal snapshots yet, which >>> is what you are trying to add. >>> >>> To add internal snapshot support, define a BlockdevInternalSnapshot type >>> in qapi-schema.json and add internal snapshot support in >>> qmp_transaction(). >>> >>> qmp_transaction() was designed with this in mind from the beginning and >>> dispatches based on BlockdevAction->kind. >>> >>> The patch series will become much smaller while still adding internal >>> snapshot support. >>> >>> Stefan >>> >> >> As API, qmp_transaction have following disadvantages: >> 1) interface is based on string not data type inside qemu, that means >> other function calling it result in: bdrv->string->bdrv > > Use bdrv_get_device_name(). You already need to fill in filename or > snapshot name strings. This is not a big disadvantage. > Yes, not a big disadvantage, but why not save string operation but use (bdrv*) as much as possible? what happens will be: hmp-snapshot | qmp-snapshot |--------- | qmp-transaction savevm(may be other..) |----------------------| | internal transaction layer >> 2) all capability are forced to be exposed. > > Is there something you cannot expose? > As other component in qemu can use it, some option may be used only in qemu not to user. For eg, vm-state-size. >> 3) need structure to record each transaction state, such as >> BlkTransactionStates. Extending it is equal to add an internal layer. > > I agree that extending it is equal coding effort to adding an internal > layer because you'll need to refactor qmp_transaction() a bit to really > support additional action types. > > But it's the right thing to do. Don't add unnecessary layers just > because writing new code is more fun than extending existing code. > If this layer is not added but depending only qmp_transaction, there will be many "if else" fragment. I have tried that and the code is awkful, this layer did not bring extra burden only make what happens inside qmp_transaction clearer, I did not add this layer just for fun. >> Actually I started up by use qmp_transaction as API, but soon >> found that work is almost done around BlkTransactionStates, so >> added a layer around it clearly. > > BlkTransactionStates is only relevant to external snapshots because they > change the BlkDriverState chain and must be able to undo that. > > For internal snapshots you simply need to store the snapshot name so you > can delete it if there is an error partway through. (BTW it's not > possible to completely restore state if you allow overwriting existing > internal snapshots unless you do something like taking a backup snapshot > of the existing snapshot first!) > Still I need BlkTransactionStates to record BlkDriverState because the granularity is block device. And yes it is not possible to completely restore state in overwrite case, which can be solved later and mark it in comments now. > Stefan > -- Best Regards Wenchao Xia