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


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.


function str_replace:string (prnt_str:string, srch_str:string,rplc_str:string)
%{
        char temp[MAXSTRINGLEN];
        char *ptr = THIS->prnt_str;
        char *ptr_base = THIS->prnt_str;

        while((ptr = strstr(ptr, THIS->srch_str)) != NULL) {
                memset(temp, 0, MAXSTRINGLEN);
                strlcpy(temp, ptr_base, ptr - ptr_base + 1);
                strlcat(THIS->__retvalue, temp, MAXSTRINGLEN);
                strlcat(THIS->__retvalue, THIS->rplc_str, MAXSTRINGLEN);
                ptr = ptr + strlen(THIS->srch_str);
                ptr_base = ptr;
        }

        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);
        }
        return;
%}



On Tuesday 16 Jun 2009 5:31:13 am Varun Chandramohan wrote:
> On Tuesday 16 Jun 2009 2:01:10 am Josh Stone wrote:
> > On 06/15/2009 12:50 AM, Varun Chandramohan wrote:
> > > Hi,function strrplc:string (prnt_str:string, srch_str:string, 
rplc_str:string)


> > >
> > > This patch adds a search and replace string functionality to existing
> > > tapsets. I found this function to be helpful in many use cases. The
> > > functionality is as follows: The function takes in a parent string
> > > and searches for a substring as specified by the user. If substing
> > > not found, the parent string is returned. If substring is found, it
> > > is replaced by another string of the same lenght as substring and
> > > returned.
> > >
> > > NOTE: The function will search and replace all the occurance of
> > > substrings in a parent string when matched.
> > >
> > > Signed-off-by: Varun Chandramohan <varunc@linux.vnet.ibm.com>
> >
> > Hi Varun,
> >
> > This seems like a good function to have, thanks.  I find strrplc() a bit
> > cryptic though -- How about str_replace() or even just replace()?
>
> i think str_replace() seems to a good name.

> > > ---
> > >  tapset/string.stp |   36 ++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 36 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/tapset/string.stp b/tapset/string.stp
> > > index 35ee9fa..b277479 100644
> > > --- a/tapset/string.stp
> > > +++ b/tapset/string.stp
> > > @@ -92,6 +92,42 @@ function tokenize:string(input:string, delim:string)
> > >  %}
> > >
> > >  /*
> > > + * strrplc - Takes a parent string, uses the second string which is a
> > > + *		substring to parent string and replaces that with the
> > > + *		third string of same lenght as second and returns the
> > > + *		new converted parent string.
> > > + * s1	The parent string. If NULL, the function returns NULL.
> > > + *
> > > + * s2	The substring which is used to search in the parent string s1.
> > > + *
> > > + * s3	The substring which is used to replace the searched string s2.
> > > + *	Both s2 and s3 have to be of same lenght. Else s1 is returned.
> > > + */
> >
> > I don't like the restriction that they must be the same length.  I
> > understand how that makes the implementation easier, but I think it
> > would be more useful if it allowed arbitrary string lengths.
>
> Well, its not because of the implementation but my usecase when i wrote
> this didnt need variable length replace string. I think its a good idea to
> provide it. I will implement it in next patch.
>
> > Also, please choose parameter names that have meaning to what they are.
> >
> > > +function strrplc:string (s1:string, s2:string, s3:string)
> > > +%{
> > > +	static char str[MAXSTRINGLEN];
> >
> > There's no reason to use a static array here, and it makes this function
> > SMP-unsafe.  Just build the result directly in the return array.
>
> ok, will fix that.
>
> > > +	if(THIS->s1 == NULL)
> > > +		return THIS->__retvalue = NULL;
> >
> > Strings in THIS are arrays, not pointers.  They can never be NULL, and
> > you definitely can't assign them.  This part will give compiler errors.
>
> Sorry, i just added the check in the last min before creating the patch and
> forgot to test it. Will fix it.
>
> > > +
> > > +	if(strlen(THIS->s2) != strlen(THIS->s3)) {
> > > +		strncpy(THIS->__retvalue, THIS->s1, MAXSTRINGLEN);
> > > +		return;
> > > +	}
> > > +
> > > +	strncpy(str, THIS->s1, MAXSTRINGLEN);
> > > +
> > > +	while((ptr = strstr(ptr, THIS->s2)) != NULL) {
> > > +		memcpy(ptr, THIS->s3, strlen(THIS->s3));
> > > +		ptr = ptr + strlen(THIS->s3);
> > > +	}
> > > +
> > > +	strncpy(THIS->__retvalue, str, MAXSTRINGLEN);
> > > +	return;
> > > +%}
> >
> > First, I would prefer using strlcpy/strlcat, as they have better
> > NUL-termination semantics.
>
> ok
>
> > To support unequal search and replace string lengths, you can do
> > something like this pseudocode:
> >
> >   while search is found:
> >     strlcat the portion up to the ptr
> >     strlcat the replace string
> >   strlcat the tail
> >
> > Finally, please provide tests for this new function, so we can make sure
> > that it always works.
>
> Yes i will submit testcase
>
> > Thanks,
> >
> > Josh
>
> Regards,
> Varun


-- 
Regards,
Varun Chandramohan


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