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