On Tue, 28 May 2013 13:17:09 +0100
David Howells <dhowells@xxxxxxxxxx> wrote:
> Chen Gang <gang.chen@xxxxxxxxxxx> wrote:
>
> > Your suggestion will improve the speed, but may merge "transferring
> > 'protocol' data" and "processing 'protocol' data" together.
>
> Look at it this way: You're having to step very carefully because you are
> fully expecting the strings not to be NUL-terminated. Therefore you probably
> avoid using string functions if you can.
>
> In fact, looking at the code, why are you copying the data through an
> intermediate buffer at all? Why not just copy directly to userspace:
>
> int len;
> char buf[2];
>
> if (!*lenp || *ppos) {
> *lenp = 0;
> return 0;
> }
> if (!write) {
> - len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
> + const char *ptr = appldata_timer_active ? "1\n" : "0\n";
> + size_t len = 2;
> if (len > *lenp)
> len = *lenp;
> if (copy_to_user(buffer, buf, len))
> return -EFAULT;
> goto out;
> }
>
> Put like that, it's fairly obvious what is going on.
Yes, we could do that for !write, but we can't get rid of the buffer
completely, as we need it for the other case and copy_from_user().
I have already applied the v2 version from Chen Gang, as it fixes the
overflow bug, and the affected code is not performance critical at all.
I like the improved readability of your approach, but the patch is already
on its way to Martins "for-linus" branch, and I think it's "good enough".
Thanks for all the feedback to this little piece of very old (and partly
ugly) code from my very first Linux device driver :-)
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
