From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXQZU-0004sz-LJ for qemu-devel@nongnu.org; Wed, 10 Aug 2016 06:17:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXQZQ-0008E9-B3 for qemu-devel@nongnu.org; Wed, 10 Aug 2016 06:17:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52504) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXQZQ-0008Dl-2S for qemu-devel@nongnu.org; Wed, 10 Aug 2016 06:17:08 -0400 From: Markus Armbruster References: <20160808141439.16908-1-marcandre.lureau@redhat.com> <20160808141439.16908-13-marcandre.lureau@redhat.com> <877fbqglj6.fsf@dusky.pond.sub.org> <393082343.564767.1470747025500.JavaMail.zimbra@redhat.com> <87h9audnik.fsf@dusky.pond.sub.org> <1611724015.610129.1470753690860.JavaMail.zimbra@redhat.com> <87r39x9ac4.fsf@dusky.pond.sub.org> Date: Wed, 10 Aug 2016 12:17:04 +0200 In-Reply-To: (=?utf-8?Q?=22Marc-Andr=C3=A9?= Lureau"'s message of "Tue, 09 Aug 2016 19:35:52 +0000") Message-ID: <87vaz9x71b.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 v3 12/15] monitor: use qmp_dispatch() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: qemu-devel@nongnu.org Marc-Andr=C3=A9 Lureau writes: > Hi > > On Tue, Aug 9, 2016 at 9:35 PM Markus Armbruster wrot= e: > >> Marc-Andr=C3=A9 Lureau writes: >> >> > Hi >> > >> > ----- Original Message ----- >> >> Marc-Andr=C3=A9 Lureau writes: >> >> >> >> > Hi >> >> > >> >> > ----- Original Message ----- >> >> >> marcandre.lureau@redhat.com writes: >> >> >> >> >> >> > From: Marc-Andr=C3=A9 Lureau >> >> >> > >> >> >> > Replace the old manual dispatch and validation code by the gener= ic >> one >> >> >> > provided by qapi common code. >> >> >> > >> >> >> > Note that it is now possible to call the following commands that >> used to >> >> >> > be disabled by compile-time conditionals: >> >> >> > - dump-skeys >> >> >> > - query-spice >> >> >> > - rtc-reset-reinjection >> >> >> > - query-gic-capabilities >> >> >> > >> >> >> > Their fallback functions return an appropriate "feature disabled" >> error. >> >> >> > >> >> >> > Signed-off-by: Marc-Andr=C3=A9 Lureau >> >> >> >> >> >> Means query-qmp-schema no longer shows whether these commands are >> >> >> supported, doesn't it? >> >> >> >> >> >> Eric, could this create difficulties for libvirt or other >> introspection >> >> >> users? >> >> > >> >> > Thinking a bit about this, I guess it would be fairly straightforwa= rd >> >> > to have a new key "c-conditional" : "#ifdef CONFIG_SPICE" that would >> >> > prepend it in C generated files, with a corresponding "#endif". Wou= ld >> >> > that be acceptable? >> >> >> >> Not exactly pretty, but the only alternative I can think of right now >> >> would be conditional qapi generation, i.e. something like >> >> >> >> { 'if': 'CONFIG_SPICE' >> >> 'then': { 'command': 'query-spice', 'returns': 'SpiceInfo' } } >> >> >> >> More general, but *much* more work. Let's not go there now. >> > >> > That looks quite unnecessarily complicated to me, and not so declarati= ve. >> > >> >> >> >> The value of key 'c-conditional' must be a preprocessing directive th= at >> >> pairs with #endif. Hmm. >> >> >> >> Could make it an expression instead, and call the key just >> >> 'conditional'. If given, wrap the code generated for the QAPI >> >> definition in >> >> >> >> #if >> >> ... >> >> #endif >> >> >> > >> > Sure, we could make it a preprocessor expression instead, so it would >> have to match with the automatically appened "#endif". >> > >> >> Feels cleaner, but to avoid -Wundef warnings, we'd have to say >> >> 'defined(CONFIG_SPICE)'. >> > >> > Yes, why not? I can work on a patch see how well it fits. >> >> Yes, please. >> > > After spending some time on this (the generator part is trivial), I think > it's not going to be that easy because the conditions are per-target, but > qmp is not, so you get poisoned defines errors with the TARGET conditions. Ah, yes. monitor.c is target-specific for similarly annoying reasons. Factoring out the parts that are actually target-specific into a separate file would be nice. > We can't easily make qmp per target, as the code is spread everywhere (ev= en What you mean by "qmp" and ... > though such qmp code won't be useful for other tools, it would be nice to > untangle this - the block code is full of qmp usage and implementation) ... "qmp usage and implementation"? > Furthermore, the current query-qmp-schema returns all commands currently, > so this won't be a regression. It returns qmp_schema_json[], generated by qapi-introspect.py. > I imagine 3 ways to solve this: > - make qmp code per-target (seems to be pretty intrusive change all over, > although I think it's nicer. As a simple ex, calling qmp_query_uuid() just > to get an uuid doesn't make much sense, it doesn't have to go through qmp) I'm not sure I understand this proposed solution. qmp_query_uuid() doesn't "go through QMP" in any sense I'd use. The (QAPIfied) QMP command handlers are intended to be "natural" C interfaces. Their only QMPish part is their use of QAPI types. For instance, qmp_query_uuid() is almost the obvious C function to convert the binary qemu_uuid to a string. "Almost", because the string is wrapped in a struct, probably due to the rule "don't return simple types, because that's not extensible". If you now pointed out that the chance we'll extend UUIDs is remote, you'd have a point. But what's done is done. If the struct wrapping gets bothersome, we can factor out the UUID formatting into its own function. > - unregister functions dynamically? (that could be useful for other reaso= ns) > - make only qmp_init_marshal() and qmp_introspect() per target. > > That last option seems the easier. What do you think? Let me try to elaborate the last option to make sure we're on the same page. Key 'conditional' makes the QAPI generators generate preprocessing directives for conditional compilation. Only commands have this key, at least for now. The preprocessing directives should wrap "everything" belonging to the command: * Command handler declaration in qmp-commands.h Problem: this header gets included widely, and we really don't want to make all the .c files including it target-dependent. Howeve, since a declaration without an implementation is not an error, we can blindly generate the declarations for now, along with a TODO comment explaining the uncleanliness. * Command marshaller declaration in qmp-commands.h Likewise. * Command marshaller definition in qmp-marshal.c Can make target-dependent. It's a big file, though. Perhaps we can split it into a target-dependent and a target-independent part some day. * Command marshaller use in qmp-marshal.c's qmp_init_marshal() Likewise. * Command introspection data in qmp-introspect.c. Can make target-dependent. Splitting it up doesn't look worth the bother. Is this basically what you're proposing?