From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7026 invoked by alias); 16 Jun 2009 23:40:25 -0000 Received: (qmail 7006 invoked by uid 22791); 16 Jun 2009 23:40:22 -0000 X-SWARE-Spam-Status: No, hits=0.1 required=5.0 tests=AWL,BAYES_50,SARE_MSGID_LONG40 X-Spam-Check-By: sourceware.org Received: from mail-fx0-f210.google.com (HELO mail-fx0-f210.google.com) (209.85.220.210) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 16 Jun 2009 23:40:16 +0000 Received: by fxm6 with SMTP id 6so5378637fxm.24 for ; Tue, 16 Jun 2009 16:40:13 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.119.198 with SMTP id a6mr5966951far.42.1245195612982; Tue, 16 Jun 2009 16:40:12 -0700 (PDT) In-Reply-To: <4A36A872.8030207@redhat.com> References: <1244863762.20494.8055@debian> <1244896709.407628.14220@debian> <4A36A872.8030207@redhat.com> Date: Tue, 16 Jun 2009 23:40:00 -0000 Message-ID: <40e92d5b0906161640r70dfd39fm65ecd8e3463da47c@mail.gmail.com> Subject: Re: [PATCH] Fix tokenize function and test. From: =?ISO-8859-2?Q?Przemys=B3aw_Pawe=B3czyk?= To: Josh Stone Cc: systemtap@sourceware.org Content-Type: text/plain; charset=ISO-8859-2 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org X-SW-Source: 2009-q2/txt/msg00917.txt.bz2 On Mon, Jun 15, 2009 at 22:00, Josh Stone 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 =3D strsep(&str_start, THIS->delim); >> + while ((token =3D 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=B3aw Pawe=B3czyk