Re: [Qemu-devel] Abort in monitor_puts.

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 22 Mar 2013 16:50:39 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Fri, 22 Mar 2013 10:17:58 +0100
> KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> 
> > Hi,
> > 
> > Seems there is an issue with the current git (found by toddf on IRC).
> > 
> > To reproduce:
> > 
> > ./qemu-system-x86_64 --monitor stdio --nographic
> > 
> > and put "?" it should abort.
> > 
> > Here is the backtrace:
> > 
> > #0  0x00007f77cd347935 in raise () from /lib64/libc.so.6
> > #1  0x00007f77cd3490e8 in abort () from /lib64/libc.so.6
> > #2  0x00007f77cd3406a2 in __assert_fail_base () from /lib64/libc.so.6
> > #3  0x00007f77cd340752 in __assert_fail () from /lib64/libc.so.6
> > #4  0x00007f77d1c1f226 in monitor_puts (mon=<optimized out>,
> >      str=<optimized out>) at 
> 
> Yes, it's easy to reproduce. Bisect says:
> 
> f628926bb423fa8a7e0b114511400ea9df38b76a is the first bad commit
> commit f628926bb423fa8a7e0b114511400ea9df38b76a
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Tue Mar 19 10:57:56 2013 +0100
> 
>     fix monitor
>     
>     chardev flow control broke monitor, fix it by adding watch support.
>     
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> My impression is that monitor_puts() in being called in parallel.

Not all.

What's happening is that qemu_chr_fe_write() is returning < 0,
mon->outbuf_index is not reset and is full, this causes the assert in
monitor_puts() to trig.

The previous version of monitor_flush() ignores errors, and everything
works, so doing the same thing here fixes the problem :)

For some reason I'm unable to see what the error code is. Gerd, do you think
the patch below is reasonable? If it's not, how should we handle errors here?

diff --git a/monitor.c b/monitor.c
index cfb5d64..ecfe97c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -274,12 +274,11 @@ void monitor_flush(Monitor *mon)
 
     if (mon && mon->outbuf_index != 0 && !mon->mux_out) {
         rc = qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index);
-        if (rc == mon->outbuf_index) {
+        if (rc == mon->outbuf_index || rc < 0) {
             /* all flushed */
             mon->outbuf_index = 0;
             return;
-        }
-        if (rc > 0) {
+        } else {
             /* partinal write */
             memmove(mon->outbuf, mon->outbuf + rc, mon->outbuf_index - rc);
             mon->outbuf_index -= rc;