[Qemu-devel] [PATCH 1.7] monitor: eliminate monitor_event_state_lock

[Qemu-devel] [PATCH 1.7] monitor: eliminate monitor_event_state_lock

[Paolo Bonzini]

This lock does not protect anything that the BQL does not already
protect.  Furthermore, with -nodefaults and no monitor, the mutex
is not initialized but monitor_protocol_event_queue is called
anyway, which causes a crash under mingw (and only works by luck.
under Linux or other POSIX OSes).

Reported-by: Orx Goshen <orx.goshen@intel.com>
Cc: Daniel Berrange <berrange@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Maybe I'm missing something?

 monitor.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 74f3f1b..0ae99dc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -511,7 +511,6 @@ static const char *monitor_event_names[] = {
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
 
 MonitorEventState monitor_event_state[QEVENT_MAX];
-QemuMutex monitor_event_state_lock;
 
 /*
  * Emits the event to every monitor instance
@@ -543,7 +542,6 @@ monitor_protocol_event_queue(MonitorEvent event,
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     assert(event < QEVENT_MAX);
 
-    qemu_mutex_lock(&monitor_event_state_lock);
     evstate = &(monitor_event_state[event]);
     trace_monitor_protocol_event_queue(event,
                                        data,
@@ -576,7 +574,6 @@ monitor_protocol_event_queue(MonitorEvent event,
             evstate->last = now;
         }
     }
-    qemu_mutex_unlock(&monitor_event_state_lock);
 }
 
 
@@ -589,7 +586,6 @@ static void monitor_protocol_event_handler(void *opaque)
     MonitorEventState *evstate = opaque;
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
-    qemu_mutex_lock(&monitor_event_state_lock);
 
     trace_monitor_protocol_event_handler(evstate->event,
                                          evstate->data,
@@ -601,7 +597,6 @@ static void monitor_protocol_event_handler(void *opaque)
         evstate->data = NULL;
     }
     evstate->last = now;
-    qemu_mutex_unlock(&monitor_event_state_lock);
 }
 
 
@@ -638,7 +633,6 @@ monitor_protocol_event_throttle(MonitorEvent event,
  * and initialize state */
 static void monitor_protocol_event_init(void)
 {
-    qemu_mutex_init(&monitor_event_state_lock);
     /* Limit RTC & BALLOON events to 1 per second */
     monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);
     monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1.7] monitor: eliminate monitor_event_state_lock

[Daniel P. Berrange]

On Wed, Oct 16, 2013 at 07:17:08PM +0200, Paolo Bonzini wrote:
> This lock does not protect anything that the BQL does not already
> protect.  Furthermore, with -nodefaults and no monitor, the mutex
> is not initialized but monitor_protocol_event_queue is called
> anyway, which causes a crash under mingw (and only works by luck.
> under Linux or other POSIX OSes).
> 
> Reported-by: Orx Goshen <orx.goshen@intel.com>
> Cc: Daniel Berrange <berrange@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	Maybe I'm missing something?

ACK, I only added it because I wasn't clear on
wether the BQL was sufficient, so it was just a
safety net.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1.7] monitor: eliminate monitor_event_state_lock

[Luiz Capitulino]

On Wed, 16 Oct 2013 19:17:08 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This lock does not protect anything that the BQL does not already
> protect.  Furthermore, with -nodefaults and no monitor, the mutex
> is not initialized but monitor_protocol_event_queue is called
> anyway, which causes a crash under mingw (and only works by luck.
> under Linux or other POSIX OSes).
> 
> Reported-by: Orx Goshen <orx.goshen@intel.com>
> Cc: Daniel Berrange <berrange@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Applied to the qmp branch, thanks.

> ---
> 	Maybe I'm missing something?

I don't think so.

> 
>  monitor.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 74f3f1b..0ae99dc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -511,7 +511,6 @@ static const char *monitor_event_names[] = {
>  QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
>  
>  MonitorEventState monitor_event_state[QEVENT_MAX];
> -QemuMutex monitor_event_state_lock;
>  
>  /*
>   * Emits the event to every monitor instance
> @@ -543,7 +542,6 @@ monitor_protocol_event_queue(MonitorEvent event,
>      int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>      assert(event < QEVENT_MAX);
>  
> -    qemu_mutex_lock(&monitor_event_state_lock);
>      evstate = &(monitor_event_state[event]);
>      trace_monitor_protocol_event_queue(event,
>                                         data,
> @@ -576,7 +574,6 @@ monitor_protocol_event_queue(MonitorEvent event,
>              evstate->last = now;
>          }
>      }
> -    qemu_mutex_unlock(&monitor_event_state_lock);
>  }
>  
>  
> @@ -589,7 +586,6 @@ static void monitor_protocol_event_handler(void *opaque)
>      MonitorEventState *evstate = opaque;
>      int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>  
> -    qemu_mutex_lock(&monitor_event_state_lock);
>  
>      trace_monitor_protocol_event_handler(evstate->event,
>                                           evstate->data,
> @@ -601,7 +597,6 @@ static void monitor_protocol_event_handler(void *opaque)
>          evstate->data = NULL;
>      }
>      evstate->last = now;
> -    qemu_mutex_unlock(&monitor_event_state_lock);
>  }
>  
>  
> @@ -638,7 +633,6 @@ monitor_protocol_event_throttle(MonitorEvent event,
>   * and initialize state */
>  static void monitor_protocol_event_init(void)
>  {
> -    qemu_mutex_init(&monitor_event_state_lock);
>      /* Limit RTC & BALLOON events to 1 per second */
>      monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);
>      monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000);