Re: [Qemu-devel] [PATCH v2 03/10] block: rename BlkTransactionState and BdrvActionOps

[Fam Zheng]

On Mon, 07/06 15:24, Stefan Hajnoczi wrote:
> From: John Snow <address@hidden>
> 
> 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 <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>

Reviewed-by: Fam Zheng <address@hidden>

> ---
>  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
>