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

Add a QMP command to allow for the behaviors specified by the
-action event=action command line option to be set at runtime.
The new command is named set-action, and takes a single argument
of type RunStateAction, which contains an event|action pair.

Example:

-> { "execute": "set-action",
     "arguments": { "pair": {
         "event": "shutdown",
	  "action": "pause" } } }
<- { "return": {} }

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 qapi/run-state.json       | 22 ++++++++++++++++++++++
 softmmu/runstate-action.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

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

On 12/9/2020 4:43 PM, Paolo Bonzini wrote:
> 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.
>
> ?
I misunderstood your request in v1. I'll try to be explicit to avoid 
more confusion. Are you expecting a command of the form:

{ 'command': 'set-action',
'data' : {
     '*reboot': 'RebootAction',
     '*shutdown': 'ShutdownAction',
     '*panic': 'PanicAction',
     '*watchdog': 'WatchdogAction' } }
?

Or is it better to encapsulate all of those optional fields inside a new 
struct definition (RunStateActions?) so that the command would be:

{ 'command': 'set-action', 'data': 'actions' : 'RunStateActions' }

which is what the "actions->has_reboot" example seems to suggest?

Or is it something else that I am not understanding yet?

>
> Note that, in patches 1-2, you don't need to add an Error** argument 
> to functions that cannot fail.
This was left over from the initial patch. I'll fix it.

Alejandro
>
> Thanks,
>
> Paolo
>

On 10/12/20 04:21, Alejandro Jimenez wrote:
> I misunderstood your request in v1. 

Oh ypu're right, in v1 you had multiple commands.  My fault then.

> 
> { 'command': 'set-action',
> 'data' : {
>      '*reboot': 'RebootAction',
>      '*shutdown': 'ShutdownAction',
>      '*panic': 'PanicAction',
>      '*watchdog': 'WatchdogAction' } }
> ?
> 
> Or is it better to encapsulate all of those optional fields inside a new 
> struct definition (RunStateActions?) so that the command would be:
> 
> { 'command': 'set-action', 'data': 'actions' : 'RunStateActions' }

Any of the two is fine; the QMP stream is the same.  I used 
actions->reboot because that's what you did in v2.

While at it, you might add

    'allow-preconfig': true,

as well.  (Right now there are relatively few allow-preconfig commands, 
but I'm in the process of adding it to all commands where it makes sense).

Thanks,

Paolo