Re: [Qemu-devel] [PATCH] monitor: remove QAPI_EVENT_VSERPORT_CHANGE throttle

[Amit Shah]

On (Tue) 11 Aug 2015 [20:21:18], Laszlo Ersek wrote:
> On 08/11/15 19:04, address@hidden wrote:
> > From: Marc-André Lureau <address@hidden>
> > 
> > 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 <address@hidden>
> > ---
> >  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