[v3] qmp: expose list of supported character device backends

Introduce 'query-chardev-backends' QMP command which lists all
supported character device backends.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
v3:
 - Omit commas at the end of list in JSON
v2:
 - Version changed from "1.8.0" to "2.0"

 qapi-schema.json | 22 ++++++++++++++++++++++
 qemu-char.c      | 19 +++++++++++++++++++
 qmp-commands.hx  | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

--
1.8.5.3

On 02/01/2014 04:52 AM, Martin Kletzander wrote:
> Introduce 'query-chardev-backends' QMP command which lists all
> supported character device backends.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> v3:
>  - Omit commas at the end of list in JSON
> v2:
>  - Version changed from "1.8.0" to "2.0"
> 
>  qapi-schema.json | 22 ++++++++++++++++++++++
>  qemu-char.c      | 19 +++++++++++++++++++
>  qmp-commands.hx  | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

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>
> ---
> v3:
>  - Omit commas at the end of list in JSON
> v2:
>  - Version changed from "1.8.0" to "2.0"
> 
>  qapi-schema.json | 22 ++++++++++++++++++++++
>  qemu-char.c      | 19 +++++++++++++++++++
>  qmp-commands.hx  | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 05ced9d..ebd278a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -437,6 +437,28 @@
>  { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
> 
>  ##
> +# @ChardevBackendInfo:
> +#
> +# Information about a character device backend
> +#
> +# @name: The backend name
> +#
> +# Since: 2.0
> +##
> +{ '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'?

> +
> +##
> +# @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.

> +#
> +# Returns: a list of @ChardevBackendInfo
> +#
> +# Since: 2.0
> +##
> +{ 'command': 'query-chardev-backends', 'returns': ['ChardevBackendInfo'] }
> +
> +##
>  # @DataFormat:
>  #
>  # An enumeration of data format.
> diff --git a/qemu-char.c b/qemu-char.c
> index 30c5a6a..c88f1c4 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3432,6 +3432,25 @@ ChardevInfoList *qmp_query_chardev(Error **errp)
>      return chr_list;
>  }
> 
> +ChardevBackendInfoList *qmp_query_chardev_backends(Error **errp)
> +{
> +    ChardevBackendInfoList *backend_list = NULL;
> +    CharDriver *c = NULL;
> +    GSList *i = NULL;
> +
> +    for (i = backends; i; i = i->next) {
> +        ChardevBackendInfoList *info = g_malloc0(sizeof(*info));
> +        c = i->data;
> +        info->value = g_malloc0(sizeof(*info->value));
> +        info->value->name = g_strdup(c->name);
> +
> +        info->next = backend_list;
> +        backend_list = info;
> +    }
> +
> +    return backend_list;
> +}
> +
>  CharDriverState *qemu_chr_find(const char *name)
>  {
>      CharDriverState *chr;
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index cce6b81..8a0e832 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1924,6 +1924,47 @@ EQMP
>      },
> 
>  SQMP
> +query-chardev-backends
> +-------------
> +
> +List available character device backends.
> +
> +Each backend is represented by a json-object, the returned value is a json-array
> +of all backends.
> +
> +Each json-object contains:
> +
> +- "name": backend name (json-string)
> +
> +Example:
> +
> +-> { "execute": "query-chardev-backends" }
> +<- {
> +      "return":[
> +         {
> +            "name":"udp"
> +         },
> +         {
> +            "name":"tcp"
> +         },
> +         {
> +            "name":"unix"
> +         },
> +         {
> +            "name":"spiceport"
> +         }
> +      ]
> +   }
> +
> +EQMP
> +
> +    {
> +        .name       = "query-chardev-backends",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_chardev_backends,
> +    },
> +
> +SQMP
>  query-block
>  -----------
> 
> --
> 1.8.5.3
>

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).

[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).

Eric Blake <eblake@redhat.com> writes:

> 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.

Yes, and Wenchao Xia has been working towards that: "[PATCH V5 00/10]
qapi script: support enum as discriminator and better enum name".

> 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).
>
> [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?

By "those", do you mean Wenchao Xia's patches?

[...]

On 02/11/2014 01:32 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:

>>
>> 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.
> 
> Yes, and Wenchao Xia has been working towards that: "[PATCH V5 00/10]
> qapi script: support enum as discriminator and better enum name".
> 

>> [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?
> 
> By "those", do you mean Wenchao Xia's patches?

Maybe my [1] to tie together two widely separated paragraphs wasn't
obvious, but yes, I meant Wenchao's tests.  Plus they just got reposted:
https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg01706.html

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).
>

On 02/11/2014 08:51 AM, Luiz Capitulino wrote:

>>>> +##
>>>> +{ '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'?
>>

>> 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?

Yes, changing 'str' to an enum type should not cause any change to the
actual QMP wire format, so it is an improvement that could be made
later.  I see nothing preventing us from taking Martin's patch as
currently written, even if we do add later patches to make it more typesafe.

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>

Applied to the qmp branch, thanks.

> ---
> v3:
>  - Omit commas at the end of list in JSON
> v2:
>  - Version changed from "1.8.0" to "2.0"
> 
>  qapi-schema.json | 22 ++++++++++++++++++++++
>  qemu-char.c      | 19 +++++++++++++++++++
>  qmp-commands.hx  | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 05ced9d..ebd278a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -437,6 +437,28 @@
>  { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
> 
>  ##
> +# @ChardevBackendInfo:
> +#
> +# Information about a character device backend
> +#
> +# @name: The backend name
> +#
> +# Since: 2.0
> +##
> +{ 'type': 'ChardevBackendInfo', 'data': {'name': 'str'} }
> +
> +##
> +# @query-chardev-backends:
> +#
> +# Returns information about character device backends.
> +#
> +# Returns: a list of @ChardevBackendInfo
> +#
> +# Since: 2.0
> +##
> +{ 'command': 'query-chardev-backends', 'returns': ['ChardevBackendInfo'] }
> +
> +##
>  # @DataFormat:
>  #
>  # An enumeration of data format.
> diff --git a/qemu-char.c b/qemu-char.c
> index 30c5a6a..c88f1c4 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3432,6 +3432,25 @@ ChardevInfoList *qmp_query_chardev(Error **errp)
>      return chr_list;
>  }
> 
> +ChardevBackendInfoList *qmp_query_chardev_backends(Error **errp)
> +{
> +    ChardevBackendInfoList *backend_list = NULL;
> +    CharDriver *c = NULL;
> +    GSList *i = NULL;
> +
> +    for (i = backends; i; i = i->next) {
> +        ChardevBackendInfoList *info = g_malloc0(sizeof(*info));
> +        c = i->data;
> +        info->value = g_malloc0(sizeof(*info->value));
> +        info->value->name = g_strdup(c->name);
> +
> +        info->next = backend_list;
> +        backend_list = info;
> +    }
> +
> +    return backend_list;
> +}
> +
>  CharDriverState *qemu_chr_find(const char *name)
>  {
>      CharDriverState *chr;
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index cce6b81..8a0e832 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1924,6 +1924,47 @@ EQMP
>      },
> 
>  SQMP
> +query-chardev-backends
> +-------------
> +
> +List available character device backends.
> +
> +Each backend is represented by a json-object, the returned value is a json-array
> +of all backends.
> +
> +Each json-object contains:
> +
> +- "name": backend name (json-string)
> +
> +Example:
> +
> +-> { "execute": "query-chardev-backends" }
> +<- {
> +      "return":[
> +         {
> +            "name":"udp"
> +         },
> +         {
> +            "name":"tcp"
> +         },
> +         {
> +            "name":"unix"
> +         },
> +         {
> +            "name":"spiceport"
> +         }
> +      ]
> +   }
> +
> +EQMP
> +
> +    {
> +        .name       = "query-chardev-backends",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_chardev_backends,
> +    },
> +
> +SQMP
>  query-block
>  -----------
> 
> --
> 1.8.5.3
>