From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52650) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUvxM-0004LM-OM for qemu-devel@nongnu.org; Wed, 24 Apr 2013 05:25:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UUvxH-0006IS-Sb for qemu-devel@nongnu.org; Wed, 24 Apr 2013 05:25:40 -0400 Received: from mail-ee0-f42.google.com ([74.125.83.42]:53776) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUvxH-0006IG-ML for qemu-devel@nongnu.org; Wed, 24 Apr 2013 05:25:35 -0400 Received: by mail-ee0-f42.google.com with SMTP id c41so321499eek.1 for ; Wed, 24 Apr 2013 02:25:35 -0700 (PDT) Date: Wed, 24 Apr 2013 11:25:32 +0200 From: Stefan Hajnoczi Message-ID: <20130424092532.GA24635@stefanha-thinkpad.redhat.com> References: <1366333030-8564-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1366333030-8564-6-git-send-email-xiawenc@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1366333030-8564-6-git-send-email-xiawenc@linux.vnet.ibm.com> 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: Wenchao Xia Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, dietmar@proxmox.com 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.