Re: [Qemu-devel] [PATCH v4 06/17] monitor: unregister conditional commands

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> ----- Original Message -----
>> address@hidden writes:
>> 
>> > From: Marc-André Lureau <address@hidden>
>> >
>> > The current monitor dispatch codes doesn't know commands that have been
>> > filtered out during qmp-commands.hx preprocessing. query-commands
>> > doesn't list them either. However, qapi generator doesn't filter them
>> > out, and they are listed in the command list.
>> >
>> > For now, disable the commands that aren't avaible to avoid introducing a
>> > regression there when switching to qmp_dispatch() or listing commands
>> > from the generated qapi code.
>> >
>> > Signed-off-by: Marc-André Lureau <address@hidden>
>> > ---
>> >  monitor.c | 23 +++++++++++++++++++++++
>> >  1 file changed, 23 insertions(+)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index b7ae552..ef946ad 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -1008,6 +1008,26 @@ static void qmp_query_qmp_schema(QDict *qdict,
>> > QObject **ret_data,
>> >      *ret_data = qobject_from_json(qmp_schema_json);
>> >  }
>> >  
>> > +/*
>> > + * Those commands are registered unconditionnally by generated
>> > + * qmp files. FIXME: Educate the QAPI schema on #ifdef commands.
>> > + */
>> > +static void qmp_disable_marshal(void)
>> > +{
>> > +#ifndef CONFIG_SPICE
>> > +    qmp_disable_command("query-spice");
>> > +#endif
>> > +#ifndef TARGET_I386
>> > +    qmp_disable_command("rtc-reset-reinjection");
>> > +#endif
>> > +#ifndef TARGET_S390X
>> > +    qmp_disable_command("dump-skeys");
>> > +#endif
>> > +#ifndef TARGET_ARM
>> > +    qmp_disable_command("query-gic-capabilities");
>> > +#endif
>> > +}
>> > +
>> >  static void qmp_init_marshal(void)
>> >  {
>> >      qmp_register_command("query-qmp-schema", qmp_query_qmp_schema,
>> > @@ -1016,6 +1036,9 @@ static void qmp_init_marshal(void)
>> >                           QCO_NO_OPTIONS);
>> >      qmp_register_command("netdev_add", qmp_netdev_add,
>> >                           QCO_NO_OPTIONS);
>> > +
>> > +    /* call it after the rest of qapi_init() */
>> > +    register_module_init(qmp_disable_marshal, MODULE_INIT_QAPI);
>> >  }
>> >  
>> >  qapi_init(qmp_init_marshal);
>> 
>> Let's see whether I understand this patch...
>> 
>> qmp_disable_command() sets a flag that can be tested directly or with
>> qmp_command_is_enabled().  do_qmp_dispatch() tests it, and fails the
>> command with an "The command FOO has been disabled for this instance"
>> error.  It's called from qmp_dispatch(), which we're not yet using for
>> QMP at this point of the series.
>> 
>> QGA uses this to implement its "freeze whitelist", i.e. to disable all
>> commands that aren't known to be safe while filesystems are frozen.
>> 
>> qmp_disable_command() does nothing when the command doesn't exist, which
>> I think is the case at this point of the series.
>> 
>> So this patch does exactly nothing by itself.  When you later flip
>> dispatch to use qmp_dispatch(), it becomes active, and the error for
>> these commands changes from
>> 
>>     {"error": {"class": "CommandNotFound", "desc": "The command FOO has not
>>     been found"}}
>> 
>> to
>> 
>>     {"error": {"class": "GenericError", "desc": "The command FOO has been
>>     disabled for this instance"}}
>> 
>> which is fine with me.
>> 
>> Is this basically correct?
>
> I think it's correct, I haven't played much with this as I focused on the 
> conditional build instead, which is actually not so difficult, see:
>
> https://github.com/elmarco/qemu/commit/d0496fdabe49149e76d765d96feada0d5269c211
>  (qapi.py change)
>
> https://github.com/elmarco/qemu/commit/ee3c17b7bd5ffaa0b5be258a2cfb46c6c577f653
>  (per target build)
>
> https://github.com/elmarco/qemu/commit/91a692b3428878f8c26933c8a113853fb2a19970
>  (per target conditonal in schema)
>
> Do you think I should introduce that conditional build earlier in this series 
> instead of that disable workaround?

Doing the real thing right away is less churn than workaround first,
real thing later.  On the other hand, workarounds can help you may
shorten your patch queue.  Do what you think is best.  I'll review
whatever you throw at me :)