Re: [Qemu-devel] [PATCH 1/3] qemu-char: Add new char backend CirMemCharDriver

[Lei Li]

On 01/23/2013 12:14 AM, Luiz Capitulino wrote:
> On Tue, 22 Jan 2013 16:12:50 +0800
> Lei Li <address@hidden> wrote:
>
> > Signed-off-by: Lei Li <address@hidden>
> > ---
> >   qemu-char.c     |  120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   qemu-options.hx |   10 +++++
> >   2 files changed, 130 insertions(+), 0 deletions(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 9ba0573..b323e94 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -98,6 +98,7 @@
> >   #include "ui/qemu-spice.h"
> >   
> >   #define READ_BUF_LEN 4096
> > +#define CBUFF_SIZE 65536
> >   
> >   /***********************************************************/
> >   /* character device */
> > @@ -2643,6 +2644,116 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
> >       return d->outbuf_size;
> >   }
> >   
> > +/*********************************************************/
> > +/*CircularMemory chardev*/
> > +
> > +typedef struct {
> > +    size_t size;
> > +    size_t prod;
> > +    size_t cons;
> > +    uint8_t *cbuf;
> > +} CirMemCharDriver;
> > +
> > +static bool cirmem_chr_is_empty(const CharDriverState *chr)
> > +{
> > +    const CirMemCharDriver *d = chr->opaque;
> > +
> > +    return d->cons == d->prod;
> > +}
> > +
> > +static bool cirmem_chr_is_full(const CharDriverState *chr)
> > +{
> > +    const CirMemCharDriver *d = chr->opaque;
> > +
> > +    return (d->prod - d->cons) == d->size;
> > +}
> > +
> > +static size_t qemu_chr_cirmem_count(const CharDriverState *chr)
> > +{
> > +    const CirMemCharDriver *d = chr->opaque;
> > +
> > +    return (d->prod - d->cons);
> > +}
> > +
> > +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;
> > +    }
> Is the above checks really needed? I don't see other char drivers
> doing that.
>
> > +
> > +    for (i = 0; i < len; i++ ) {
> > +        /* Avoid writing the IAC information to the queue. */
> > +        if ((unsigned char)buf[i] == IAC) {
> > +            continue;
> > +        }
> > +
> > +        d->cbuf[d->prod++ % d->size] = buf[i];
> You never reset d->prod, can't it overflow?
>
> > +        if ((d->prod - d->cons) > d->size) {
> > +            d->cons = d->prod - d->size;
> > +        }
> Why is the the if block above needed?

This algorithm is Anthony's suggestion which I squashed
it in. I think there is no overflow for that we use unsigned, and
this if block will adjust the index of product and consumer.

> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
> > +{
> > +    CirMemCharDriver *d = chr->opaque;
> > +    int i;
> > +
> > +    for (i = 0; i < len && !cirmem_chr_is_empty(chr); i++) {
> > +        buf[i] = d->cbuf[d->cons++ % d->size];
> You never reset d->cons, can't it overflow?
>
> > +    }
> > +
> > +    return i;
> > +}
> > +
> > +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)) {
> Please, print a descriptive error message. Otherwise all the user will get
> is a generic error.

Sure.

> > +        goto fail;
> > +    }
> > +
> > +    d->prod = 0;
> > +    d->cons = 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];
> > @@ -2707,6 +2818,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const 
> > char *filename)
> >           qemu_opt_set(opts, "path", p);
> >           return opts;
> >       }
> > +    if (strstart(filename, "memory", &p)) {
> > +        qemu_opt_set(opts, "backend", "memory");
> > +        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) {
> > @@ -2796,6 +2912,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 = "memory",    .open = qemu_chr_open_cirmemchr },
> >   #ifdef _WIN32
> >       { .name = "file",      .open = qemu_chr_open_win_file_out },
> >       { .name = "pipe",      .open = qemu_chr_open_win_pipe },
> > @@ -3055,6 +3172,9 @@ 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 4e2b499..2d44137 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1736,6 +1736,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 memory,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
> > @@ -1777,6 +1778,7 @@ Backend is one of:
> >   @option{udp},
> >   @option{msmouse},
> >   @option{vc},
> > address@hidden,
> >   @option{file},
> >   @option{pipe},
> >   @option{console},
> > @@ -1885,6 +1887,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.
> >   
> > address@hidden -chardev memory ,address@hidden ,address@hidden
> > +
> > +Create a circular buffer with fixed size indicated by optionally 
> > @option{maxcapacity}
> > +which will be default 64K if it is not given.
> > +
> > address@hidden specifies the max capacity of the size of circular buffer
> > +to create. Should be power of 2.
> > +
> >   @item -chardev file ,address@hidden ,address@hidden
> >   
> >   Log all traffic received from the guest to a file.


--
Lei