From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48690) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKpSZ-00032Y-OJ for qemu-devel@nongnu.org; Wed, 27 Mar 2013 08:28:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKpSY-000571-Ew for qemu-devel@nongnu.org; Wed, 27 Mar 2013 08:28:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31305) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKpSY-00056u-5t for qemu-devel@nongnu.org; Wed, 27 Mar 2013 08:28:06 -0400 Date: Wed, 27 Mar 2013 08:27:56 -0400 From: Luiz Capitulino Message-ID: <20130327082756.212b15f7@redhat.com> In-Reply-To: <515295A1.8030705@linux.vnet.ibm.com> References: <1364240439-23450-1-git-send-email-lcapitulino@redhat.com> <1364240439-23450-3-git-send-email-lcapitulino@redhat.com> <515295A1.8030705@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] Monitor: Make output buffer dynamic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: fred.konrad@greensocs.com, qemu-devel@nongnu.org, kraxel@redhat.com On Wed, 27 Mar 2013 14:45:53 +0800 Wenchao Xia 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 > > --- > > monitor.c | 42 +++++++++++++++++++++++++----------------- > > 1 file changed, 25 insertions(+), 17 deletions(-) > > >