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

[Lei Li <lilei@linux.vnet.ibm.com>]

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.
>


-- 
Lei