[Qemu-devel] [PULL 0/5] Tracing patches

[Qemu-devel] [PULL 0/5] Tracing patches

[Stefan Hajnoczi]

The following changes since commit e4ae62b802cec437f877f2cadc4ef059cc0eca76:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2018-03-09 17:28:16 +0000)

are available in the Git repository at:

  git://github.com/stefanha/qemu.git tags/tracing-pull-request

for you to fetch changes up to 73ff061032efa0b260bcd1a177ac89b57a1642d3:

  trace: only permit standard C types and fixed size integer types (2018-03-12 11:10:20 +0000)

----------------------------------------------------------------

----------------------------------------------------------------

Daniel P. Berrangé (3):
  trace: include filename when printing parser error messages
  trace: remove use of QEMU specific types from trace probes
  trace: only permit standard C types and fixed size integer types

Peter Maydell (1):
  log-for-trace.h: Split out parts of log.h used by trace.h

Stefan Hajnoczi (1):
  simpletrace: fix timestamp argument type

 include/qemu/log-for-trace.h     | 35 +++++++++++++++++++++++++++
 include/qemu/log.h               | 18 ++++----------
 scripts/simpletrace.py           |  6 ++---
 scripts/tracetool.py             |  2 +-
 scripts/tracetool/__init__.py    | 52 ++++++++++++++++++++++++++++++++++++++--
 scripts/tracetool/backend/log.py | 13 +++++-----
 trace-events                     |  6 ++---
 7 files changed, 102 insertions(+), 30 deletions(-)
 create mode 100644 include/qemu/log-for-trace.h

-- 
2.14.3

[Qemu-devel] [PULL 1/5] log-for-trace.h: Split out parts of log.h used by trace.h

[Stefan Hajnoczi]

From: Peter Maydell <peter.maydell@linaro.org>

A persistent build problem we see is where a source file
accidentally omits the #include of log.h. This slips through
local developer testing because if you configure with the
default (log) trace backend trace.h will pull in log.h for you.
Compilation fails only if some other backend is selected.

To make this error cause a compile failure regardless of
the configured trace backend, split out the parts of log.h
that trace.h requires into a new log-for-trace.h header.
Since almost all manual uses of the log.h functions will
use constants or functions which aren't in log-for-trace.h,
this will let us catch missing #include "qemu/log.h" more
consistently.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-id: 20180213140029.8308-1-peter.maydell@linaro.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/log-for-trace.h     | 35 +++++++++++++++++++++++++++++++++++
 include/qemu/log.h               | 18 ++++--------------
 scripts/tracetool/backend/log.py | 13 ++++++-------
 3 files changed, 45 insertions(+), 21 deletions(-)
 create mode 100644 include/qemu/log-for-trace.h

diff --git a/include/qemu/log-for-trace.h b/include/qemu/log-for-trace.h
new file mode 100644
index 0000000000..2f0a5b080e
--- /dev/null
+++ b/include/qemu/log-for-trace.h
@@ -0,0 +1,35 @@
+/* log-for-trace.h: logging basics required by the trace.h generated
+ * by the log trace backend.
+ *
+ * This should not be included directly by any .c file: if you
+ * need to use the logging functions include "qemu/log.h".
+ *
+ * The purpose of splitting these parts out into their own header
+ * is to catch the easy mistake where a .c file includes trace.h
+ * but forgets to include qemu/log.h. Without this split, that
+ * would result in the .c file compiling fine when the default
+ * trace backend is in use but failing to compile with any other
+ * backend.
+ *
+ * This code is licensed under the GNU General Public License,
+ * version 2 or (at your option) any later version.
+ */
+
+#ifndef QEMU_LOG_FOR_TRACE_H
+#define QEMU_LOG_FOR_TRACE_H
+
+/* Private global variable, don't use */
+extern int qemu_loglevel;
+
+#define LOG_TRACE          (1 << 15)
+
+/* Returns true if a bit is set in the current loglevel mask */
+static inline bool qemu_loglevel_mask(int mask)
+{
+    return (qemu_loglevel & mask) != 0;
+}
+
+/* main logging function */
+int GCC_FMT_ATTR(1, 2) qemu_log(const char *fmt, ...);
+
+#endif
diff --git a/include/qemu/log.h b/include/qemu/log.h
index a50e994c21..ff92a8b86a 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -1,10 +1,11 @@
 #ifndef QEMU_LOG_H
 #define QEMU_LOG_H
 
+/* A small part of this API is split into its own header */
+#include "qemu/log-for-trace.h"
 
-/* Private global variables, don't use */
+/* Private global variable, don't use */
 extern FILE *qemu_logfile;
-extern int qemu_loglevel;
 
 /* 
  * The new API:
@@ -41,16 +42,9 @@ static inline bool qemu_log_separate(void)
 #define CPU_LOG_MMU        (1 << 12)
 #define CPU_LOG_TB_NOCHAIN (1 << 13)
 #define CPU_LOG_PAGE       (1 << 14)
-#define LOG_TRACE          (1 << 15)
+/* LOG_TRACE (1 << 15) is defined in log-for-trace.h */
 #define CPU_LOG_TB_OP_IND  (1 << 16)
 
-/* Returns true if a bit is set in the current loglevel mask
- */
-static inline bool qemu_loglevel_mask(int mask)
-{
-    return (qemu_loglevel & mask) != 0;
-}
-
 /* Lock output for a series of related logs.  Since this is not needed
  * for a single qemu_log / qemu_log_mask / qemu_log_mask_and_addr, we
  * assume that qemu_loglevel_mask has already been tested, and that
@@ -69,10 +63,6 @@ static inline void qemu_log_unlock(void)
 
 /* Logging functions: */
 
-/* main logging function
- */
-int GCC_FMT_ATTR(1, 2) qemu_log(const char *fmt, ...);
-
 /* vfprintf-like logging function
  */
 static inline void GCC_FMT_ATTR(1, 0)
diff --git a/scripts/tracetool/backend/log.py b/scripts/tracetool/backend/log.py
index da86f6b882..78933d03ad 100644
--- a/scripts/tracetool/backend/log.py
+++ b/scripts/tracetool/backend/log.py
@@ -20,7 +20,7 @@ PUBLIC = True
 
 
 def generate_h_begin(events, group):
-    out('#include "qemu/log.h"',
+    out('#include "qemu/log-for-trace.h"',
         '')
 
 
@@ -35,14 +35,13 @@ def generate_h(event, group):
     else:
         cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
 
-    out('    if (%(cond)s) {',
+    out('    if (%(cond)s && qemu_loglevel_mask(LOG_TRACE)) {',
         '        struct timeval _now;',
         '        gettimeofday(&_now, NULL);',
-        '        qemu_log_mask(LOG_TRACE,',
-        '                      "%%d@%%zd.%%06zd:%(name)s " %(fmt)s "\\n",',
-        '                      getpid(),',
-        '                      (size_t)_now.tv_sec, (size_t)_now.tv_usec',
-        '                      %(argnames)s);',
+        '        qemu_log("%%d@%%zd.%%06zd:%(name)s " %(fmt)s "\\n",',
+        '                 getpid(),',
+        '                 (size_t)_now.tv_sec, (size_t)_now.tv_usec',
+        '                 %(argnames)s);',
         '    }',
         cond=cond,
         name=event.name,
-- 
2.14.3

[Qemu-devel] [PULL 2/5] simpletrace: fix timestamp argument type

[Stefan Hajnoczi]

The timestamp argument to a trace event method is documented as follows:

  The method can also take a timestamp argument before the trace event
  arguments:

    def runstate_set(self, timestamp, new_state):
        ...

  Timestamps have the uint64_t type and are in nanoseconds.

In reality methods with a timestamp argument actually receive a tuple
like (123456789,) as the timestamp argument.  This is due to a bug in
simpletrace.py.

This patch unpacks the tuple so that methods receive the correct
timestamp argument type.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20180222163901.14095-1-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/simpletrace.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index a3a6315055..be3d1affaf 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -199,7 +199,7 @@ def process(events, log, analyzer, read_header=True):
         fn_argcount = len(inspect.getargspec(fn)[0]) - 1
         if fn_argcount == event_argcount + 1:
             # Include timestamp as first argument
-            return lambda _, rec: fn(*((rec[1:2],) + rec[3:3 + event_argcount]))
+            return lambda _, rec: fn(*(rec[1:2] + rec[3:3 + event_argcount]))
         elif fn_argcount == event_argcount + 2:
             # Include timestamp and pid
             return lambda _, rec: fn(*rec[1:3 + event_argcount])
-- 
2.14.3

[Qemu-devel] [PULL 3/5] trace: include filename when printing parser error messages

[Stefan Hajnoczi]

From: Daniel P. Berrangé <berrange@redhat.com>

Improves error messages from:

  ValueError: Error on line 72: need more than 1 value to unpack

To

  ValueError: Error at /home/berrange/src/virt/qemu/trace-events:72:
    need more than 1 value to unpack

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 20180306154650.24075-1-berrange@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/simpletrace.py        | 4 ++--
 scripts/tracetool.py          | 2 +-
 scripts/tracetool/__init__.py | 6 ++++--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index be3d1affaf..9d45c6ba4e 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -168,7 +168,7 @@ class Analyzer(object):
 def process(events, log, analyzer, read_header=True):
     """Invoke an analyzer on each event in a log."""
     if isinstance(events, str):
-        events = read_events(open(events, 'r'))
+        events = read_events(open(events, 'r'), events)
     if isinstance(log, str):
         log = open(log, 'rb')
 
@@ -233,7 +233,7 @@ def run(analyzer):
                          '<trace-file>\n' % sys.argv[0])
         sys.exit(1)
 
-    events = read_events(open(sys.argv[1], 'r'))
+    events = read_events(open(sys.argv[1], 'r'), sys.argv[1])
     process(events, sys.argv[2], analyzer, read_header=read_header)
 
 if __name__ == '__main__':
diff --git a/scripts/tracetool.py b/scripts/tracetool.py
index c55a21518b..fe2b0771f2 100755
--- a/scripts/tracetool.py
+++ b/scripts/tracetool.py
@@ -142,7 +142,7 @@ def main(args):
     events = []
     for arg in args:
         with open(arg, "r") as fh:
-            events.extend(tracetool.read_events(fh))
+            events.extend(tracetool.read_events(fh, arg))
 
     try:
         tracetool.generate(events, arg_group, arg_format, arg_backends,
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 3646c2b9fc..4236062650 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -291,13 +291,15 @@ class Event(object):
                      self)
 
 
-def read_events(fobj):
+def read_events(fobj, fname):
     """Generate the output for the given (format, backends) pair.
 
     Parameters
     ----------
     fobj : file
         Event description file.
+    fname : str
+        Name of event file
 
     Returns a list of Event objects
     """
@@ -312,7 +314,7 @@ def read_events(fobj):
         try:
             event = Event.build(line)
         except ValueError as e:
-            arg0 = 'Error on line %d: %s' % (lineno, e.args[0])
+            arg0 = 'Error at %s:%d: %s' % (fname, lineno, e.args[0])
             e.args = (arg0,) + e.args[1:]
             raise
 
-- 
2.14.3

[Qemu-devel] [PULL 4/5] trace: remove use of QEMU specific types from trace probes

[Stefan Hajnoczi]

From: Daniel P. Berrangé <berrange@redhat.com>

Any compound structs / unions / etc, should always be declared as
'void *' pointers, since it cannot be assumed that trace backends
are able to resolve QEMU typedefs.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20180308155524.5082-2-berrange@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 trace-events | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/trace-events b/trace-events
index 89fcad0fd1..855b0ab240 100644
--- a/trace-events
+++ b/trace-events
@@ -68,9 +68,9 @@ memory_region_tb_read(int cpu_index, uint64_t addr, uint64_t value, unsigned siz
 memory_region_tb_write(int cpu_index, uint64_t addr, uint64_t value, unsigned size) "cpu %d addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
-flatview_new(FlatView *view, MemoryRegion *root) "%p (root %p)"
-flatview_destroy(FlatView *view, MemoryRegion *root) "%p (root %p)"
-flatview_destroy_rcu(FlatView *view, MemoryRegion *root) "%p (root %p)"
+flatview_new(void *view, void *root) "%p (root %p)"
+flatview_destroy(void *view, void *root) "%p (root %p)"
+flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
 
 # gdbstub.c
 gdbstub_op_start(const char *device) "Starting gdbstub using device %s"
-- 
2.14.3

[Qemu-devel] [PULL 5/5] trace: only permit standard C types and fixed size integer types

[Stefan Hajnoczi]

From: Daniel P. Berrangé <berrange@redhat.com>

Some trace backends will compile code based on the declared trace
events. It should not be assumed that the backends can resolve any QEMU
specific typedefs. So trace events should restrict their argument
types to the standard C types and fixed size integer types. Any complex
pointer types can be declared as "void *" for purposes of trace events,
since nothing will be dereferencing these pointer arguments.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20180308155524.5082-3-berrange@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/__init__.py | 46 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 4236062650..b20fac34a3 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -41,6 +41,51 @@ def out(*lines, **kwargs):
     lines = [ l % kwargs for l in lines ]
     sys.stdout.writelines("\n".join(lines) + "\n")
 
+# We only want to allow standard C types or fixed sized
+# integer types. We don't want QEMU specific types
+# as we can't assume trace backends can resolve all the
+# typedefs
+ALLOWED_TYPES = [
+    "int",
+    "long",
+    "short",
+    "char",
+    "bool",
+    "unsigned",
+    "signed",
+    "float",
+    "double",
+    "int8_t",
+    "uint8_t",
+    "int16_t",
+    "uint16_t",
+    "int32_t",
+    "uint32_t",
+    "int64_t",
+    "uint64_t",
+    "void",
+    "size_t",
+    "ssize_t",
+    "uintptr_t",
+    "ptrdiff_t",
+    # Magic substitution is done by tracetool
+    "TCGv",
+]
+
+def validate_type(name):
+    bits = name.split(" ")
+    for bit in bits:
+        bit = re.sub("\*", "", bit)
+        if bit == "":
+            continue
+        if bit == "const":
+            continue
+        if bit not in ALLOWED_TYPES:
+            raise ValueError("Argument type '%s' is not in whitelist. "
+                             "Only standard C types and fixed size integer "
+                             "types should be used. struct, union, and "
+                             "other complex pointer types should be "
+                             "declared as 'void *'" % name)
 
 class Arguments:
     """Event arguments description."""
@@ -87,6 +132,7 @@ class Arguments:
             else:
                 arg_type, identifier = arg.rsplit(None, 1)
 
+            validate_type(arg_type)
             res.append((arg_type, identifier))
         return Arguments(res)
 
-- 
2.14.3

Re: [Qemu-devel] [PULL 0/5] Tracing patches

[Peter Maydell]

On 12 March 2018 at 16:00, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The following changes since commit e4ae62b802cec437f877f2cadc4ef059cc0eca76:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2018-03-09 17:28:16 +0000)
>
> are available in the Git repository at:
>
>   git://github.com/stefanha/qemu.git tags/tracing-pull-request
>
> for you to fetch changes up to 73ff061032efa0b260bcd1a177ac89b57a1642d3:
>
>   trace: only permit standard C types and fixed size integer types (2018-03-12 11:10:20 +0000)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
> Daniel P. Berrangé (3):
>   trace: include filename when printing parser error messages
>   trace: remove use of QEMU specific types from trace probes
>   trace: only permit standard C types and fixed size integer types
>
> Peter Maydell (1):
>   log-for-trace.h: Split out parts of log.h used by trace.h
>
> Stefan Hajnoczi (1):
>   simpletrace: fix timestamp argument type
>

Applied, thanks.

-- PMM