Re: [Qemu-devel] [PATCH 2/2] char: report frontend open/closed state in 'query-chardev'

[Laszlo Ersek]

On 05/29/14 22:12, Eric Blake wrote:
> On 05/29/2014 01:36 PM, Laszlo Ersek wrote:
>> In addition to the on-line reporting added in the previous patch, allow
>> libvirt to query frontend state independently of events.
>>
>> Libvirt's path to identify the guest agent channel it cares about differs
>> between the event added in the previous patch and the QMP response field
>> added here. The event identifies the frontend device, by "id". The
>> 'query-chardev' QMP command identifies the backend device (again by "id").
>> The association is under libvirt's control.
>>
>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1080376
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  qapi-schema.json |  8 +++++++-
>>  qemu-char.c      |  2 ++
>>  qmp-commands.hx  | 19 ++++++++++++++-----
>>  3 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 7bc33ea..7692f9f 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -476,12 +476,18 @@
>>  #
>>  # @filename: the filename of the character device
>>  #
>> +# @frontend_open: #optional shows whether the frontend device attached to 
>> this
> 
> s/frontend_open/frontend-open/ - as a new interface, we should be
> preferring '-' in the name.

Aaaargh, you're right of course. Shame I forgot that.

> 
>> +#                 backend (eg. with the chardev=... option) is in open or
>> +#                 closed state (since 2.2)
> 
> Why 2.2? Are you saying it is too late to make the 2.1 soft freeze?

I thought that reviewers would immediately question the direction of the
patchset (ie. monitor events + new query field), and not just suggest
tweaks; so 2.2 seemed safer. Perhaps I can make it till the 2.1 soft
freeze (June 17th), but that depends (as I've learned now) on Wenchao's
series too.

> Why optional?  It is always emitted by your code, so it's simpler to
> just state that it is now a permanent part of the output struct (no
> back-compat considerations, since it is an output-only struct).

I wasn't sure how libvirt would react to an earlier qemu's output struct
if its parser code expected the new field as mandatory. I didn't
consider that you could infer this field from the presence of the
events. So yeah I can make it mandatory.

> 
>> +#
>>  # Notes: @filename is encoded using the QEMU command line character device
>>  #        encoding.  See the QEMU man page for details.
>>  #
>>  # Since: 0.14.0
>>  ##
>> -{ 'type': 'ChardevInfo', 'data': {'label': 'str', 'filename': 'str'} }
>> +{ 'type': 'ChardevInfo', 'data': {'label': 'str',
>> +                                  'filename': 'str',
>> +                                  '*frontend_open': 'bool'} }
> 
> Thus, s/*frontend_open/frontend-open/

Yep.

> 
>>  
>>  ##
>>  # @query-chardev:
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 17b476e..021f86c 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -3439,6 +3439,8 @@ ChardevInfoList *qmp_query_chardev(Error **errp)
>>          info->value = g_malloc0(sizeof(*info->value));
>>          info->value->label = g_strdup(chr->label);
>>          info->value->filename = g_strdup(chr->filename);
>> +        info->value->has_frontend_open = true;
> 
> and drop this line
> 
>> +        info->value->frontend_open = chr->fe_open;
>>  
>>          info->next = chr_list;
>>          chr_list = info;
> 
> 

Thanks!
Laszlo