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] Add Strrplc() To Tapsets


On Wednesday 17 Jun 2009 7:48:23 am Josh Stone wrote:
> On 06/16/2009 03:47 AM, Varun Chandramohan wrote:
> > Hi Josh,
> >
> > 	   Here is a modified version of the function. I made the changes you
> > suggested me. I have not made a patch as i wanted to check with you
> > first. Let me know if its ok? I will submit proper patch along with
> > testcase later.
>
> Thanks.  When you write your test cases, please try to include corner
> cases like truncation and a 0-length search string.
>
> > function str_replace:string (prnt_str:string,
> > srch_str:string,rplc_str:string) %{
> >         char temp[MAXSTRINGLEN];
>
> If you must use a temp array, then it will have to be static, since
> MAXSTRINGLEN could be too large to fit on the kernel stack.  But I still
> don't think you need this array.
>
> >         while((ptr = strstr(ptr, THIS->srch_str)) != NULL) {
>
> This will loop forever if strlen(THIS->srch_str)==0.  Since you also
> need that length repeatedly to advance ptr, I would save and check that
> length ahead of time.  If 0, set CONTEXT->last_error and return.
>
Ah, something i didnt notice. Thanks for that. Now, this makes me wonder,
THIS->__retvalue max size would be MAXSTRINGLEN right? If so, if we have to 
replace a sting which would result in a string much bigger than MAXSTRINGLEN 
would cause an overflow right? Should we check that case too? Or am i missing 
something?
> >                 memset(temp, 0, MAXSTRINGLEN);
> >                 strlcpy(temp, ptr_base, ptr - ptr_base + 1);
> >                 strlcat(THIS->__retvalue, temp, MAXSTRINGLEN);
>
> It's ok to modify the parameters, so how about this:
>
>                   *ptr = '\0';
>                   strlcat(THIS->__retvalue, ptr_base, MAXSTRINGLEN);
I didnt want to modify the original parameters, so ideally i would have copied 
the full string into some array and then manipulated as you specified above.
I chose temp to avoid that copy. Since you suggest that original parameter can 
be changed then we can remove temp. I will make the change and submit. 
>
> >         if(ptr_base == THIS->prnt_str) {
> >                 strlcpy(THIS->__retvalue, THIS->prnt_str, MAXSTRINGLEN);
> >         } else {
> >                 ptr = THIS->prnt_str;
> >                 ptr = ptr + strlen(THIS->prnt_str);
> >                 strlcpy(temp, ptr_base, ptr - ptr_base + 1);
> >                 strlcat(THIS->__retvalue, temp, MAXSTRINGLEN);
> >         }
>
> This seems strangely complicates -- why not just this?
>
Will be done.
>           strlcat(THIS->__retvalue, ptr_base, MAXSTRINGLEN);
>
>
> Josh

-- 
Regards,
Varun Chandramohan


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