| Message ID | 1351250512-6386-2-git-send-email-lilei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 26 Oct 2012 19:21:49 +0800 Lei Li <lilei@linux.vnet.ibm.com> wrote: > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > --- > 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(). > + 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? > + } > 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. > #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.
On 10/27/2012 12:47 AM, Luiz Capitulino wrote: > On Fri, 26 Oct 2012 19:21:49 +0800 > Lei Li <lilei@linux.vnet.ibm.com> wrote: > >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> >> --- >> 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. >
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; +} + +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 (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; + } 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 }, #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.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> --- qemu-char.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu-config.c | 3 + qemu-options.hx | 10 ++++ 3 files changed, 153 insertions(+), 0 deletions(-)