Re: [PATCH v2 3/4] qmp: Allow setting -action parameters on the fly

[Paolo Bonzini]

On 09/12/20 18:52, Alejandro Jimenez wrote:
> +# Set the action that will be taken by the emulator in response to a guest
> +# event.
> +#
> +# @pair: a @RunStateAction type that describes an event|action pair.
> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 6.0
> +#
> +# Example:
> +#
> +# -> { "execute": "set-action",
> +#         "arguments": { "pair": {
> +#             "event": "shutdown",
> +#             "action": "pause" } } }
> +# <- { "return": {} }
> +##
> +{ 'command': 'set-action', 'data' : {'pair': 'RunStateAction'} }
> +
> +##
>   # @GUEST_PANICKED:
>   #
>   # Emitted when guest OS panic is detected
> diff --git a/softmmu/runstate-action.c b/softmmu/runstate-action.c
> index a644d80..7877e7e 100644
> --- a/softmmu/runstate-action.c
> +++ b/softmmu/runstate-action.c
> @@ -80,6 +80,35 @@ static void panic_set_action(PanicAction action, Error 
> **errp)
>   }
>   
>   /*
> + * Receives a RunStateAction type which represents an event|action pair
> + * and sets the internal state as requested.
> + */
> +void qmp_set_action(RunStateAction *pair, Error **errp)
> +{
> +    switch (pair->event) {
> +    case RUN_STATE_EVENT_TYPE_REBOOT:
> +        reboot_set_action(pair->u.reboot.action, NULL);
> +        break;
> +    case RUN_STATE_EVENT_TYPE_SHUTDOWN:
> +        shutdown_set_action(pair->u.shutdown.action, NULL);
> +        break;
> +    case RUN_STATE_EVENT_TYPE_PANIC:
> +        panic_set_action(pair->u.panic.action, NULL);
> +        break;
> +    case RUN_STATE_EVENT_TYPE_WATCHDOG:
> +        qmp_watchdog_set_action(pair->u.watchdog.action, NULL);
> +        break;
> +    default:
> +        /*
> +         * The fields in the RunStateAction argument are validated
> +         * by the QMP marshalling code before this function is called.
> +         * This case is unreachable unless new variants are added.
> +         */
> +        g_assert_not_reached();
> +    }
> +}
> +

Any reason not to have the multiple optional arguments as discussed in 
v1 (no reply usually means you agree)?  The implementation would be 
nice, like

    if (actions->has_reboot) {
        reboot_set_action(actions->reboot);
    }
    etc.

?

Note that, in patches 1-2, you don't need to add an Error** argument to 
functions that cannot fail.

Thanks,

Paolo