Re: [PATCH v8 4/5] blockdev: transaction: refactor handling transaction properties

[Kevin Wolf <kwolf@redhat.com>]

Am 21.04.2023 um 13:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Only backup supports GROUPED mode. Make this logic more clear. And
> avoid passing extra thing to each action.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  blockdev.c | 92 +++++++++++-------------------------------------------
>  1 file changed, 19 insertions(+), 73 deletions(-)

> @@ -2376,18 +2310,34 @@ void qmp_transaction(TransactionActionList *actions,
>                       Error **errp)
>  {
>      TransactionActionList *act;
> -    bool has_properties = !!properties;
>      JobTxn *block_job_txn = NULL;
>      Error *local_err = NULL;
>      Transaction *tran = tran_new();
> +    ActionCompletionMode comp_mode =
> +        properties ? properties->completion_mode :
> +        ACTION_COMPLETION_MODE_INDIVIDUAL;
>  
>      GLOBAL_STATE_CODE();
>  
>      /* Does this transaction get canceled as a group on failure?
>       * If not, we don't really need to make a JobTxn.
>       */
> -    properties = get_transaction_properties(properties);
> -    if (properties->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
> +    if (comp_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
> +        for (act = actions; act; act = act->next) {
> +            TransactionActionKind type = act->value->type;
> +
> +            if (type != TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP &&
> +                type != TRANSACTION_ACTION_KIND_DRIVE_BACKUP)
> +            {
> +                error_setg(errp,
> +                           "Action '%s' does not support Transaction property "

Should this be lower case "transaction"?

> +                           "completion-mode = %s",
> +                           TransactionActionKind_str(type),
> +                           ActionCompletionMode_str(comp_mode));
> +                return;

This leaks tran.

> +            }
> +        }
> +
>          block_job_txn = job_txn_new();
>      }

Kevin