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

[Vladimir Sementsov-Ogievskiy]

On 10.05.23 14:47, Kevin Wolf wrote:
> 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.

Agree, that's a tricky place and better be handled separately.

> The rest of the patch looks good.
>
> Kevin

Thanks for reviewing, I'll resend soon.

--
Best regards,
Vladimir