From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46487) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVHJl-0004iB-6T for qemu-devel@nongnu.org; Thu, 25 Apr 2013 04:14:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVHJg-0004hh-Ff for qemu-devel@nongnu.org; Thu, 25 Apr 2013 04:14:13 -0400 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:55578) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVHJf-0004hQ-Tq for qemu-devel@nongnu.org; Thu, 25 Apr 2013 04:14:08 -0400 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 25 Apr 2013 13:39:08 +0530 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 773CE3940057 for ; Thu, 25 Apr 2013 13:44:01 +0530 (IST) Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3P8DsqJ10420710 for ; Thu, 25 Apr 2013 13:43:54 +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 r3P8E0Yc032629 for ; Thu, 25 Apr 2013 18:14:00 +1000 Message-ID: <5178E5A2.4010602@linux.vnet.ibm.com> Date: Thu, 25 Apr 2013 16:13:22 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1366333030-8564-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1366333030-8564-6-git-send-email-xiawenc@linux.vnet.ibm.com> <20130424092532.GA24635@stefanha-thinkpad.redhat.com> In-Reply-To: <20130424092532.GA24635@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V3 5/5] block: make all steps in qmp_transaction() as callback List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, dietmar@proxmox.com 于 2013-4-24 17:25, Stefan Hajnoczi 写道: > On Fri, Apr 19, 2013 at 08:57:10AM +0800, Wenchao Xia wrote: >> diff --git a/blockdev.c b/blockdev.c >> index 051be98..b336794 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -779,14 +779,41 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, >> >> >> /* New and old BlockDriverState structs for group snapshots */ >> -typedef struct BlkTransactionStates { >> + >> +typedef struct BlkTransactionStates BlkTransactionStates; >> + >> +/* Only prepare() may fail. In a single transaction, only one of commit() or >> + rollback() will be called. */ > > Please document that clean() is always called - after either commit() or > rollback(). > >> +const BdrvActionOps external_snapshot_ops = { > > static > >> @@ -909,32 +950,36 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) >> /* We don't do anything in this loop that commits us to the snapshot */ >> while (NULL != dev_entry) { >> BlockdevAction *dev_info = NULL; >> + ExternalSnapshotStates *ext; >> >> dev_info = dev_entry->value; >> dev_entry = dev_entry->next; >> >> - states = g_malloc0(sizeof(BlkTransactionStates)); >> - QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry); >> - >> switch (dev_info->kind) { >> case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC: >> - external_snapshot_prepare(dev_info, states, errp); >> - if (error_is_set(&local_err)) { >> - error_propagate(errp, local_err); >> - goto delete_and_fail; >> - } >> + ext = g_malloc0(sizeof(ExternalSnapshotStates)); >> + states = &ext->common; >> + states->ops = &external_snapshot_ops; >> break; >> default: >> abort(); >> } > > Code duplication can be avoided like this: > > typedef struct BdrvActionOps { > /* Size of state struct, in bytes */ > size_t instance_size; > /* Prepare the work, must NOT be NULL. */ > void (*prepare)(BlkTransactionStates *common, Error **errp); > /* Commit the changes, must NOT be NULL. */ > void (*commit)(BlkTransactionStates *common); > /* Rollback the changes on fail, can be NULL. */ > void (*rollback)(BlkTransactionStates *common); > /* Clean up resource in the end, can be NULL. */ > void (*clean)(BlkTransactionStates *common); > } BdrvActionOps; > > static const BdrvActionOps actions[] = { > [BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = { > .instance_size = sizeof(ExternalSnapshotStates), > .prepare = external_snapshot_prepare, > .commit = external_snapshot_commit, > .rollback = external_snapshot_rollback, > }, > }; > > Then the state struct is allocated as follows: > > assert(dev_info->kind < ARRAY_SIZE(actions)); > const BdrvActionOps *ops = &actions[dev_info->kind]; > states = g_malloc0(ops->instance_size); > states->ops = ops; > > No switch statement is necessary and the states setup doesn't need to be > duplicated when new actions are added. > It is better, will use it, thanks. -- Best Regards Wenchao Xia