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

[Laszlo Ersek <lersek@redhat.com>]

On 08/11/15 19:04, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@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.lureau@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.

Thanks
Laszlo