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.


2009/6/17 Josh Stone <jistone@redhat.com>:
> 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.

I'm not so concerned about optimization, but I don't like doing
superfluous things. You're totally right with memcpy, will you accept
it if I use it in my patch?

>> 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 have no feelings towards strlcpy. Why do you think so? It's useful
function and I don't negate this fact.
Simply there is no reason for doing implicit strlen call every
tokenize("", delim), because (as you pointed already) position of the
NUL is precisely known.
Maybe in a few years MAXSTRINGLEN will be hundred times greater than
today, so then we will be changing this function once again?
IMHO assuming that input string is known to be small enough is not the
right way.

> 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. :)

"The end of this token" semantic is consequence of how strsep works.
Nevertheless I can add some comments if you think it will make the
code more clear.

> Josh

Regards.

-- 
Przemysław Pawełczyk


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