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

[Luiz Capitulino]

On Wed, 23 Jan 2013 11:15:40 +0800
Lei Li <address@hidden> wrote:

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

No answer.

> >> +
> >> +    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,

Actually, it can overflow. However, at least on my system the overflow
reset the variable to 0. Not sure if this will always be the case, but
that's the fix I'd propose anyway.

> and
> this if block will adjust the index of product and consumer.

I can see that, I asked why it's needed. I'm seeing a bug that might
be related to it:

(qemu) memchar_write foo luiz3
(qemu) memchar_read foo 10
uiz3, 
(qemu) 

I'd expect to read '3uiz' back.