On 05/29/2013 12:03 AM, Gerald Schaefer wrote:
> 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 :-)
>
OK, thank you. and thank all related feedback from Geert, Walter, David
Thanks.
--
Chen Gang
Asianux Corporation
--
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
