[Qemu-devel] [PATCH for-2.1 v2 0/2] help libvirt know what's up with qga

[Qemu-devel] [PATCH for-2.1 v2 0/2] help libvirt know what's up with qga

[Laszlo Ersek]

Version 2 of the series originally posted at
<http://thread.gmane.org/gmane.comp.emulators.qemu/276342>, rebased to
Wenchao Xia's qapi-event feature, and addressing Eric's review notes for
v1. Changes are broken out per patch.

The series builds at each patch. I repeated the tests described in the
v1 blurb. Please review.

Thanks,
Laszlo

Laszlo Ersek (2):
  virtio-serial: report frontend connection state via monitor
  char: report frontend open/closed state in 'query-chardev'

 qapi-event.json          | 16 ++++++++++++++++
 qapi-schema.json         | 24 +++++++++++++++++++++++-
 hw/char/virtio-console.c | 15 ++++++++++++---
 monitor.c                |  1 +
 qemu-char.c              |  1 +
 qmp-commands.hx          | 19 ++++++++++++++-----
 6 files changed, 67 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.1 v2 1/2] virtio-serial: report frontend connection state via monitor

[Laszlo Ersek]

Libvirt wants to know about the guest-side connection state of some
virtio-serial ports (in particular the one(s) assigned to guest agent(s)).
Report such states with a new monitor event.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1080376
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    - emit event unconditionally; drop per-port "report_connstate" property
      [Eric]
    - rate-limit the new event [Eric] -- unify it with other
      guest-triggerable events in monitor_qapi_event_init()
    - rebased to Wenchao's qapi-event series
    - introduce one event only, VSERPORT_CHANGE, and make the port status a
      parameter [Eric]

 qapi-event.json          | 16 ++++++++++++++++
 qapi-schema.json         | 16 ++++++++++++++++
 hw/char/virtio-console.c | 15 ++++++++++++---
 monitor.c                |  1 +
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/qapi-event.json b/qapi-event.json
index e7a47f9..0ec20c4 100644
--- a/qapi-event.json
+++ b/qapi-event.json
@@ -315,4 +315,20 @@
 ##
 { 'event': 'QUORUM_REPORT_BAD',
   'data': { '*error': 'str', 'node-name': 'str',
             'sector-num': 'int', 'sector-count': 'int' } }
+
+##
+# @VSERPORT_CHANGE
+#
+# Emitted when the guest takes an action that changes the status of a
+# virtio-serial port (eg. opens or closes it).
+#
+# @id: device identifier of the virtio-serial port
+#
+# @status: new status of the virtio-serial port
+#
+# Since: 2.1
+##
+{ 'event': 'VSERPORT_CHANGE',
+  'data': { 'id': 'str',
+            'status': 'VirtioSerialPortStatus' } }
diff --git a/qapi-schema.json b/qapi-schema.json
index e7727a1..d1dc8be 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3398,5 +3398,21 @@
 ##
 { 'enum': 'GuestPanicAction',
   'data': [ 'pause' ] }
 
+##
+# @VirtioSerialPortStatus
+#
+# An enumeration of the possible / recognized states of virtio-serial ports.
+#
+# @connected: the virtio-serial port has been opened (connected to) in the
+#              guest.
+#
+# @disconnected: the virtio-serial port has been closed (disconnected from) in
+#                the guest.
+#
+# Since: 2.1
+##
+{ 'enum': 'VirtioSerialPortStatus',
+  'data': [ 'connected', 'disconnected' ] }
+
 { 'include': 'qapi-event.json' }
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 6c8be0f..729faac 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -13,8 +13,9 @@
 #include "sysemu/char.h"
 #include "qemu/error-report.h"
 #include "trace.h"
 #include "hw/virtio/virtio-serial.h"
+#include "qapi-event.h"
 
 #define TYPE_VIRTIO_CONSOLE_SERIAL_PORT "virtserialport"
 #define VIRTIO_CONSOLE(obj) \
     OBJECT_CHECK(VirtConsole, (obj), TYPE_VIRTIO_CONSOLE_SERIAL_PORT)
@@ -80,13 +81,21 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
 /* Callback function that's called when the guest opens/closes the port */
 static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
 {
     VirtConsole *vcon = VIRTIO_CONSOLE(port);
+    DeviceState *dev = DEVICE(port);
 
-    if (!vcon->chr) {
-        return;
+    if (vcon->chr) {
+        qemu_chr_fe_set_open(vcon->chr, guest_connected);
+    }
+
+    if (dev->id) {
+        VirtioSerialPortStatus status = guest_connected ?
+            VIRTIO_SERIAL_PORT_STATUS_CONNECTED :
+            VIRTIO_SERIAL_PORT_STATUS_DISCONNECTED;
+
+        qapi_event_send_vserport_change(dev->id, status, &error_abort);
     }
-    qemu_chr_fe_set_open(vcon->chr, guest_connected);
 }
 
 /* Readiness of the guest to accept data on a port */
 static int chr_can_read(void *opaque)
diff --git a/monitor.c b/monitor.c
index a8ab600..5fbf987 100644
--- a/monitor.c
+++ b/monitor.c
@@ -584,8 +584,9 @@ static void monitor_qapi_event_init(void)
     /* Limit guest-triggerable events to 1 per second */
     monitor_qapi_event_throttle(QAPI_EVENT_RTC_CHANGE, 1000);
     monitor_qapi_event_throttle(QAPI_EVENT_WATCHDOG, 1000);
     monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
+    monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
     /* limit the rate of quorum events to avoid hammering the management */
     monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
     monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
 
-- 
1.8.3.1

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

[Laszlo Ersek]

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 <lersek@redhat.com>
---

Notes:
    v2:
    - rename "frontend_open" to "frontend-open" in the schema [Eric]
    - "frontend-open" is mandatory [Eric]
    - advertise / target qemu-2.1 with the new field

 qapi-schema.json |  8 +++++++-
 qemu-char.c      |  1 +
 qmp-commands.hx  | 19 ++++++++++++++-----
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index d1dc8be..4f968c6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -210,14 +210,20 @@
 # @label: the label of the character device
 #
 # @filename: the filename of the character device
 #
+# @frontend-open: shows whether the frontend device attached to this backend
+#                 (eg. with the chardev=... option) is in open or closed state
+#                 (since 2.1)
+#
 # 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'} }
 
 ##
 # @query-chardev:
 #
diff --git a/qemu-char.c b/qemu-char.c
index 2e50a10..98fb0ab 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3703,8 +3703,9 @@ ChardevInfoList *qmp_query_chardev(Error **errp)
         ChardevInfoList *info = g_malloc0(sizeof(*info));
         info->value = g_malloc0(sizeof(*info->value));
         info->value->label = g_strdup(chr->label);
         info->value->filename = g_strdup(chr->filename);
+        info->value->frontend_open = chr->fe_open;
 
         info->next = chr_list;
         chr_list = info;
     }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e4a1c80..35f5146 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1920,21 +1920,30 @@ of all devices.
 Each json-object contain the following:
 
 - "label": device's label (json-string)
 - "filename": device's file (json-string)
+- "frontend-open": open/closed state of the frontend device attached to this
+                   backend (json-bool)
 
 Example:
 
 -> { "execute": "query-chardev" }
 <- {
-      "return":[
+      "return": [
          {
-            "label":"monitor",
-            "filename":"stdio"
+            "label": "charchannel0",
+            "filename": "unix:/var/lib/libvirt/qemu/seabios.rhel6.agent,server",
+            "frontend-open": false
          },
          {
-            "label":"serial0",
-            "filename":"vc"
+            "label": "charmonitor",
+            "filename": "unix:/var/lib/libvirt/qemu/seabios.rhel6.monitor,server",
+            "frontend-open": true
+         },
+         {
+            "label": "charserial0",
+            "filename": "pty:/dev/pts/2",
+            "frontend-open": true
          }
       ]
    }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.1 v2 0/2] help libvirt know what's up with qga

[Amit Shah]

On (Thu) 26 Jun 2014 [13:11:13], Laszlo Ersek wrote:
> Version 2 of the series originally posted at
> <http://thread.gmane.org/gmane.comp.emulators.qemu/276342>, rebased to
> Wenchao Xia's qapi-event feature, and addressing Eric's review notes for
> v1. Changes are broken out per patch.
> 
> The series builds at each patch. I repeated the tests described in the
> v1 blurb. Please review.
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (2):
>   virtio-serial: report frontend connection state via monitor
>   char: report frontend open/closed state in 'query-chardev'

Reviewed-by: Amit Shah <amit.shah@redhat.com>


		Amit

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On 06/26/2014 05:11 AM, 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 <lersek@redhat.com>
> ---
> 

>  ##
> -{ 'type': 'ChardevInfo', 'data': {'label': 'str', 'filename': 'str'} }
> +{ 'type': 'ChardevInfo', 'data': {'label': 'str',
> +                                  'filename': 'str',
> +                                  'frontend-open': 'bool'} }

Hmm; I wonder if this should instead be
'frontend-status':'VirtioSerialPortStatus', to reuse the type from patch
1/2.  That way, if we ever add a third state, then both the event and
the poll will be reusing the same enum values to report that state.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.1 v2 1/2] virtio-serial: report frontend connection state via monitor

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

On 06/26/2014 05:11 AM, Laszlo Ersek wrote:
> Libvirt wants to know about the guest-side connection state of some
> virtio-serial ports (in particular the one(s) assigned to guest agent(s)).
> Report such states with a new monitor event.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1080376
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 

> +++ b/monitor.c
> @@ -584,8 +584,9 @@ static void monitor_qapi_event_init(void)
>      /* Limit guest-triggerable events to 1 per second */
>      monitor_qapi_event_throttle(QAPI_EVENT_RTC_CHANGE, 1000);
>      monitor_qapi_event_throttle(QAPI_EVENT_WATCHDOG, 1000);
>      monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
> +    monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
>      /* limit the rate of quorum events to avoid hammering the management */
>      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);

Trivial merge conflict with Wenchao's pending patch:
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg05897.html
but the resolution is obvious.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Laszlo Ersek]

On 06/26/14 14:09, Eric Blake wrote:
> On 06/26/2014 05:11 AM, 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 <lersek@redhat.com>
>> ---
>>
> 
>>  ##
>> -{ 'type': 'ChardevInfo', 'data': {'label': 'str', 'filename': 'str'} }
>> +{ 'type': 'ChardevInfo', 'data': {'label': 'str',
>> +                                  'filename': 'str',
>> +                                  'frontend-open': 'bool'} }
> 
> Hmm; I wonder if this should instead be
> 'frontend-status':'VirtioSerialPortStatus', to reuse the type from patch
> 1/2.  That way, if we ever add a third state, then both the event and
> the poll will be reusing the same enum values to report that state.

I expected this remark :)

The difference is rooted in the fact that the event approaches the
virtio-serial port status from the frontend (ie. guest) side, while the
query approaches the same from the backend (ie. host, chardev) side.

If I wanted to bring those in future-proof sync, then I would have to
change the underlying, generic chardev machinery -- namely, the fe_open
field, and everything that operates on it.

I actually considered the other direction too: rather than introducing
status:VirtioSerialPortStatus, just add open:bool (which was your
original suggestion in your v1 review). I decided against it because the
current list of enum constants (connected, disconnected) expresses just
the same, and it'll be a *tiny* bit easier to extend, should that
necessity arise.

Sounds acceptible? :) If not, then I'm OK to replace
status:VirtioSerialPortStatus with open:bool in the first patch.

Thanks,
Laszlo

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

On 06/26/2014 07:38 AM, Laszlo Ersek wrote:

>>>  ##
>>> -{ 'type': 'ChardevInfo', 'data': {'label': 'str', 'filename': 'str'} }
>>> +{ 'type': 'ChardevInfo', 'data': {'label': 'str',
>>> +                                  'filename': 'str',
>>> +                                  'frontend-open': 'bool'} }
>>
>> Hmm; I wonder if this should instead be
>> 'frontend-status':'VirtioSerialPortStatus', to reuse the type from patch
>> 1/2.  That way, if we ever add a third state, then both the event and
>> the poll will be reusing the same enum values to report that state.
> 
> I expected this remark :)
> 
> The difference is rooted in the fact that the event approaches the
> virtio-serial port status from the frontend (ie. guest) side, while the
> query approaches the same from the backend (ie. host, chardev) side.
> 
> If I wanted to bring those in future-proof sync, then I would have to
> change the underlying, generic chardev machinery -- namely, the fe_open
> field, and everything that operates on it.
> 
> I actually considered the other direction too: rather than introducing
> status:VirtioSerialPortStatus, just add open:bool (which was your
> original suggestion in your v1 review). I decided against it because the
> current list of enum constants (connected, disconnected) expresses just
> the same, and it'll be a *tiny* bit easier to extend, should that
> necessity arise.
> 
> Sounds acceptible? :) If not, then I'm OK to replace
> status:VirtioSerialPortStatus with open:bool in the first patch.

If we ever add another state in the future, then both places would be
touched at the same time to figure out how to support that new state.
So I agree with your counter-proposal of just simplifying things to use
open:bool in the first patch; it also has the benefit of less code (no
need to add an enum).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]