Re: [PATCH v2 2/4] block: let bdrv_reopen_commit_post() report a failure

On 8/11/26 6:42 PM, Denis V. Lunev wrote:
> The callback runs after bdrv_reopen_multiple() has committed the
> transaction, so it cannot reject the reopen. It can still find that
> the node it has just made writable is unusable, and has no way to say
> so: bdrv_reopen() returns success and the caller carries on.
> 
> Give it a return value and an Error argument. The reopen stays
> committed, the error only reports that the node is gone. Every queued
> node still gets its callback, the first error is the one reported.
> qcow2 is the only implementation and does not fail yet.
> 
> Signed-off-by: Denis V. Lunev <[email protected]>
> CC: Kevin Wolf <[email protected]>
> CC: Hanna Reitz <[email protected]>
> CC: Andrey Drobyshev <[email protected]>
> Cc: [email protected]
> ---
>  block.c                          | 18 +++++++++++++++---
>  block/qcow2.c                    |  4 +++-
>  include/block/block_int-common.h |  5 +++--
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f0a6042e61..99ed06f8ca 100644
> --- a/block.c
> +++ b/block.c
> @@ -4653,15 +4653,27 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>      tran_commit(tran);
>      bdrv_graph_wrunlock();
>  
> +    ret = 0;
>      QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
>          BlockDriverState *bs = bs_entry->state.bs;
> +        Error *local_err = NULL;
> +        int commit_ret;
>  
> -        if (bs->drv->bdrv_reopen_commit_post) {
> -            bs->drv->bdrv_reopen_commit_post(&bs_entry->state);
> +        if (!bs->drv->bdrv_reopen_commit_post) {
> +            continue;
> +        }
> +
> +        commit_ret = bs->drv->bdrv_reopen_commit_post(&bs_entry->state,
> +                                                      &local_err);
> +        if (commit_ret < 0 && ret == 0) {
> +            /* Committed already, so report the first failure and go on */
> +            error_propagate(errp, local_err);
> +            ret = commit_ret;
> +        } else {
> +            error_free(local_err);
>          }
>      }
>  
> -    ret = 0;
>      goto cleanup;
>  
>  abort:
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1543255eba..553a94d003 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2145,7 +2145,7 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
>      g_free(state->opaque);
>  }
>  
> -static void qcow2_reopen_commit_post(BDRVReopenState *state)
> +static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp)
>  {
>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
> @@ -2163,6 +2163,8 @@ static void qcow2_reopen_commit_post(BDRVReopenState *state)
>                                bdrv_get_node_name(state->bs));
>          }
>      }
> +
> +    return 0;
>  }
>  
>  static void qcow2_reopen_abort(BDRVReopenState *state)
> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
> index 147c08155f..61e6621c93 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -239,8 +239,9 @@ struct BlockDriver {
>          BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp);
>      void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit)(
>          BDRVReopenState *reopen_state);
> -    void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)(
> -        BDRVReopenState *reopen_state);
> +    /* Cannot fail the reopen, an error only reports an unusable node */
> +    int GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)(
> +        BDRVReopenState *reopen_state, Error **errp);
>      void GRAPH_UNLOCKED_PTR (*bdrv_reopen_abort)(
>          BDRVReopenState *reopen_state);
>      void (*bdrv_join_options)(QDict *options, QDict *old_options);

The overall commit logic is fine, but it changes the reopen logic.
Failures up to and including permissions refresh abort the transaction,
but commit_post failure (which can now potentially happen) takes place
after actual reopen.  So now we can end up in a situation where node is
reopened, but unusable.  I think this should be better documented.

Andrey