On Wed, 23 Jan 2013 13:31:37 -0200
Luiz Capitulino <lcapitul...@redhat.com> wrote:
> On Wed, 23 Jan 2013 11:15:40 +0800
> Lei Li <li...@linux.vnet.ibm.com> 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
