Messages in this thread | | | Date | Thu, 5 Jan 2006 10:13:52 -0500 | From | Russ Cox <> | Subject | Re: [V9fs-developer] [PATCH 3/3] v9fs: zero copy implementation |
| |
> +char *v9fs_str_copy(char *buf, int buflen, struct v9fs_str *str) > +{ > + int n; > + > + if (buflen < str->len) > + n = buflen; > + else > + n = str->len; > + > + memmove(buf, str->str, n - 1); > + > + return buf; > +}
The above is wrong. You'll chop the end of the string off when str->len <= buflen.
n = str->len; if (n > buflen-1) n = buflen-1; memmove(buf, str->str, n); buf[n] = 0;
> +int v9fs_str_compare(char *buf, struct v9fs_str *str) > +{ > + int n, ret; > + > + ret = strncmp(buf, str->str, str->len); > + > + if (!ret) { > + n = strlen(buf); > + if (n < str->len) > + ret = -1; > + else if (n > str->len) > + ret = 1; > + } > + > + return ret; > +}
You go through all this work to avoid copying the strings, which has questionable benefit, and then this routine walks the length of the string twice, unnecessarily. Also if strlen(buf) < str->len, then strncmp can't return 0.
ret = strncmp(buf, str->str, str->len); if (!ret && buf[str->len]) ret = 1; return ret;
> static inline int buf_check_size(struct cbuf *buf, int len) > { [snip deleted lines] > + if (buf->p + len > buf->ep && buf->p < buf->ep) { > + eprintk(KERN_ERR, "buffer overflow: want %d has %d\n", > + len, (int)(buf->ep - buf->p)); > + dump_stack(); > + buf->p = buf->ep + 1; > + return 0; > } > > return 1;
I think it's weird that you return 1 when you've already overflowed. It's fine that you don't print more than once, but what's the harm in returning 0 always when buf->p + len > buf->ep?
Russ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |