Re: [Qemu-devel] [PATCH 1/5] qemu-char: Add new char backend CircularMemCharDriver

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 22 Oct 2012 00:47:57 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>

This patch should be squashed in the next one. More comments below.

> ---
>  qemu-char.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 72 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index b082bae..b174da1 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
>      return d->outbuf_size;
>  }
>  
> +/*********************************************************/
> +/*CircularMemoryr chardev*/

s/Memoryr/Memory

> +
> +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 int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    CirMemCharDriver *d = chr->opaque;
> +    int i;
> +
> +    if (len < 0) {
> +        return -1;
> +    }
> +
> +    /* The size should be a power of 2. */
> +    for (i = 0; i < len; i++ ) {
> +        d->cbuf[d->producer % d->size] = buf[i];
> +        d->producer++;
> +    }
> +
> +    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++) {
> +        buf[i] = d->cbuf[d->consumer % d->size];
> +        d->consumer++;
> +    }
> +
> +    return 0;
> +}

You don't seem to reset producer/consumer anywhere, I wonder if it's possible
for a long running VM to trigger the limit here.

> +
> +static void cirmem_chr_close(struct CharDriverState *chr)
> +{
> +    CirMemCharDriver *d = chr->opaque;
> +
> +    g_free(d);
> +    g_free(chr->opaque);

Double free. I think you want to free cbuf, like:

g_free(d->cbuf);
g_free(d);

> +    chr->opaque = NULL;
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];