Re: [Qemu-devel] [PATCH v3] qmp: expose list of supported character device backends

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 10 Feb 2014 14:36:20 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 02/10/2014 02:16 PM, Luiz Capitulino wrote:
> > On Sat,  1 Feb 2014 12:52:42 +0100
> > Martin Kletzander <mkletzan@redhat.com> wrote:
> > 
> >> Introduce 'query-chardev-backends' QMP command which lists all
> >> supported character device backends.
> >>
> >> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> >> ---
> 
> >> +##
> >> +{ '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).
>