Marc-André Lureau <mlur...@redhat.com> writes: > Hi > > ----- Original Message ----- >> marcandre.lur...@redhat.com writes: >> >> > From: Marc-André Lureau <marcandre.lur...@redhat.com> >> > >> > 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 <marcandre.lur...@redhat.com> >> > --- >> > 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.
Unfortunately, it looks like it's not fine with libvirt. qemuMonitorJSONGetGICCapabilities() in src/qemu/qemu_monitor_json.c: int ret = -1; [...] /* If the 'query-gic-capabilities' QMP command was not available * we simply successfully return zero capabilities. * This is the case for QEMU <2.6 and all non-ARM architectures */ if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { ret = 0; goto cleanup; } if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; Ideally, libvirt would use query-commands instead of ErrorClass CommandNotFound. But we need to play the hand we've been dealt. That means preserving ErrorClass CommandNotFound. >> 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: [...]