From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZfSJ-0008MK-Dz for qemu-devel@nongnu.org; Tue, 16 Aug 2016 10:35:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bZfSF-0007pS-7Z for qemu-devel@nongnu.org; Tue, 16 Aug 2016 10:35:02 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:35054) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZfSE-0007pN-V8 for qemu-devel@nongnu.org; Tue, 16 Aug 2016 10:34:59 -0400 Date: Tue, 16 Aug 2016 10:34:56 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1604394942.66237.1471358096460.JavaMail.zimbra@redhat.com> In-Reply-To: <8737m44wod.fsf@dusky.pond.sub.org> References: <20160810180235.32469-1-marcandre.lureau@redhat.com> <20160810180235.32469-7-marcandre.lureau@redhat.com> <8737m44wod.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 06/17] monitor: unregister conditional commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre lureau , qemu-devel@nongnu.org Hi ----- Original Message ----- > marcandre.lureau@redhat.com writes: >=20 > > From: Marc-Andr=C3=A9 Lureau > > > > 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=C3=A9 Lureau > > --- > > 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 =3D qobject_from_json(qmp_schema_json); > > } > > =20 > > +/* > > + * 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); > > } > > =20 > > qapi_init(qmp_init_marshal); >=20 > Let's see whether I understand this patch... >=20 > 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. >=20 > 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. >=20 > qmp_disable_command() does nothing when the command doesn't exist, which > I think is the case at this point of the series. >=20 > 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 >=20 > {"error": {"class": "CommandNotFound", "desc": "The command FOO has n= ot > been found"}} >=20 > to >=20 > {"error": {"class": "GenericError", "desc": "The command FOO has been > disabled for this instance"}} >=20 > which is fine with me. >=20 > Is this basically correct? I think it's correct, I haven't played much with this as I focused on the c= onditional build instead, which is actually not so difficult, see: https://github.com/elmarco/qemu/commit/d0496fdabe49149e76d765d96feada0d5269= c211 (qapi.py change) https://github.com/elmarco/qemu/commit/ee3c17b7bd5ffaa0b5be258a2cfb46c6c577= f653 (per target build) https://github.com/elmarco/qemu/commit/91a692b3428878f8c26933c8a113853fb2a1= 9970 (per target conditonal in schema) Do you think I should introduce that conditional build earlier in this seri= es instead of that disable workaround?