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 Mon, Jun 15, 2009 at 22:00, Josh Stone<jistone@redhat.com> wrote:
> On 06/13/2009 05:33 AM, Przemyslaw Pawelczyk wrote:
>> Previous implementation was error-prone, because allowed returning empty
>> tokens (mimiced strsep()), which is fine if there is a NULL semantic.
>> Unfortunately SystemTap doesn't provide it in scripts and has only blank
>> string (""), therefore testing against it was misleading.
>> The solution is to return only non-empty tokens (mimic strtok()).
>>
>> * tapset/string.stp: Fix tokenize.
>> * testsuite/systemtap.string/tokenize.stp: Improve and add case with
>>   more than one delimiter in the delim string.
>> * stapfuncs.3stap.in: Update tokenize description.
>> * doc/langref.tex: Ditto.
> [...]
>
>> -     token = strsep(&str_start, THIS->delim);
>> +     while ((token = strsep(&str_start, THIS->delim)) && !token[0])
>> +             ;
>
> do-while would be cleaner, please.

I'll fix this in next version of patch.

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

> Josh

Thank you for reviewing this patch.

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]