From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52547) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDFcP-0005hT-Ry for qemu-devel@nongnu.org; Tue, 11 Feb 2014 10:51:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WDFcJ-0000rI-TX for qemu-devel@nongnu.org; Tue, 11 Feb 2014 10:51:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:22984) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDFcJ-0000r3-L1 for qemu-devel@nongnu.org; Tue, 11 Feb 2014 10:51:23 -0500 Date: Tue, 11 Feb 2014 10:51:14 -0500 From: Luiz Capitulino Message-ID: <20140211105114.1912c198@redhat.com> In-Reply-To: <52F94654.8080401@redhat.com> References: <20140210161600.4f9b34a1@redhat.com> <52F94654.8080401@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] qmp: expose list of supported character device backends List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Martin Kletzander , qemu-devel@nongnu.org, Anthony Liguori , Markus Armbruster On Mon, 10 Feb 2014 14:36:20 -0700 Eric Blake wrote: > On 02/10/2014 02:16 PM, Luiz Capitulino wrote: > > On Sat, 1 Feb 2014 12:52:42 +0100 > > Martin Kletzander wrote: > > > >> Introduce 'query-chardev-backends' QMP command which lists all > >> supported character device backends. > >> > >> Signed-off-by: Martin Kletzander > >> --- > > >> +## > >> +{ 'type': 'ChardevBackendInfo', 'data': {'name': 'str'} } > > > > We already have ChardevBackend, it's an union though. I'm wondering if > > you could change it to an enum and use it instead of plain 'str'? > > Hmm, right now, the ChardevBackend union pre-dates when we added flat > unions. For flat unions, we can set a discriminator to be an enum type > [1], at which point the code generator then validates that we cover all > values of the enum in branches of the union; maybe it's worth > retro-fitting simple unions to also take advantage of the additional > coverage of the discriminator being an enum. > > That is, right now, we have: > > { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile', > 'serial' : 'ChardevHostdev', > > and we also document in qapi-code-gen.txt that when using > 'discriminator', you either have to have a base class (and the > discriminator is a string-typed member of that base class), or the > discriminator is {} because it is an anonymous union. But I'm asking > about yet another situation, of having a typed discriminator with no > change to the wire format (no base class), something like: > > { 'enum': 'ChardevBackendTypes', [ 'file', 'serial', ... ] } > { 'union': 'ChardevBackend', > 'discriminator': 'ChardevBackendTypes', > 'data': { 'file': 'ChardevFile', ... > > The benefit of such a plan is that we then have an introspectible enum > of all possible backends known at compile time (ChardevBackendTypes), > and the new addition in this patch becomes: > > { 'type': 'ChardevBackendInfo', > 'data': {'name', 'ChardevBackendTypes' } } > > rather than raw 'str', while still allowing potential future additions > of additional backend info. Note that there would still be a difference > between ChardevBackendTypes (an enum of all possible known types at > compile time) vs. query-chardev-backends (a runtime list of the possible > types that can be used for this particular machine, even if it is a > subset of all possible ChardevBackendTypes). Do you envision whether, by applying Martin's patch now, it would be compatible to do what you suggest on top? > > [1] actually, did those patches ever get applied, and we just missed > documenting it in qapi-code-gen.txt, or are they still pending review? > > > > >> + > >> +## > >> +# @query-chardev-backends: > >> +# > >> +# Returns information about character device backends. > > > > Actually, it returns information about registered backends (registration > > is done by register_char_driver_qapi(). So, I think it's good thing to > > mention that this list is about available backends. > > Again, it sounds like you want to emphasize an enum of all possible > types (compile time, learned via introspection, whenever we get that) > vs. registered backends (possibly a subset based on what libraries were > used in building this qemu binary, and learned via the new QMP command). >