Re: [Qemu-devel] [PATCH 1/6] qemu-char: Convert MemCharDriver to circular buffer

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 23 Aug 2012 13:14:21 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qemu-char.c |   96 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>  qemu-char.h |    2 +-
>  2 files changed, 78 insertions(+), 20 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 398baf1..b21b93a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2528,38 +2528,96 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>  /***********************************************************/
>  /* Memory chardev */
>  typedef struct {
> -    size_t outbuf_size;
> -    size_t outbuf_capacity;
> -    uint8_t *outbuf;
> +    size_t cbuf_capacity;
> +    size_t cbuf_in;
> +    size_t cbuf_out;
> +    size_t cbuf_count;
> +    uint8_t *cbuf;
>  } MemoryDriver;
>  
> +static int mem_chr_is_empty(CharDriverState *chr)
> +{
> +    MemoryDriver *d = chr->opaque;
> +
> +    return d->cbuf_count == 0;
> +}
> +
> +static int mem_chr_is_full(CharDriverState *chr)
> +{
> +    MemoryDriver *d = chr->opaque;
> +
> +    return d->cbuf_count == d->cbuf_capacity;
> +}

Please, make them return a bool and chr can be const.

> +
>  static int mem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>  {
>      MemoryDriver *d = chr->opaque;
> +    int left;
>  
> -    /* TODO: the QString implementation has the same code, we should
> -     * introduce a generic way to do this in cutils.c */
> -    if (d->outbuf_capacity < d->outbuf_size + len) {
> -        /* grow outbuf */
> -        d->outbuf_capacity += len;
> -        d->outbuf_capacity *= 2;
> -        d->outbuf = g_realloc(d->outbuf, d->outbuf_capacity);
> +    if (d->cbuf_capacity < len) {
> +        return -1;
>      }

This is the first time I look at a circular buffer implementation, but
I'd expect this too work: ie. you just write the last bytes that fit on
the buffer.

>  
> -    memcpy(d->outbuf + d->outbuf_size, buf, len);
> -    d->outbuf_size += len;
> +    left = d->cbuf_capacity - d->cbuf_count % d->cbuf_capacity;
> +
> +    /* Some of cbuf need to be overwrited */
> +    if (left < len) {
> +        memcpy(d->cbuf + d->cbuf_in, buf, left);
> +        memcpy(d->cbuf + d->cbuf_out, buf + left, len - left);
> +        d->cbuf_out = (d->cbuf_out + len - left) % d->cbuf_capacity;
> +        d->cbuf_count = d->cbuf_count + left;
> +    } else {
> +        /* Completely overwrite */
> +        if (mem_chr_is_full(chr)) {
> +            d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
> +        } else {
> +        /* Enough cbuf to write */
> +        d->cbuf_count += len;
> +        }
> +        memcpy(d->cbuf + d->cbuf_in, buf, len);
> +    }

Couldn't this be made simpler by having a pointer to d->cbuf that points
to where we are, then we just made that pointer points to the beginning
of the buffer every time we cross its end? Just an idea.

> +
> +    d->cbuf_in = (d->cbuf_in + len) % d->cbuf_capacity;
>  
>      return len;
>  }
>  
> -void qemu_chr_init_mem(CharDriverState *chr)
> +static void mem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
> +{
> +    MemoryDriver *d = chr->opaque;
> +    int left;
> +
> +    if (mem_chr_is_empty(chr)) {
> +        return;
> +    }
> +
> +    left = d->cbuf_capacity - d->cbuf_count % d->cbuf_capacity;
> +
> +    if (d->cbuf_capacity < len) {
> +        len = d->cbuf_capacity;
> +    }
> +
> +    if (left < len) {
> +        memcpy(buf, d->cbuf + d->cbuf_out, left);
> +        memcpy(buf + left, d->cbuf + d->cbuf_out + left, len - left);
> +    } else {
> +        memcpy(buf, d->cbuf + d->cbuf_out, len);
> +    }
> +
> +    d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
> +    d->cbuf_count -= len;
> +}
> +
> +void qemu_chr_init_mem(CharDriverState *chr, size_t size)

Won't this break bisect?

>  {
>      MemoryDriver *d;
>  
>      d = g_malloc(sizeof(*d));
> -    d->outbuf_size = 0;
> -    d->outbuf_capacity = 4096;
> -    d->outbuf = g_malloc0(d->outbuf_capacity);
> +    d->cbuf_capacity = size;
> +    d->cbuf_in = 0;
> +    d->cbuf_out = 0;
> +    d->cbuf_count = 0;
> +    d->cbuf = g_malloc0(d->cbuf_capacity);
>  
>      memset(chr, 0, sizeof(*chr));
>      chr->opaque = d;
> @@ -2569,7 +2627,7 @@ void qemu_chr_init_mem(CharDriverState *chr)
>  QString *qemu_chr_mem_to_qs(CharDriverState *chr)
>  {
>      MemoryDriver *d = chr->opaque;
> -    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
> +    return qstring_from_substr((char *) d->cbuf, 0, d->cbuf_count - 1);
>  }
>  
>  /* NOTE: this driver can not be closed with qemu_chr_delete()! */
> @@ -2577,7 +2635,7 @@ void qemu_chr_close_mem(CharDriverState *chr)
>  {
>      MemoryDriver *d = chr->opaque;
>  
> -    g_free(d->outbuf);
> +    g_free(d->cbuf);
>      g_free(chr->opaque);
>      chr->opaque = NULL;
>      chr->chr_write = NULL;
> @@ -2586,7 +2644,7 @@ void qemu_chr_close_mem(CharDriverState *chr)
>  size_t qemu_chr_mem_osize(const CharDriverState *chr)
>  {
>      const MemoryDriver *d = chr->opaque;
> -    return d->outbuf_size;
> +    return d->cbuf_count;
>  }
>  
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> diff --git a/qemu-char.h b/qemu-char.h
> index 486644b..d8d90cc 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -243,7 +243,7 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
>  extern int term_escape_char;
>  
>  /* memory chardev */
> -void qemu_chr_init_mem(CharDriverState *chr);
> +void qemu_chr_init_mem(CharDriverState *chr, size_t size);
>  void qemu_chr_close_mem(CharDriverState *chr);
>  QString *qemu_chr_mem_to_qs(CharDriverState *chr);
>  size_t qemu_chr_mem_osize(const CharDriverState *chr);