[Qemu-devel] [PATCH 0/3] tracetool: improve error messages

[Qemu-devel] [PATCH 0/3] tracetool: improve error messages

[Stefan Hajnoczi]

This series improves the tracetool error messages to make them more
user-friendly.

Stefan Hajnoczi (3):
  tracetool: prefix parse errors with line numbers
  tracetool: clarify that "formats" means "format strings"
  tracetool: report error on foo() instead of foo(void)

 scripts/tracetool/__init__.py | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/3] tracetool: prefix parse errors with line numbers

[Stefan Hajnoczi]

Include the file line number in the message that is printed when
trace-events parse errors are raised.

Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/__init__.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 0670ec17d5..fbccaaa0cf 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -300,13 +300,18 @@ def read_events(fobj):
     """
 
     events = []
-    for line in fobj:
+    for lineno, line in enumerate(fobj):
         if not line.strip():
             continue
         if line.lstrip().startswith('#'):
             continue
 
-        event = Event.build(line)
+        try:
+            event = Event.build(line)
+        except ValueError as e:
+            arg0 = 'Error on line %d: %s' % (lineno + 1, e.args[0])
+            e.args = (arg0,) + e.args[1:]
+            raise
 
         # transform TCG-enabled events
         if "tcg" not in event.properties:
-- 
2.14.3

[Qemu-devel] [PATCH 2/3] tracetool: clarify that "formats" means "format strings"

[Stefan Hajnoczi]

The terminology used by tracetool is not consistent with C sprintf or
docs/devel/tracing.txt.  The word "formats" is sometimes used to mean
"format strings".

This patch clarifies comments and error messages that contain this word.

Note that the error message lines are longer than 80 characters but I
have not wrapped them to aid grepping.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/__init__.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index fbccaaa0cf..5db1f5c395 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -173,7 +173,7 @@ class Event(object):
         props : list of str
             Property names.
         fmt : str, list of str
-            Event printing format (or formats).
+            Event printing format string(s).
         args : Arguments
             Event arguments.
         orig : Event or None
@@ -237,9 +237,9 @@ class Event(object):
         if "tcg-exec" in props:
             raise ValueError("Invalid property 'tcg-exec'")
         if "tcg" not in props and not isinstance(fmt, str):
-            raise ValueError("Only events with 'tcg' property can have two formats")
+            raise ValueError("Only events with 'tcg' property can have two format strings")
         if "tcg" in props and isinstance(fmt, str):
-            raise ValueError("Events with 'tcg' property must have two formats")
+            raise ValueError("Events with 'tcg' property must have two format strings")
 
         event = Event(name, props, fmt, args)
 
@@ -263,7 +263,7 @@ class Event(object):
     _FMT = re.compile("(%[\d\.]*\w+|%.*PRI\S+)")
 
     def formats(self):
-        """List of argument print formats."""
+        """List conversion specifiers in the argument print format string."""
         assert not isinstance(self.fmt, list)
         return self._FMT.findall(self.fmt)
 
-- 
2.14.3

[Qemu-devel] [PATCH 3/3] tracetool: report error on foo() instead of foo(void)

[Stefan Hajnoczi]

C functions with no arguments must be declared foo(void) instead of
foo().  The tracetool argument list parser has never accepted an empty
argument list.  This patch adds a clear error message for this error
case.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/__init__.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 5db1f5c395..8e9efba78f 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -75,6 +75,8 @@ class Arguments:
         res = []
         for arg in arg_str.split(","):
             arg = arg.strip()
+            if not arg:
+                raise ValueError("Empty argument (did you forget to use 'void'?)")
             if arg == 'void':
                 continue
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/3] tracetool: prefix parse errors with line numbers

[Eric Blake]

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

On 01/10/2018 02:25 PM, Stefan Hajnoczi wrote:
> Include the file line number in the message that is printed when
> trace-events parse errors are raised.
> 
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  scripts/tracetool/__init__.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> index 0670ec17d5..fbccaaa0cf 100644
> --- a/scripts/tracetool/__init__.py
> +++ b/scripts/tracetool/__init__.py
> @@ -300,13 +300,18 @@ def read_events(fobj):
>      """
>  
>      events = []
> -    for line in fobj:
> +    for lineno, line in enumerate(fobj):

If you use enumerate(fobj, 1) here...

>          if not line.strip():
>              continue
>          if line.lstrip().startswith('#'):
>              continue
>  
> -        event = Event.build(line)
> +        try:
> +            event = Event.build(line)
> +        except ValueError as e:
> +            arg0 = 'Error on line %d: %s' % (lineno + 1, e.args[0])

you could avoid the +1 here.

Up to you; either way, the patch is an improvement.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 0/3] tracetool: improve error messages

[Eric Blake]

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

On 01/10/2018 02:25 PM, Stefan Hajnoczi wrote:
> This series improves the tracetool error messages to make them more
> user-friendly.
> 
> Stefan Hajnoczi (3):
>   tracetool: prefix parse errors with line numbers
>   tracetool: clarify that "formats" means "format strings"
>   tracetool: report error on foo() instead of foo(void)

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

but see nit on patch 1

> 
>  scripts/tracetool/__init__.py | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 0/3] tracetool: improve error messages

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180110202553.31889-1-stefanha@redhat.com
Subject: [Qemu-devel] [PATCH 0/3] tracetool: improve error messages

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
08b156de69 tracetool: report error on foo() instead of foo(void)
7c4579f8ce tracetool: clarify that "formats" means "format strings"
3842c08817 tracetool: prefix parse errors with line numbers

=== OUTPUT BEGIN ===
Checking PATCH 1/3: tracetool: prefix parse errors with line numbers...
Checking PATCH 2/3: tracetool: clarify that "formats" means "format strings"...
ERROR: line over 90 characters
#37: FILE: scripts/tracetool/__init__.py:240:
+            raise ValueError("Only events with 'tcg' property can have two format strings")

WARNING: line over 80 characters
#40: FILE: scripts/tracetool/__init__.py:242:
+            raise ValueError("Events with 'tcg' property must have two format strings")

total: 1 errors, 1 warnings, 27 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/3: tracetool: report error on foo() instead of foo(void)...
WARNING: line over 80 characters
#24: FILE: scripts/tracetool/__init__.py:79:
+                raise ValueError("Empty argument (did you forget to use 'void'?)")

total: 0 errors, 1 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 0/3] tracetool: improve error messages

[Stefan Hajnoczi]

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

On Wed, Jan 10, 2018 at 02:48:16PM -0600, Eric Blake wrote:
> On 01/10/2018 02:25 PM, Stefan Hajnoczi wrote:
> > This series improves the tracetool error messages to make them more
> > user-friendly.
> > 
> > Stefan Hajnoczi (3):
> >   tracetool: prefix parse errors with line numbers
> >   tracetool: clarify that "formats" means "format strings"
> >   tracetool: report error on foo() instead of foo(void)
> 
> Series:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> but see nit on patch 1

Thanks, will fix!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/3] tracetool: improve error messages

[Stefan Hajnoczi]

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

On Wed, Jan 10, 2018 at 08:25:50PM +0000, Stefan Hajnoczi wrote:
> This series improves the tracetool error messages to make them more
> user-friendly.
> 
> Stefan Hajnoczi (3):
>   tracetool: prefix parse errors with line numbers
>   tracetool: clarify that "formats" means "format strings"
>   tracetool: report error on foo() instead of foo(void)
> 
>  scripts/tracetool/__init__.py | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> -- 
> 2.14.3
> 

Thanks, applied (with Eric Blake's improvement) to my tracing tree:
https://github.com/stefanha/qemu/commits/tracing

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]