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

[Lei Li]

On 01/23/2013 11:31 PM, Luiz Capitulino 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.

Sorry for that...
Yes, I think this check is needed for that we should avoid
passing len with negative value and buf with NULL. And
other char drivers also did this although not exactly the
same, like fd_chr_read, pty_chr_read...


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


--
Lei