Re: [Qemu-devel] [PATCH 6/6] monitor: protect event emission

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue,  3 Jun 2014 18:39:11 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Event emission must be protected by a mutex because of access to
> the shared rate-limiting state, and to guard against concurrent
> monitor "hot-plug" by means of human-monitor-command.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  monitor.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index ebc66fb..f7a4ccf 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -210,6 +210,9 @@ struct Monitor {
>  /* QMP checker flags */
>  #define QMP_ACCEPT_UNKNOWNS 1
>  
> +/* Protects mon_list, monitor_event_state.  */
> +static QemuMutex monitor_lock;
> +
>  static QLIST_HEAD(mon_list, Monitor) mon_list;
>  static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
>  static int mon_refcount;
> @@ -531,10 +534,11 @@ static const char *monitor_event_names[] = {
>  };
>  QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
>  
> -MonitorEventState monitor_event_state[QEVENT_MAX];
> +static MonitorEventState monitor_event_state[QEVENT_MAX];
>  
>  /*
> - * Emits the event to every monitor instance
> + * Emits the event to every monitor instance.
> + * Called with monitor_lock held.
>   */
>  static void
>  monitor_protocol_event_emit(MonitorEvent event,
> @@ -571,6 +575,7 @@ monitor_protocol_event_queue(MonitorEvent event,
>                                         now);
>  
>      /* Rate limit of 0 indicates no throttling */
> +    qemu_mutex_lock(&monitor_lock);
>      if (!evstate->rate) {
>          monitor_protocol_event_emit(event, data);
>          evstate->last = now;
> @@ -595,6 +600,7 @@ monitor_protocol_event_queue(MonitorEvent event,
>              evstate->last = now;
>          }
>      }
> +    qemu_mutex_unlock(&monitor_lock);
>  }
>  
>  
> @@ -612,12 +618,14 @@ static void monitor_protocol_event_handler(void *opaque)
>                                           evstate->data,
>                                           evstate->last,
>                                           now);
> +    qemu_mutex_lock(&monitor_lock);
>      if (evstate->data) {
>          monitor_protocol_event_emit(evstate->event, evstate->data);
>          qobject_decref(evstate->data);
>          evstate->data = NULL;
>      }
>      evstate->last = now;
> +    qemu_mutex_unlock(&monitor_lock);
>  }
>  
>  
> @@ -5040,6 +5048,11 @@ static void monitor_readline_flush(void *opaque)
>      monitor_flush(opaque);
>  }
>  
> +static void __attribute__((constructor)) monitor_lock_init(void)
> +{
> +    qemu_mutex_init(&monitor_lock);
> +}
> +
>  void monitor_init(CharDriverState *chr, int flags)
>  {
>      static int is_first_init = 1;
> @@ -5077,7 +5090,10 @@ void monitor_init(CharDriverState *chr, int flags)
>                                monitor_event, mon);
>      }
>  
> +    qemu_mutex_lock(&monitor_lock);
>      QLIST_INSERT_HEAD(&mon_list, mon, entry);
> +    qemu_mutex_unlock(&monitor_lock);
> +
>      if (!default_mon || (flags & MONITOR_IS_DEFAULT))
>          default_mon = mon;
>  }