Re: [PATCH v8 1/5] blockdev: refactor transaction to use Transaction API

[Kevin Wolf]

Am 21.04.2023 um 13:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We are going to add more block-graph modifying transaction actions,
> and block-graph modifying functions are already based on Transaction
> API.
> 
> Next, we'll need to separately update permissions after several
> graph-modifying actions, and this would be simple with help of
> Transaction API.
> 
> So, now let's just transform what we have into new-style transaction
> actions.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  blockdev.c | 317 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 186 insertions(+), 131 deletions(-)

> diff --git a/blockdev.c b/blockdev.c
> index d7b5c18f0a..293f6a958e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1380,10 +1384,9 @@ static void internal_snapshot_abort(BlkActionState 
> *common)
>      aio_context_release(aio_context);
>  }
>  
> -static void internal_snapshot_clean(BlkActionState *common)
> +static void internal_snapshot_clean(void *opaque)
>  {
> -    InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
> -                                             common, common);
> +    InternalSnapshotState *state = opaque;
>      AioContext *aio_context;
>  
>      if (!state->bs) {
> @@ -1396,6 +1399,8 @@ static void internal_snapshot_clean(BlkActionState 
> *common)
>      bdrv_drained_end(state->bs);
>  
>      aio_context_release(aio_context);
> +
> +    g_free(state);
>  }

state is leaked if we take the early return a few lines above:

    if (!state->bs) {
        return;
    }

>  /* external snapshot private data */
> @@ -1657,6 +1670,8 @@ static void external_snapshot_clean(BlkActionState 
> *common)
>      bdrv_unref(state->new_bs);
>  
>      aio_context_release(aio_context);
> +
> +    g_free(state);
>  }

Same potential leak of state.

>  typedef struct DriveBackupState {
> @@ -1856,6 +1883,8 @@ static void drive_backup_clean(BlkActionState *common)
>      bdrv_drained_end(state->bs);
>  
>      aio_context_release(aio_context);
> +
> +    g_free(state);
>  }

Here as well.

>  typedef struct BlockdevBackupState {
> @@ -1950,6 +1991,8 @@ static void blockdev_backup_clean(BlkActionState 
> *common)
>      bdrv_drained_end(state->bs);
>  
>      aio_context_release(aio_context);
> +
> +    g_free(state);
>  }

And here.

Other than that, the patch looks good to me.

Kevin