qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH-for-4.2 v3 3/3] trace: Forbid dynamic field width in event fo


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH-for-4.2 v3 3/3] trace: Forbid dynamic field width in event format
Date: Mon, 18 Nov 2019 22:31:25 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1

On 11/18/19 10:26 PM, Eric Blake wrote:
On 11/18/19 3:04 PM, Philippe Mathieu-Daudé wrote:
Since not all trace backends support dynamic field width in
format (dtrace via stap does not), forbid them.

Add a check to refuse field width in new formats:

   $ make
   [...]
     GEN     hw/block/trace.h
   Traceback (most recent call last):
     File "scripts/tracetool.py", line 152, in <module>
       main(sys.argv)
     File "scripts/tracetool.py", line 143, in main
       events.extend(tracetool.read_events(fh, arg))
     File "scripts/tracetool/__init__.py", line 371, in read_events
       event = Event.build(line)
     File "scripts/tracetool/__init__.py", line 285, in build
       raise ValueError("Event format must not contain field width '%*'")
   ValueError: Error at hw/block/trace-events:11: Event format must not contain field width '%*'

Reported-by: Eric Blake <address@hidden>
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
---

+++ b/docs/devel/tracing.txt
@@ -113,7 +113,8 @@ Format strings should reflect the types defined in the trace event.  Take
  special care to use PRId64 and PRIu64 for int64_t and uint64_t types,
  respectively.  This ensures portability between 32- and 64-bit platforms.   Format strings must not end with a newline character.  It is the responsibility
-of backends to adapt line ending for proper logging.
+of backends to adapt line ending for proper logging.  Format strings must not +use numeric field width dynamic precision (SystemTap does not support them).

Reads awkwardly - a dynamic precision is not numeric in the format string (but '*' instead).  Better might be:

Format strings must not use dynamic field width or precision ('*'), as at least SystemTap does not support them.

Or even:

Format strings may use numeric field width or precision, but must not use dynamic forms ('*') as at least SystemTap does not support that.

  Each event declaration will start with the event name, then its arguments,
  finally a format string for pretty-printing. For example:
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 44c118bc2a..ec7fe9fa4a 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -206,6 +206,7 @@ class Event(object):
                        "\s*"
                        "(?:(?:(?P<fmt_trans>\".+),)?\s*(?P<fmt>\".+))?"
                        "\s*")
+    _DFWRE = re.compile(r"%[\d\.\- +#']*\*") # dynamic width precision

The comment is slightly off - this catches both dynamic field width (any '*' before '.') and dynamic precision (any '*' after '.'), maybe the fix is just s/width/width or/

      _VALID_PROPS = set(["disable", "tcg", "tcg-trans", "tcg-exec", "vcpu"])
@@ -280,6 +281,8 @@ class Event(object):
          if fmt.endswith(r'\n"'):
              raise ValueError("Event format must not end with a newline "
                               "character")
+        if Event._DFWRE.search(fmt):
+            raise ValueError("Event format must not contain field width '%*'")

and I don't know if you want to tweak the error message, maybe:
  Event format must not use dynamic '*'

If we're trying to get stuff in 4.2-rc2, patch 1 and 2 are actual bug fixes and deserve to go in; patch 3 is nice-to-have but doesn't affect the build if it is omitted (as there are no other offenders left), so slipping it into 5.0 for a v4 to clean it up slightly doesn't hurt.  I don't know who would send the pull request, though, and slipping 1 and 2 into -rc3 just because of 3 is not ideal.

Wise thought. I'll respin 1-2 with the Fixes: tag in case qemu-block@ or qemu-trivial@ have pending pull-request for tomorrow, they can queue it. Else they can go via the mips-next tree, since both patch affect the Malta board.

Thanks!

Phil.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]