From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41204) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzIeV-0001MS-Tn for qemu-devel@nongnu.org; Thu, 01 Sep 2011 21:34:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QzIeU-0002X8-FX for qemu-devel@nongnu.org; Thu, 01 Sep 2011 21:34:39 -0400 Received: from mail-yi0-f45.google.com ([209.85.218.45]:54446) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzIeU-0002X2-Bk for qemu-devel@nongnu.org; Thu, 01 Sep 2011 21:34:38 -0400 Received: by yih10 with SMTP id 10so2087721yih.4 for ; Thu, 01 Sep 2011 18:34:37 -0700 (PDT) Message-ID: <4E6032AB.8080804@codemonkey.ws> Date: Thu, 01 Sep 2011 20:34:35 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <20110901163545.71ba1515@doriath> In-Reply-To: <20110901163545.71ba1515@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] monitor: Protect outbuf from concurrent access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Marian Krcmarik , Alon Levy , qemu-devel On 09/01/2011 02:35 PM, Luiz Capitulino wrote: > Sometimes, when having lots of VMs running on a RHEV host and the user > attempts to close a SPICE window, libvirt will get corrupted json from > QEMU. > > After some investigation, I found out that the problem is that different > SPICE threads are calling monitor functions (such as > monitor_protocol_event()) in parallel which causes concurrent access > to the monitor's internal buffer outbuf[]. > > This fixes the problem by protecting accesses to outbuf[] with a mutex. > > Honestly speaking, I'm not completely sure this the best thing to do > because the monitor itself and other qemu subsystems are not thread safe, > so having subsystems like SPICE assuming the contrary seems a bit > catastrophic to me... > > Anyways, this commit fixes the problem at hand. Nack. This is absolutely a Spice bug. Spice should not be calling into QEMU code from multiple threads. It should only call into QEMU code while it's holding the qemu_mutex. The right way to fix this is probably to make all of the SpiceCoreInterface callbacks simply write to a file descriptor which can then wake up QEMU to do the operation on behalf of it. It's ugly but the libspice interface is far too tied to QEMU internals in the first place which is the root of the problem. Regards, Anthony Liguori > > Signed-off-by: Luiz Capitulino > --- > monitor.c | 16 +++++++++++++++- > 1 files changed, 15 insertions(+), 1 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 04f465a..61d4d93 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -57,6 +57,7 @@ > #include "json-parser.h" > #include "osdep.h" > #include "cpu.h" > +#include "qemu-thread.h" > #ifdef CONFIG_SIMPLE_TRACE > #include "trace.h" > #endif > @@ -144,6 +145,7 @@ struct Monitor { > int suspend_cnt; > uint8_t outbuf[1024]; > int outbuf_index; > + QemuMutex mutex; > ReadLineState *rs; > MonitorControl *mc; > CPUState *mon_cpu; > @@ -246,10 +248,14 @@ static int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, > > void monitor_flush(Monitor *mon) > { > + qemu_mutex_lock(&mon->mutex); > + > if (mon&& mon->outbuf_index != 0&& !mon->mux_out) { > qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); > mon->outbuf_index = 0; > } > + > + qemu_mutex_unlock(&mon->mutex); > } > > /* flush at every end of line or if the buffer is full */ > @@ -257,6 +263,8 @@ static void monitor_puts(Monitor *mon, const char *str) > { > char c; > > + qemu_mutex_lock(&mon->mutex); > + > for(;;) { > c = *str++; > if (c == '\0') > @@ -265,9 +273,14 @@ static void monitor_puts(Monitor *mon, const char *str) > mon->outbuf[mon->outbuf_index++] = '\r'; > mon->outbuf[mon->outbuf_index++] = c; > if (mon->outbuf_index>= (sizeof(mon->outbuf) - 1) > - || c == '\n') > + || c == '\n') { > + qemu_mutex_unlock(&mon->mutex); > monitor_flush(mon); > + qemu_mutex_lock(&mon->mutex); > + } > } > + > + qemu_mutex_unlock(&mon->mutex); > } > > void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) > @@ -5275,6 +5288,7 @@ void monitor_init(CharDriverState *chr, int flags) > > mon = g_malloc0(sizeof(*mon)); > > + qemu_mutex_init(&mon->mutex); > mon->chr = chr; > mon->flags = flags; > if (flags& MONITOR_USE_READLINE) {