From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42762) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TSgqw-0005Qh-Vw for qemu-devel@nongnu.org; Mon, 29 Oct 2012 00:21:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TSgqv-0002IC-Gz for qemu-devel@nongnu.org; Mon, 29 Oct 2012 00:21:30 -0400 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:40686) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TSgqu-0002I6-P3 for qemu-devel@nongnu.org; Mon, 29 Oct 2012 00:21:29 -0400 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 29 Oct 2012 14:19:31 +1000 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q9T4B6OH58392638 for ; Mon, 29 Oct 2012 15:11:06 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q9T4LIjX010225 for ; Mon, 29 Oct 2012 15:21:19 +1100 Message-ID: <508E0411.4080806@linux.vnet.ibm.com> Date: Mon, 29 Oct 2012 12:20:33 +0800 From: Lei Li MIME-Version: 1.0 References: <1351250512-6386-1-git-send-email-lilei@linux.vnet.ibm.com> <1351250512-6386-2-git-send-email-lilei@linux.vnet.ibm.com> <20121026144715.13f0bebe@doriath.home> In-Reply-To: <20121026144715.13f0bebe@doriath.home> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/4] qemu-char: Add new char backend CirMemCharDriver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: blauwirbel@gmail.com, aliguori@us.ibm.com, qemu-devel@nongnu.org On 10/27/2012 12:47 AM, Luiz Capitulino wrote: > On Fri, 26 Oct 2012 19:21:49 +0800 > Lei Li wrote: > >> Signed-off-by: Lei Li >> --- >> qemu-char.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qemu-config.c | 3 + >> qemu-options.hx | 10 ++++ >> 3 files changed, 153 insertions(+), 0 deletions(-) >> >> diff --git a/qemu-char.c b/qemu-char.c >> index b082bae..c3ec43d 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -99,6 +99,7 @@ >> #include "ui/qemu-spice.h" >> >> #define READ_BUF_LEN 4096 >> +#define CBUFF_SIZE 65536 >> >> /***********************************************************/ >> /* character device */ >> @@ -2588,6 +2589,134 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr) >> return d->outbuf_size; >> } >> >> +/*********************************************************/ >> +/*CircularMemory chardev*/ >> + >> +typedef struct { >> + size_t size; >> + size_t producer; >> + size_t consumer; >> + uint8_t *cbuf; >> +} CirMemCharDriver; >> + >> +static bool cirmem_chr_is_empty(const CharDriverState *chr) >> +{ >> + const CirMemCharDriver *d = chr->opaque; >> + >> + return d->producer == d->consumer; >> +} >> + >> +static bool cirmem_chr_is_full(const CharDriverState *chr) >> +{ >> + const CirMemCharDriver *d = chr->opaque; >> + >> + return (d->producer - d->consumer) >= d->size; >> +} >> + >> +static bool cirmem_chr_unlimit(const CharDriverState *chr) >> +{ >> + const CirMemCharDriver *d = chr->opaque; >> + >> + return d->producer >= d->consumer; >> +} > I'm not sure I get why this is needed. Are you trying to fix the problem > I mentioned in my last review that producer and consumer might overflow > for long running VMs? If yes, then the problem still exists. > > The only fix I can think of is to re-initialize them. Of course that you > could do it in a way that they keep pointing to the right position in cbuf[]. > >> + >> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) >> +{ >> + CirMemCharDriver *d = chr->opaque; >> + int i; >> + >> + if (!buf || (len < 0)) { >> + return -1; >> + } >> + >> + for (i = 0; i < len; i++ ) { >> + if (cirmem_chr_unlimit(chr)) { >> + /* Avoid writing the IAC information to the queue. */ >> + if ((unsigned char)buf[i] == IAC) { >> + continue; >> + } >> + d->cbuf[d->producer % d->size] = buf[i]; >> + d->producer++; >> + } else { >> + return -1; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len) >> +{ >> + CirMemCharDriver *d = chr->opaque; >> + int i; >> + >> + if (cirmem_chr_is_empty(chr) || len < 0) { >> + return -1; >> + } >> + >> + if (len > d->size) { >> + len = d->size; >> + } >> + >> + for (i = 0; i < len; i++) { >> + if (cirmem_chr_unlimit(chr)) { >> + buf[i] = d->cbuf[d->consumer % d->size]; >> + d->consumer++; >> + >> + if (cirmem_chr_is_empty(chr)) { >> + break; >> + } >> + } else { >> + return -1; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static void cirmem_chr_close(struct CharDriverState *chr) >> +{ >> + CirMemCharDriver *d = chr->opaque; >> + >> + g_free(d->cbuf); >> + g_free(d); >> + chr->opaque = NULL; >> +} >> + >> +static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts) >> +{ >> + CharDriverState *chr; >> + CirMemCharDriver *d; >> + >> + chr = g_malloc0(sizeof(CharDriverState)); >> + d = g_malloc(sizeof(*d)); >> + >> + d->size = qemu_opt_get_number(opts, "maxcapacity", 0); >> + if (d->size == 0) { >> + d->size = CBUFF_SIZE; >> + } >> + >> + /* The size must be power of 2 */ > If this is enforced in vl.c (which I can't remember right now), then > you should turn this into an assert(). It is just enforced in here, not in vl.c. >> + if (d->size & (d->size - 1)) { >> + goto fail; >> + } >> + >> + d->producer = 0; >> + d->consumer = 0; >> + d->cbuf = g_malloc0(d->size); >> + >> + chr->opaque = d; >> + chr->chr_write = cirmem_chr_write; >> + chr->chr_close = cirmem_chr_close; >> + >> + return chr; >> + >> +fail: >> + g_free(d); >> + g_free(chr); >> + return NULL; >> +} >> + >> QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) >> { >> char host[65], port[33], width[8], height[8]; >> @@ -2652,6 +2781,16 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) >> qemu_opt_set(opts, "path", p); >> return opts; >> } >> + if (strstart(filename, "memchr", &p)) { >> + qemu_opt_set(opts, "backend", "memchr"); >> + qemu_opt_set(opts, "maxcapacity", p); >> + return opts; >> + } >> + if (strstart(filename, "memchr", &p)) { >> + qemu_opt_set(opts, "backend", "memchr"); >> + qemu_opt_set(opts, "maxcapacity", p); >> + return opts; > Duplication? Yeah... >> + } >> if (strstart(filename, "tcp:", &p) || >> strstart(filename, "telnet:", &p)) { >> if (sscanf(p, "%64[^:]:%32[^,]%n", host, port, &pos) < 2) { >> @@ -2725,6 +2864,7 @@ static const struct { >> { .name = "udp", .open = qemu_chr_open_udp }, >> { .name = "msmouse", .open = qemu_chr_open_msmouse }, >> { .name = "vc", .open = text_console_init }, >> + { .name = "memchr", .open = qemu_chr_open_cirmemchr }, > What about my suggestion of calling this "memory"? All backends here are > chardevs, but we don't have vcchr, msmousechr, stdiochr etc. Yes, it make sense. >> #ifdef _WIN32 >> { .name = "file", .open = qemu_chr_open_win_file_out }, >> { .name = "pipe", .open = qemu_chr_open_win_pipe }, >> diff --git a/qemu-config.c b/qemu-config.c >> index cd1ec21..5553bb6 100644 >> --- a/qemu-config.c >> +++ b/qemu-config.c >> @@ -213,6 +213,9 @@ static QemuOptsList qemu_chardev_opts = { >> },{ >> .name = "debug", >> .type = QEMU_OPT_NUMBER, >> + },{ >> + .name = "maxcapacity", >> + .type = QEMU_OPT_NUMBER, >> }, >> { /* end of list */ } >> }, >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 46f0539..9b2d792 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -1680,6 +1680,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev, >> "-chardev msmouse,id=id[,mux=on|off]\n" >> "-chardev vc,id=id[[,width=width][,height=height]][[,cols=cols][,rows=rows]]\n" >> " [,mux=on|off]\n" >> + "-chardev memchr,id=id,maxcapacity=maxcapacity\n" >> "-chardev file,id=id,path=path[,mux=on|off]\n" >> "-chardev pipe,id=id,path=path[,mux=on|off]\n" >> #ifdef _WIN32 >> @@ -1718,6 +1719,7 @@ Backend is one of: >> @option{udp}, >> @option{msmouse}, >> @option{vc}, >> +@option{memchr}, >> @option{file}, >> @option{pipe}, >> @option{console}, >> @@ -1824,6 +1826,14 @@ the console, in pixels. >> @option{cols} and @option{rows} specify that the console be sized to fit a text >> console with the given dimensions. >> >> +@item -chardev memchr ,id=@var{id} ,maxcapacity=@var{maxcapacity} >> + >> +Create a circular buffer with fixed size indicated by optionally @option{maxcapacity} >> +which will be default 64K if it is not given. >> + >> +@option{maxcapacity} specifies the max capacity of the size of circular buffer >> +to create. Should be power of 2. >> + >> @item -chardev file ,id=@var{id} ,path=@var{path} >> >> Log all traffic received from the guest to a file. > -- Lei