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

[Alejandro Jimenez]

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