Re: [PATCH v8 5/5] blockdev: qmp_transaction: drop extra generic layer

[Kevin Wolf <kwolf@redhat.com>]

Am 21.04.2023 um 13:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Let's simplify things:
> 
> First, actions generally don't need access to common BlkActionState
> structure. The only exclusion are backup actions that need
> block_job_txn.
> 
> Next, for transaction actions of Transaction API is more native to
> allocated state structure in the action itself.
> 
> So, do the following transformation:
> 
> 1. Let all actions be represented by a function with corresponding
>    structure as arguments.
> 
> 2. Instead of array-map marshaller, let's make a function, that calls
>    corresponding action directly.
> 
> 3. BlkActionOps and BlkActionState structures become unused
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> @@ -1973,11 +1910,9 @@ static void blockdev_backup_clean(void *opaque)
>  }
>  
>  typedef struct BlockDirtyBitmapState {
> -    BlkActionState common;
>      BdrvDirtyBitmap *bitmap;
>      BlockDriverState *bs;
>      HBitmap *backup;
> -    bool prepared;
>      bool was_enabled;
>  } BlockDirtyBitmapState;
>  
> @@ -1987,17 +1922,14 @@ TransactionActionDrv block_dirty_bitmap_add_drv = {
>      .clean = g_free,
>  };
>  
> -static void block_dirty_bitmap_add_action(BlkActionState *common,
> +static void block_dirty_bitmap_add_action(BlockDirtyBitmapAdd *action,
>                                            Transaction *tran, Error **errp)
>  {
>      Error *local_err = NULL;
> -    BlockDirtyBitmapAdd *action;
> -    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> -                                             common, common);
> +    BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
>  
>      tran_add(tran, &block_dirty_bitmap_add_drv, state);
>  
> -    action = common->action->u.block_dirty_bitmap_add.data;
>      /* AIO context taken and released within qmp_block_dirty_bitmap_add */
>      qmp_block_dirty_bitmap_add(action->node, action->name,
>                                 action->has_granularity, action->granularity,
> @@ -2006,7 +1938,8 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
>                                 &local_err);
>  
>      if (!local_err) {
> -        state->prepared = true;
> +        state->bitmap = block_dirty_bitmap_lookup(action->node, action->name,
> +                                                  NULL, &error_abort);
>      } else {
>          error_propagate(errp, local_err);
>      }
> @@ -2014,15 +1947,10 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
>  
>  static void block_dirty_bitmap_add_abort(void *opaque)
>  {
> -    BlockDirtyBitmapAdd *action;
>      BlockDirtyBitmapState *state = opaque;
>  
> -    action = state->common.action->u.block_dirty_bitmap_add.data;
> -    /* Should not be able to fail: IF the bitmap was added via .prepare(),
> -     * then the node reference and bitmap name must have been valid.
> -     */
> -    if (state->prepared) {
> -        qmp_block_dirty_bitmap_remove(action->node, action->name, &error_abort);
> +    if (state->bitmap) {
> +        bdrv_release_dirty_bitmap(state->bitmap);
>      }
>  }

So you're moving the lookup of the bitmap from .abort to .prepare (or
*_action now). I'm not entirely sure how this is related to the goal of
this specific patch. So the first question is, should it be separate?

The second question is if it is right. Moving it like this means we must
be sure that the bitmap can't be deleted between the lookup and the
.abort call. How can we guarantee this?

On the other hand, we already used &error_abort before, so the
assumption isn't actually new. Just the failure mode changes from
abort() to accessing a dangling pointer, which could be a bit worse.

The rest of the patch looks good.

Kevin