From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39809) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ttafe-0005El-Qm for qemu-devel@nongnu.org; Fri, 11 Jan 2013 04:13:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ttafc-0004C0-1w for qemu-devel@nongnu.org; Fri, 11 Jan 2013 04:13:02 -0500 Received: from mail-wg0-f48.google.com ([74.125.82.48]:65032) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ttafb-0004Bn-Kl for qemu-devel@nongnu.org; Fri, 11 Jan 2013 04:12:59 -0500 Received: by mail-wg0-f48.google.com with SMTP id dt10so692211wgb.3 for ; Fri, 11 Jan 2013 01:12:59 -0800 (PST) Date: Fri, 11 Jan 2013 10:12:53 +0100 From: Stefan Hajnoczi Message-ID: <20130111091253.GA31400@stefanha-thinkpad.muc.redhat.com> 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> <50EFAFA4.1030705@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <50EFAFA4.1030705@linux.vnet.ibm.com> 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: Wenchao Xia Cc: kwolf@redhat.com, aliguori@us.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote: > 于 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 Saving the string operation is not worth duplicating the API. > >>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. When we hit a limitation of QAPI then it needs to be extended. I'm sure there's a solution for splitting or hiding parts of the QAPI generated API. > >>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. The qmp_transaction() implementation can be changed, I'm not saying you have to hack in more if statements. It's cleanest to introduce a BdrvActionOps abstraction: typedef struct BdrvActionOps BdrvActionOps; typedef struct BdrvTransactionState { const BdrvActionOps *ops; QLIST_ENTRY(BdrvTransactionState); } BdrvTransactionState; struct BdrvActionOps { int (*prepare)(BdrvTransactionState *s, ...); int (*commit)(BdrvTransactionState *s, ...); int (*rollback)(BdrvTransactionState *s, ...); }; BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action); Then qmp_transaction() can be generic code that steps through the transactions. This is similar to what your series does and I think it's the right direction. But please don't duplicate the qmp_transaction() and BlockdevAction/BlockdevActionList APIs. In other words, change the engine, not the whole car. Stefan