[Qemu-devel] [PATCH] trace: Fix 'char **' compilation error in simple backend

[Qemu-devel] [PATCH] trace: Fix 'char **' compilation error in simple backend

[Fam Zheng]

Currently, the generated function body will do "strlen(arg)" but the
argument could be 'char **'. Avoid that by exclusding such cases in
is_string check.

Reported by patchew's "make docker-test-mingw@fedora".

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 scripts/tracetool/backend/simple.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
index 9885e83..2538795 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -21,7 +21,9 @@ PUBLIC = True
 
 def is_string(arg):
     strtype = ('const char*', 'char*', 'const char *', 'char *')
-    if arg.lstrip().startswith(strtype):
+    non_strtype = ('const char**', 'char**', 'const char **', 'char **')
+    arg_strip = arg.lstrip()
+    if arg_strip.startswith(strtype) and not arg_strip.startswith(non_strtype):
         return True
     else:
         return False
-- 
2.7.4

Re: [Qemu-devel] [PATCH] trace: Fix 'char **' compilation error in simple backend

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

On 10/25/2016 08:59 PM, Fam Zheng wrote:
> Currently, the generated function body will do "strlen(arg)" but the
> argument could be 'char **'. Avoid that by exclusding such cases in

s/exclusding/excluding/

> is_string check.
> 
> Reported by patchew's "make docker-test-mingw@fedora".
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  scripts/tracetool/backend/simple.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
> index 9885e83..2538795 100644
> --- a/scripts/tracetool/backend/simple.py
> +++ b/scripts/tracetool/backend/simple.py
> @@ -21,7 +21,9 @@ PUBLIC = True
>  
>  def is_string(arg):
>      strtype = ('const char*', 'char*', 'const char *', 'char *')
> -    if arg.lstrip().startswith(strtype):
> +    non_strtype = ('const char**', 'char**', 'const char **', 'char **')
> +    arg_strip = arg.lstrip()
> +    if arg_strip.startswith(strtype) and not arg_strip.startswith(non_strtype):

There may be a more compact way to write it, but I'm not enough of a
python expert to know offhand what else to suggest (it's not as simple
as string concatenation of strtype + '*', since strtype is a tuple
rather than a string).

What you have will fail to detect 'const char *const *' as a non-string
(possible if we have some argv-like function that takes a constant array
of constant strings), but I guess we can worry about that if we actually
try to trace something with that signature.  In the meantime, what you
have solves the immediate failure, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] trace: Fix 'char **' compilation error in simple backend

[Fam Zheng]

On Tue, 10/25 21:29, Eric Blake wrote:
> On 10/25/2016 08:59 PM, Fam Zheng wrote:
> > Currently, the generated function body will do "strlen(arg)" but the
> > argument could be 'char **'. Avoid that by exclusding such cases in
> 
> s/exclusding/excluding/

Yes, I blame the insomnia last night. @.@

I assume this can be fixed when applying.

> 
> > is_string check.
> > 
> > Reported by patchew's "make docker-test-mingw@fedora".
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  scripts/tracetool/backend/simple.py | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
> > index 9885e83..2538795 100644
> > --- a/scripts/tracetool/backend/simple.py
> > +++ b/scripts/tracetool/backend/simple.py
> > @@ -21,7 +21,9 @@ PUBLIC = True
> >  
> >  def is_string(arg):
> >      strtype = ('const char*', 'char*', 'const char *', 'char *')
> > -    if arg.lstrip().startswith(strtype):
> > +    non_strtype = ('const char**', 'char**', 'const char **', 'char **')
> > +    arg_strip = arg.lstrip()
> > +    if arg_strip.startswith(strtype) and not arg_strip.startswith(non_strtype):
> 
> There may be a more compact way to write it, but I'm not enough of a
> python expert to know offhand what else to suggest (it's not as simple
> as string concatenation of strtype + '*', since strtype is a tuple
> rather than a string).

Did you mean

    non_strtype = tuple(x + '*' for x in strtype)

?

But personally I'd stick to the flatten version in this specific case for
a bit more readability.

Thanks!

Fam

Re: [Qemu-devel] [PATCH] trace: Fix 'char **' compilation error in simple backend

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]

On 10/25/2016 09:46 PM, Fam Zheng wrote:
> On Tue, 10/25 21:29, Eric Blake wrote:
>> On 10/25/2016 08:59 PM, Fam Zheng wrote:
>>> Currently, the generated function body will do "strlen(arg)" but the
>>> argument could be 'char **'. Avoid that by exclusding such cases in
>>

>>>  def is_string(arg):
>>>      strtype = ('const char*', 'char*', 'const char *', 'char *')
>>> -    if arg.lstrip().startswith(strtype):
>>> +    non_strtype = ('const char**', 'char**', 'const char **', 'char **')
>>> +    arg_strip = arg.lstrip()
>>> +    if arg_strip.startswith(strtype) and not arg_strip.startswith(non_strtype):
>>
>> There may be a more compact way to write it, but I'm not enough of a
>> python expert to know offhand what else to suggest (it's not as simple
>> as string concatenation of strtype + '*', since strtype is a tuple
>> rather than a string).
> 
> Did you mean
> 
>     non_strtype = tuple(x + '*' for x in strtype)

Hmm, I guess that would work.

Or, what about a different approach, something like:
  if arg_strip.startswith(strtype) and no_multiple_star(arg_strip):
for some sane definition of no_multiple_star() that checks that there is
exactly one '*' in a string.  In C, I'd check roughly:
  p = strchr(str, '*');
  if (p && !strchr(p + 1, '*')) {
    // treat str as string
  }
but again, I'm not enough of an expert to pop that out late at night,
even if python has an easy one-liner way to express that.


> But personally I'd stick to the flatten version in this specific case for
> a bit more readability.

Indeed, and that's why I gave R-b as-is, even if it fails when there are
multiple 'const' qualifiers in a string with multiple '*' :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] trace: Fix 'char **' compilation error in simple backend

[Fam Zheng]

On Tue, 10/25 21:56, Eric Blake wrote:
> On 10/25/2016 09:46 PM, Fam Zheng wrote:
> > On Tue, 10/25 21:29, Eric Blake wrote:
> >> On 10/25/2016 08:59 PM, Fam Zheng wrote:
> >>> Currently, the generated function body will do "strlen(arg)" but the
> >>> argument could be 'char **'. Avoid that by exclusding such cases in
> >>
> 
> >>>  def is_string(arg):
> >>>      strtype = ('const char*', 'char*', 'const char *', 'char *')
> >>> -    if arg.lstrip().startswith(strtype):
> >>> +    non_strtype = ('const char**', 'char**', 'const char **', 'char **')
> >>> +    arg_strip = arg.lstrip()
> >>> +    if arg_strip.startswith(strtype) and not arg_strip.startswith(non_strtype):
> >>
> >> There may be a more compact way to write it, but I'm not enough of a
> >> python expert to know offhand what else to suggest (it's not as simple
> >> as string concatenation of strtype + '*', since strtype is a tuple
> >> rather than a string).
> > 
> > Did you mean
> > 
> >     non_strtype = tuple(x + '*' for x in strtype)
> 
> Hmm, I guess that would work.
> 
> Or, what about a different approach, something like:
>   if arg_strip.startswith(strtype) and no_multiple_star(arg_strip):
> for some sane definition of no_multiple_star() that checks that there is
> exactly one '*' in a string.  In C, I'd check roughly:
>   p = strchr(str, '*');
>   if (p && !strchr(p + 1, '*')) {
>     // treat str as string
>   }
> but again, I'm not enough of an expert to pop that out late at night,
> even if python has an easy one-liner way to express that.

That's indeed a nicer approach:

    if arg_strip.startswith(strtype) and arg_strip.count("*") == 1:

Do you want a respin with your suggested-by? :-)

Fam

> 
> 
> > But personally I'd stick to the flatten version in this specific case for
> > a bit more readability.
> 
> Indeed, and that's why I gave R-b as-is, even if it fails when there are
> multiple 'const' qualifiers in a string with multiple '*' :)
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>