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

[Eric Blake]

On 05/29/2014 01:36 PM, 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)).
> Introduce a new property that allows libvirt to request connection state
> reporting, and report the state via new monitor events.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1080376
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>  include/monitor/monitor.h |  2 ++
>  hw/char/virtio-console.c  | 20 +++++++++++++++++---
>  monitor.c                 |  2 ++
>  docs/qmp/qmp-events.txt   | 34 ++++++++++++++++++++++++++++++++++
>  4 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 1c1f56f..4fcb5b4 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -51,6 +51,8 @@ typedef enum MonitorEvent {
>      QEVENT_BLOCK_IMAGE_CORRUPTED,
>      QEVENT_QUORUM_FAILURE,
>      QEVENT_QUORUM_REPORT_BAD,
> +    QEVENT_VSERPORT_CONNECTED,
> +    QEVENT_VSERPORT_DISCONNECTED,

Yay - it's discoverable.  Libvirt already queries the set of events that
qemu announces it can send, so the existence of these events is a
witness that the new connection property exists and should be enabled.

Would it hurt anything to unconditionally emit the events, rather than
requiring a configuration option to turn them on?

Also, since these events are triggered in relation to guest activity, I
think they need to be rate-limited (a guest that repeatedly opens and
closes the device as fast as possible should not cause an event storm).


> +    if (vcon->report_connstate && dev->id) {
> +        QObject *data;
> +
> +        data = qobject_from_jsonf("{ 'id': %s }", dev->id);
> +        monitor_protocol_event(guest_connected ? QEVENT_VSERPORT_CONNECTED :
> +                                                 
> QEVENT_VSERPORT_DISCONNECTED,
> +                               data);

I wish Wenchao's series on converting events into full-blown QAPI
citizens would hurry up - one of these series will have to rebase on top
of the other.

https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00000.html


> +++ b/docs/qmp/qmp-events.txt
> @@ -509,6 +509,40 @@ Example:
>                      "host": "127.0.0.1", "sasl_username": "luiz" } },
>          "timestamp": { "seconds": 1263475302, "microseconds": 150772 } }
>  
> +VSERPORT_CONNECTED
> +------------------

Do we _really_ need two separate events?  Why not just one event
VSERPORT_CHANGE, along with an additional data member 'open':'bool' that
is true when the guest opened the port, and false when the guest closed
the port?

1 vs. 2 events is bike-shedding, so I can live with your proposal.  But
missing rate-limiting is worth either a v2 or a followup patch.  And
unless there is a compelling reason to require a configuration variable
to turn the event on or off, I think it would be simpler to just make
the event unconditional.

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

signature.asc
Description: OpenPGP digital signature