Re: [Qemu-devel] [PATCH 2/2] Monitor: Make output buffer dynamic

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 27 Mar 2013 14:45:53 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> Hi, Luiz
>   Personally I hope reduce the dynamic allocated buffer which brings
> fragments and unexpected memory grow. Instead, how about sacrifice
> some time to wait output complete, since monitor is not time critical?
> in this case static buffer's size can decide how many work can be
> postponded. Following is my suggestion:

I'd be fine with it if you find a good way to postpone work, but sleeping
on random timers is not a good way.

Also, QEMU allocates fragments of memory in so many places that I doubt
this is an valid argument. You're right that long QMP output can cause
unexpected memory growth, but I expected this to be really rare and if
this really bother us, we can ask monitor_puts() to flush at, say, every
4096 bytes.

> 
> --- a/monitor.c
> +++ b/monitor.c
> @@ -293,17 +293,28 @@ static void monitor_puts(Monitor *mon, const char
> *str)
>  {
>      char c;
> 
> +    /* if mux do not put in any thing to buffer */
> +    if (mon->mux_out) {
> +        return;
> +    }
> +
>      for(;;) {
> -        assert(mon->outbuf_index < sizeof(mon->outbuf) - 1);
> +        if (mon->outbuf_index >= sizeof(mon->outbuf) - 1) {
> +            /* when buffer is full, flush it and retry. If buffer is
> bigger, more
> +               work can be postponed. */
> +            monitor_flush(mon);
> +            usleep(1);
> +            continue;
> +        }
>          c = *str++;
>          if (c == '\0')
>              break;
>          if (c == '\n')
>              mon->outbuf[mon->outbuf_index++] = '\r';
>          mon->outbuf[mon->outbuf_index++] = c;
> -        if (mon->outbuf_index >= (sizeof(mon->outbuf) - 1)
> -            || c == '\n')
> +        if (c == '\n') {
>              monitor_flush(mon);
> +        }
>      }
>  }
> 
> > Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush()
> > to retry on qemu_chr_fe_write() errors. However, the Monitor's output
> > buffer can keep growing while the retry is not issued and this can
> > cause the buffer to overflow.
> > 
> > To reproduce this issue, just start qemu and type on the Monitor:
> > 
> > (qemu) ?
> > 
> > This will cause the assertion to trig.
> > 
> > To fix this problem this commit makes the Monitor buffer dynamic,
> > which means that it can grow as much as needed.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >   monitor.c | 42 +++++++++++++++++++++++++-----------------
> >   1 file changed, 25 insertions(+), 17 deletions(-)
> > 
>