This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]