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

[Laszlo Ersek]

On 05/29/14 22:05, Eric Blake wrote:
> 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?

Emitting the events unconditionally shouldn't hurt anything, I think. I
added the property for two reasons:

- I was sure someone would ask for it. :)

- The property is device-level. The example I gave in the blurb uses
-global with qemu:arg for simplicity, but the intent is that libvirt set
it only for the one (or few) virtio-serial port(s) where it actually
intends to communicate with the guest agent(s). Libvirt can set up
several virtio-serial ports (to be read & written by other programs on
the host side via their respective chardevs (*)), and if libvirt doesn't
connect to those, then qemu shouldn't spam it with uninteresting events.
(If libvirt does set the property for more than one virtio-serial port,
then it can distinguish the ports by the ids carried in the events.)

(*) In fact I regularly use this feature: it lets me hack on qga and
test it from the host side (with "socat" or otherwise), without libvirt
"interfering" with the traffic, but nonetheless setting up the unix
domain socket for me, which is nice.

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

I did notice some throttling code in the event emission code... Let me see.

set_guest_connected() [hw/char/virtio-console.c]
  monitor_protocol_event() [monitor.c]
    monitor_protocol_event_queue()

Ah okay. So rate limiting is indeed an attribute of the event. I didn't
know that. Apparently, the rates are configured statically, in
monitor_protocol_event_init() [monitor.c]. Any idea what limit I should set?

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

If that series is converging, I can rebase (or simply postpone), sure.
I'll be on PTO next week anyway :)

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

That's a valid remark entirely, and I did contemplate the question, but
other events seem to exist for CONNECT and DISCONNECT cases separately,
already:

- SPICE_CONNECTED, SPICE_DISCONNECTED -- even documented together
- VNC_CONNECTED, VNC_DISCONNECTED -- here too the accompanying data
  structure seems to be identical between connect & disconnect

I just wanted to follow that pattern, but I don't insist.

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

Agreed.

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

As I said, it's not a global switch; it's a per-device property that
could help cut event spam. I used it with the -global option in my
example only because that was easiest with <qemu:arg>. (I couldn't set
the property just for one or two virtio-serial ports in the libvirt XML.)

Anyway, it's of course easier to drop the property, so if you think that
controlling the event on the level of the individual virtio-serial port
is overkill, I can remove it.

Thanks!
Laszlo