On Mon, Oct 13, 2014 at 11:20:35PM +0200, Rickard Strandqvist wrote:
> 2014-10-12 21:22 GMT+02:00 Jason Cooper <ja...@lakedaemon.net>:
> > Rickard,
> >
> > On Sun, Oct 12, 2014 at 12:49:31PM +0200, Rickard Strandqvist wrote:
> >> Changed from using strncat to strlcat to simplify the code
> >
> > I'd like to see a little more explicit discussion here. As Guenter got
> > caught up in the mis-understanding, I doubt he'd be the only one. I
> > think it's worth spelling out that the old code prevents overflowing the
> > buffer 'buf' of size PAGE_SIZE. And that strlcat() does that internally
> > allowing this code to be more readable.
> >
> > It should also be mentioned that the final strlen(buf) is safe because
> > every operation on buf will insert a NULL terminator within the
> > buffers limit.
> >
> > thx,
> >
> > Jason.
> >
> >> Signed-off-by: Rickard Strandqvist <rickard_strandqv...@spectrumdigital.se>
> >> ---
[1]
> >> drivers/char/hw_random/core.c | 12 ++++--------
> >> 1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> >> index aa30a25..1500cfd 100644
> >> --- a/drivers/char/hw_random/core.c
> >> +++ b/drivers/char/hw_random/core.c
> >> @@ -281,7 +281,6 @@ static ssize_t hwrng_attr_available_show(struct device
> >> *dev,
> >> char *buf)
> >> {
> >> int err;
> >> - ssize_t ret = 0;
> >> struct hwrng *rng;
> >>
> >> err = mutex_lock_interruptible(&rng_mutex);
> >> @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct
> >> device *dev,
> >> return -ERESTARTSYS;
> >> buf[0] = '\0';
> >> list_for_each_entry(rng, &rng_list, list) {
> >> - strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> >> - ret += strlen(rng->name);
> >> - strncat(buf, " ", PAGE_SIZE - ret - 1);
> >> - ret++;
> >> + strlcat(buf, rng->name, PAGE_SIZE);
> >> + strlcat(buf, " ", PAGE_SIZE);
> >> }
> >> - strncat(buf, "\n", PAGE_SIZE - ret - 1);
> >> - ret++;
> >> + strlcat(buf, "\n", PAGE_SIZE);
> >> mutex_unlock(&rng_mutex);
> >>
> >> - return ret;
> >> + return strlen(buf);
> >> }
> >>
> >> static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> >> --
> >> 1.7.10.4
>
>
> Hi
>
> Do not know if I understand this right, you want to explain strlcat
> function better then ..?
I want to see that the submitter of the patch has thought this through
and isn't just blindly doing s/strn/strl/g and some cleanup.
Please keep in mind that the kernel community is *huge* and no one
person can see everything going on. The type of fixes and cleanup
you're doing crosses many sub-systems. As a result, you haven't popped
up on anyones radar as a regular contributor within a sub-system yet.
iow, I didn't have the thought in my head "Rickard, yeah, he's the guy
doing the cppcheck and strn/l cleanup properly" because none of your
patches have crossed my inbox until now.
> But while I think this is something you have to learn, rather than
> typing it in git comment.
Wether it's appropriate for the git comment or not is debatable, I'll
agree. The point I'm trying to make is that reviewers aren't
super-human. All I saw at first is a patch from someone I don't know
changing buffer handling in crypto/rng code. I had no indication in the
email as to how carefully this had been done. I'll call that out every
time. :)
A short explanation, here [1], would have let first-time reviewers of
your patches know that you had taken the time to grok the code and
wasn't blindly fulfilling a eudyptula challenge or similar.
thx,
Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
