This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH] Fix tokenize function and test.
On 06/16/2009 04:40 PM, Przemysław Pawełczyk wrote:
>>> if (token)
>>> - strncpy (THIS->__retvalue, token, MAXSTRINGLEN);
>>> + strncpy(THIS->__retvalue, token, (str_start ? str_start : str_end + 1) - token);
>>> %}
>>
>> Why do you need the explicit length computation? There will always be a
>> NUL there anyway, right? I tried reverting this part, and the tests
>> still pass, so I don't see what this is for.
>>
>> Also, while we're at it, strlcpy would be preferred for better
>> termination semantics (guaranteed NUL and no extra NUL padding).
>
> You contradict yourself. There is no need for using strlcpy here,
> because what is here does the same, but better -- without implicit
> strlen calls (only one for the whole input string). Guaranteed NUL and
> no extra NUL padding.
If you really want to be concerned with optimizing this, then you should
be using memcpy instead, since the position of the NUL is precisely known.
> Maybe you're talking about putting it into another patch? Yes, it
> works without this change, but I would say, that is also a fix, but
> for badly written code. Of course I can send it as a second patch if
> you wish.
I don't understand the hate for strlcpy, but I don't care so much to
argue over it. Technically, we could even be using strcpy here, since
the input string is already known to be small enough.
I care that the code is clear and concise. It wasn't immediately
obvious to me that (str_start ? str_start : str_end + 1) means "the end
of this token," and my question shows that I also didn't see why that
was a win. I get it now, but I'm bound to forget the next time I look
at it. :)
Josh