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

[Luiz Capitulino]

On Wed, 23 Jan 2013 13:31:37 -0200
Luiz Capitulino <address@hidden> wrote:

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

Forgot to say that, my buffer size is 4 bytes:

 -chardev memory,id=foo,maxcapacity=4