From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56090) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCMig-00040E-MP for qemu-devel@nongnu.org; Tue, 07 Jul 2015 02:51:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCMid-00074v-9Y for qemu-devel@nongnu.org; Tue, 07 Jul 2015 02:51:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58106) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCMic-00074k-M8 for qemu-devel@nongnu.org; Tue, 07 Jul 2015 02:51:03 -0400 Date: Tue, 7 Jul 2015 14:50:58 +0800 From: Fam Zheng Message-ID: <20150707065058.GB9515@ad.nay.redhat.com> References: <1436192669-10062-1-git-send-email-stefanha@redhat.com> <1436192669-10062-4-git-send-email-stefanha@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436192669-10062-4-git-send-email-stefanha@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 03/10] block: rename BlkTransactionState and BdrvActionOps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, John Snow On Mon, 07/06 15:24, Stefan Hajnoczi wrote: > From: John Snow > > These structures are misnomers, somewhat. > > (1) BlockTransactionState is not state for a transaction, > but is rather state for a single transaction action. > Rename it "BlkActionState" to be more accurate. > > (2) The BdrvActionOps describes operations for the BlkActionState, > above. This name might imply a 'BdrvAction' or a 'BdrvActionState', > which there isn't. > Rename this to 'BlkActionOps' to match 'BlkActionState'. > > Lastly, update the surrounding in-line documentation and comments > to reflect the current nature of how Transactions operate. > > This patch changes only comments and names, and should not affect > behavior in any way. > > Signed-off-by: John Snow > Reviewed-by: Max Reitz > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Stefan Hajnoczi Reviewed-by: Fam Zheng > --- > blockdev.c | 116 ++++++++++++++++++++++++++++++++++--------------------------- > 1 file changed, 65 insertions(+), 51 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index a4d8f65..0ab8ad9 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1240,43 +1240,57 @@ static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node, > > /* New and old BlockDriverState structs for atomic group operations */ > > -typedef struct BlkTransactionState BlkTransactionState; > +typedef struct BlkActionState BlkActionState; > > -/* Only prepare() may fail. In a single transaction, only one of commit() or > - abort() will be called, clean() will always be called if it present. */ > -typedef struct BdrvActionOps { > - /* Size of state struct, in bytes. */ > +/** > + * BlkActionOps: > + * Table of operations that define an Action. > + * > + * @instance_size: Size of state struct, in bytes. > + * @prepare: Prepare the work, must NOT be NULL. > + * @commit: Commit the changes, can be NULL. > + * @abort: Abort the changes on fail, can be NULL. > + * @clean: Clean up resources after all transaction actions have called > + * commit() or abort(). Can be NULL. > + * > + * Only prepare() may fail. In a single transaction, only one of commit() or > + * abort() will be called. clean() will always be called if it is present. > + */ > +typedef struct BlkActionOps { > size_t instance_size; > - /* Prepare the work, must NOT be NULL. */ > - void (*prepare)(BlkTransactionState *common, Error **errp); > - /* Commit the changes, can be NULL. */ > - void (*commit)(BlkTransactionState *common); > - /* Abort the changes on fail, can be NULL. */ > - void (*abort)(BlkTransactionState *common); > - /* Clean up resource in the end, can be NULL. */ > - void (*clean)(BlkTransactionState *common); > -} BdrvActionOps; > + void (*prepare)(BlkActionState *common, Error **errp); > + void (*commit)(BlkActionState *common); > + void (*abort)(BlkActionState *common); > + void (*clean)(BlkActionState *common); > +} BlkActionOps; > > -/* > - * This structure must be arranged as first member in child type, assuming > - * that compiler will also arrange it to the same address with parent instance. > - * Later it will be used in free(). > +/** > + * BlkActionState: > + * Describes one Action's state within a Transaction. > + * > + * @action: QAPI-defined enum identifying which Action to perform. > + * @ops: Table of ActionOps this Action can perform. > + * @entry: List membership for all Actions in this Transaction. > + * > + * This structure must be arranged as first member in a subclassed type, > + * assuming that the compiler will also arrange it to the same offsets as the > + * base class. > */ > -struct BlkTransactionState { > +struct BlkActionState { > TransactionAction *action; > - const BdrvActionOps *ops; > - QSIMPLEQ_ENTRY(BlkTransactionState) entry; > + const BlkActionOps *ops; > + QSIMPLEQ_ENTRY(BlkActionState) entry; > }; > > /* internal snapshot private data */ > typedef struct InternalSnapshotState { > - BlkTransactionState common; > + BlkActionState common; > BlockDriverState *bs; > AioContext *aio_context; > QEMUSnapshotInfo sn; > } InternalSnapshotState; > > -static void internal_snapshot_prepare(BlkTransactionState *common, > +static void internal_snapshot_prepare(BlkActionState *common, > Error **errp) > { > Error *local_err = NULL; > @@ -1372,7 +1386,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common, > state->bs = bs; > } > > -static void internal_snapshot_abort(BlkTransactionState *common) > +static void internal_snapshot_abort(BlkActionState *common) > { > InternalSnapshotState *state = > DO_UPCAST(InternalSnapshotState, common, common); > @@ -1395,7 +1409,7 @@ static void internal_snapshot_abort(BlkTransactionState *common) > } > } > > -static void internal_snapshot_clean(BlkTransactionState *common) > +static void internal_snapshot_clean(BlkActionState *common) > { > InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState, > common, common); > @@ -1407,13 +1421,13 @@ static void internal_snapshot_clean(BlkTransactionState *common) > > /* external snapshot private data */ > typedef struct ExternalSnapshotState { > - BlkTransactionState common; > + BlkActionState common; > BlockDriverState *old_bs; > BlockDriverState *new_bs; > AioContext *aio_context; > } ExternalSnapshotState; > > -static void external_snapshot_prepare(BlkTransactionState *common, > +static void external_snapshot_prepare(BlkActionState *common, > Error **errp) > { > BlockDriver *drv; > @@ -1534,7 +1548,7 @@ static void external_snapshot_prepare(BlkTransactionState *common, > } > } > > -static void external_snapshot_commit(BlkTransactionState *common) > +static void external_snapshot_commit(BlkActionState *common) > { > ExternalSnapshotState *state = > DO_UPCAST(ExternalSnapshotState, common, common); > @@ -1552,7 +1566,7 @@ static void external_snapshot_commit(BlkTransactionState *common) > aio_context_release(state->aio_context); > } > > -static void external_snapshot_abort(BlkTransactionState *common) > +static void external_snapshot_abort(BlkActionState *common) > { > ExternalSnapshotState *state = > DO_UPCAST(ExternalSnapshotState, common, common); > @@ -1565,13 +1579,13 @@ static void external_snapshot_abort(BlkTransactionState *common) > } > > typedef struct DriveBackupState { > - BlkTransactionState common; > + BlkActionState common; > BlockDriverState *bs; > AioContext *aio_context; > BlockJob *job; > } DriveBackupState; > > -static void drive_backup_prepare(BlkTransactionState *common, Error **errp) > +static void drive_backup_prepare(BlkActionState *common, Error **errp) > { > DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); > BlockDriverState *bs; > @@ -1612,7 +1626,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp) > state->job = state->bs->job; > } > > -static void drive_backup_abort(BlkTransactionState *common) > +static void drive_backup_abort(BlkActionState *common) > { > DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); > BlockDriverState *bs = state->bs; > @@ -1623,7 +1637,7 @@ static void drive_backup_abort(BlkTransactionState *common) > } > } > > -static void drive_backup_clean(BlkTransactionState *common) > +static void drive_backup_clean(BlkActionState *common) > { > DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); > > @@ -1633,13 +1647,13 @@ static void drive_backup_clean(BlkTransactionState *common) > } > > typedef struct BlockdevBackupState { > - BlkTransactionState common; > + BlkActionState common; > BlockDriverState *bs; > BlockJob *job; > AioContext *aio_context; > } BlockdevBackupState; > > -static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp) > +static void blockdev_backup_prepare(BlkActionState *common, Error **errp) > { > BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); > BlockdevBackup *backup; > @@ -1688,7 +1702,7 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp) > state->job = state->bs->job; > } > > -static void blockdev_backup_abort(BlkTransactionState *common) > +static void blockdev_backup_abort(BlkActionState *common) > { > BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); > BlockDriverState *bs = state->bs; > @@ -1699,7 +1713,7 @@ static void blockdev_backup_abort(BlkTransactionState *common) > } > } > > -static void blockdev_backup_clean(BlkTransactionState *common) > +static void blockdev_backup_clean(BlkActionState *common) > { > BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); > > @@ -1709,7 +1723,7 @@ static void blockdev_backup_clean(BlkTransactionState *common) > } > > typedef struct BlockDirtyBitmapState { > - BlkTransactionState common; > + BlkActionState common; > BdrvDirtyBitmap *bitmap; > BlockDriverState *bs; > AioContext *aio_context; > @@ -1717,7 +1731,7 @@ typedef struct BlockDirtyBitmapState { > bool prepared; > } BlockDirtyBitmapState; > > -static void block_dirty_bitmap_add_prepare(BlkTransactionState *common, > +static void block_dirty_bitmap_add_prepare(BlkActionState *common, > Error **errp) > { > Error *local_err = NULL; > @@ -1738,7 +1752,7 @@ static void block_dirty_bitmap_add_prepare(BlkTransactionState *common, > } > } > > -static void block_dirty_bitmap_add_abort(BlkTransactionState *common) > +static void block_dirty_bitmap_add_abort(BlkActionState *common) > { > BlockDirtyBitmapAdd *action; > BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > @@ -1753,7 +1767,7 @@ static void block_dirty_bitmap_add_abort(BlkTransactionState *common) > } > } > > -static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common, > +static void block_dirty_bitmap_clear_prepare(BlkActionState *common, > Error **errp) > { > BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > @@ -1782,7 +1796,7 @@ static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common, > /* AioContext is released in .clean() */ > } > > -static void block_dirty_bitmap_clear_abort(BlkTransactionState *common) > +static void block_dirty_bitmap_clear_abort(BlkActionState *common) > { > BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > common, common); > @@ -1790,7 +1804,7 @@ static void block_dirty_bitmap_clear_abort(BlkTransactionState *common) > bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup); > } > > -static void block_dirty_bitmap_clear_commit(BlkTransactionState *common) > +static void block_dirty_bitmap_clear_commit(BlkActionState *common) > { > BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > common, common); > @@ -1798,7 +1812,7 @@ static void block_dirty_bitmap_clear_commit(BlkTransactionState *common) > hbitmap_free(state->backup); > } > > -static void block_dirty_bitmap_clear_clean(BlkTransactionState *common) > +static void block_dirty_bitmap_clear_clean(BlkActionState *common) > { > BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > common, common); > @@ -1808,17 +1822,17 @@ static void block_dirty_bitmap_clear_clean(BlkTransactionState *common) > } > } > > -static void abort_prepare(BlkTransactionState *common, Error **errp) > +static void abort_prepare(BlkActionState *common, Error **errp) > { > error_setg(errp, "Transaction aborted using Abort action"); > } > > -static void abort_commit(BlkTransactionState *common) > +static void abort_commit(BlkActionState *common) > { > g_assert_not_reached(); /* this action never succeeds */ > } > > -static const BdrvActionOps actions[] = { > +static const BlkActionOps actions[] = { > [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = { > .instance_size = sizeof(ExternalSnapshotState), > .prepare = external_snapshot_prepare, > @@ -1838,7 +1852,7 @@ static const BdrvActionOps actions[] = { > .clean = blockdev_backup_clean, > }, > [TRANSACTION_ACTION_KIND_ABORT] = { > - .instance_size = sizeof(BlkTransactionState), > + .instance_size = sizeof(BlkActionState), > .prepare = abort_prepare, > .commit = abort_commit, > }, > @@ -1869,10 +1883,10 @@ static const BdrvActionOps actions[] = { > void qmp_transaction(TransactionActionList *dev_list, Error **errp) > { > TransactionActionList *dev_entry = dev_list; > - BlkTransactionState *state, *next; > + BlkActionState *state, *next; > Error *local_err = NULL; > > - QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states; > + QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states; > QSIMPLEQ_INIT(&snap_bdrv_states); > > /* drain all i/o before any operations */ > @@ -1881,7 +1895,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp) > /* We don't do anything in this loop that commits us to the operations */ > while (NULL != dev_entry) { > TransactionAction *dev_info = NULL; > - const BdrvActionOps *ops; > + const BlkActionOps *ops; > > dev_info = dev_entry->value; > dev_entry = dev_entry->next; > -- > 2.4.3 >