On (Tue) 11 Aug 2015 [20:21:18], Laszlo Ersek wrote: > On 08/11/15 19:04, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > > > QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port > > state. However, the events may be for different ports, but the throttle > > mechanism may replace the event for a different port, since it only > > checks the event type. > > > > libvirt relies on a correct state to be reported for all channels: the > > qemu-ga commands may no longer work if the state is reported > > disconnected. This can be triggered easily by having more than 1 > > virtio-serial (qemu-ga + spice agent for example), and restarting > > quickly daemons or more realistically going quickly in and out of > > suspend. > > > > In a future patch, we may want to throttle events based on their > > arguments, but this will likely require dynamic allocations and more > > complicated code to insert/lookup pending events based on various > > arguments ("id" in QAPI_EVENT_VSERPORT_CHANGE case). > > > > Fixes: > > https://bugzilla.redhat.com/show_bug.cgi?id=1244064 > > > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > --- > > monitor.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/monitor.c b/monitor.c > > index aeea2b5..e4d56f7 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -558,7 +558,6 @@ static void monitor_qapi_event_init(void) > > monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000); > > monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000); > > monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000); > > - monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000); > > > > qmp_event_set_func_emit(monitor_qapi_event_queue); > > } > > > > I don't mind the change (and the point of argument sensitivity is not > lost on me), but note that this undoes the protection that is spelled > out in the leading comment of the function, not visible in the context here: > > /* Limit guest-triggerable events to 1 per second */ > > That was probably put in place in order to prevent a "malicious" guest > from spamming the log files (and the CPU usage) of the management apps. > > One solution to that would be arg sensitivity. Another would be a > burst-capable (ie. a slowly re-filling, limited size token bucket) > throttle, maintained per event type. > > Until one of those gets written, I guess this patch is acceptable -- as > long as mgmt people are okay with it. Daniel seems to be, so I don't mind.
OK - so I'll queue it. Thanks, Amit