This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
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