* [PATCH v3 1/8] checkpatch: cull trailing '*/' in SPDX check
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
@ 2025-08-06 16:48 ` Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 2/8] tracetool: eliminate trailing whitespace in C format Daniel P. Berrangé
` (7 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-06 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, Daniel P. Berrangé
Sometimes SPDX expressions appear inside C comments, and this
confuses checkpatch.pl. Drop the closing C comment characters
to avoid this.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
scripts/checkpatch.pl | 3 +++
1 file changed, 3 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 833f20f555..91616c974f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1368,6 +1368,9 @@ sub checkspdx {
$expr =~ s/^\s*//g;
$expr =~ s/\s*$//g;
+ # Cull C comment end
+ $expr =~ s/\*\/.*//;
+
my @bits = split / +/, $expr;
my $prefer = "GPL-2.0-or-later";
--
2.50.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 2/8] tracetool: eliminate trailing whitespace in C format
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 1/8] checkpatch: cull trailing '*/' in SPDX check Daniel P. Berrangé
@ 2025-08-06 16:48 ` Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 3/8] tracetool: avoid space after "*" in arg types Daniel P. Berrangé
` (6 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-06 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, Daniel P. Berrangé
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
scripts/tracetool/format/c.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/tracetool/format/c.py b/scripts/tracetool/format/c.py
index 69edf0d588..7aa51cd41a 100644
--- a/scripts/tracetool/format/c.py
+++ b/scripts/tracetool/format/c.py
@@ -36,7 +36,7 @@ def generate(events, backend, group):
' .id = 0,',
' .name = \"%(name)s\",',
' .sstate = %(sstate)s,',
- ' .dstate = &%(dstate)s ',
+ ' .dstate = &%(dstate)s',
'};',
event = e.api(e.QEMU_EVENT),
name = e.name,
--
2.50.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 3/8] tracetool: avoid space after "*" in arg types
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 1/8] checkpatch: cull trailing '*/' in SPDX check Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 2/8] tracetool: eliminate trailing whitespace in C format Daniel P. Berrangé
@ 2025-08-06 16:48 ` Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 4/8] tracetool: include SPDX-License-Identifier in generated files Daniel P. Berrangé
` (5 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-06 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, Daniel P. Berrangé
QEMU code style is to have no whitespace between "*" and the
arg name. Since generated trace code will soon be added to
git, make it comply with code style.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
scripts/tracetool/__init__.py | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 2ae2e562d6..0f33758870 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -170,10 +170,16 @@ def __len__(self):
def __str__(self):
"""String suitable for declaring function arguments."""
+ def onearg(t, n):
+ if t[-1] == '*':
+ return "".join([t, n])
+ else:
+ return " ".join([t, n])
+
if len(self._args) == 0:
return "void"
else:
- return ", ".join([ " ".join([t, n]) for t,n in self._args ])
+ return ", ".join([ onearg(t, n) for t,n in self._args ])
def __repr__(self):
"""Evaluable string representation for this object."""
--
2.50.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 4/8] tracetool: include SPDX-License-Identifier in generated files
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
` (2 preceding siblings ...)
2025-08-06 16:48 ` [PATCH v3 3/8] tracetool: avoid space after "*" in arg types Daniel P. Berrangé
@ 2025-08-06 16:48 ` Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout Daniel P. Berrangé
` (4 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-06 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, Daniel P. Berrangé
While these files are auto-generated, a later commit will add
reference output to git, so having SPDX-License-Identifier is
desirable.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
scripts/tracetool/format/c.py | 1 +
scripts/tracetool/format/d.py | 3 ++-
scripts/tracetool/format/h.py | 1 +
scripts/tracetool/format/log_stap.py | 1 +
scripts/tracetool/format/simpletrace_stap.py | 1 +
scripts/tracetool/format/stap.py | 1 +
scripts/tracetool/format/ust_events_c.py | 1 +
scripts/tracetool/format/ust_events_h.py | 1 +
8 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/tracetool/format/c.py b/scripts/tracetool/format/c.py
index 7aa51cd41a..e473fb6c6e 100644
--- a/scripts/tracetool/format/c.py
+++ b/scripts/tracetool/format/c.py
@@ -22,6 +22,7 @@ def generate(events, backend, group):
header = "trace-" + group + ".h"
out('/* This file is autogenerated by tracetool, do not edit. */',
+ '/* SPDX-License-Identifier: GPL-2.0-or-later */',
'',
'#include "qemu/osdep.h"',
'#include "qemu/module.h"',
diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
index ebfb714200..a5e096e214 100644
--- a/scripts/tracetool/format/d.py
+++ b/scripts/tracetool/format/d.py
@@ -39,7 +39,8 @@ def generate(events, backend, group):
if not events and platform != "darwin":
return
- out('/* This file is autogenerated by tracetool, do not edit. */'
+ out('/* This file is autogenerated by tracetool, do not edit. */',
+ '/* SPDX-License-Identifier: GPL-2.0-or-later */',
'',
'provider qemu {')
diff --git a/scripts/tracetool/format/h.py b/scripts/tracetool/format/h.py
index ea126b07ea..a00ae475f7 100644
--- a/scripts/tracetool/format/h.py
+++ b/scripts/tracetool/format/h.py
@@ -19,6 +19,7 @@ def generate(events, backend, group):
header = "trace/control.h"
out('/* This file is autogenerated by tracetool, do not edit. */',
+ '/* SPDX-License-Identifier: GPL-2.0-or-later */',
'',
'#ifndef TRACE_%s_GENERATED_TRACERS_H' % group.upper(),
'#define TRACE_%s_GENERATED_TRACERS_H' % group.upper(),
diff --git a/scripts/tracetool/format/log_stap.py b/scripts/tracetool/format/log_stap.py
index b49afababd..710d62bffe 100644
--- a/scripts/tracetool/format/log_stap.py
+++ b/scripts/tracetool/format/log_stap.py
@@ -88,6 +88,7 @@ def c_fmt_to_stap(fmt):
def generate(events, backend, group):
out('/* This file is autogenerated by tracetool, do not edit. */',
+ '/* SPDX-License-Identifier: GPL-2.0-or-later */',
'')
for event_id, e in enumerate(events):
diff --git a/scripts/tracetool/format/simpletrace_stap.py b/scripts/tracetool/format/simpletrace_stap.py
index 4f4633b4e6..72971133bf 100644
--- a/scripts/tracetool/format/simpletrace_stap.py
+++ b/scripts/tracetool/format/simpletrace_stap.py
@@ -22,6 +22,7 @@ def global_var_name(name):
def generate(events, backend, group):
out('/* This file is autogenerated by tracetool, do not edit. */',
+ '/* SPDX-License-Identifier: GPL-2.0-or-later */',
'')
for event_id, e in enumerate(events):
diff --git a/scripts/tracetool/format/stap.py b/scripts/tracetool/format/stap.py
index a218b0445c..4d77fbc11a 100644
--- a/scripts/tracetool/format/stap.py
+++ b/scripts/tracetool/format/stap.py
@@ -38,6 +38,7 @@ def generate(events, backend, group):
if "disable" not in e.properties]
out('/* This file is autogenerated by tracetool, do not edit. */',
+ '/* SPDX-License-Identifier: GPL-2.0-or-later */',
'')
for e in events:
diff --git a/scripts/tracetool/format/ust_events_c.py b/scripts/tracetool/format/ust_events_c.py
index deced9533d..569754a304 100644
--- a/scripts/tracetool/format/ust_events_c.py
+++ b/scripts/tracetool/format/ust_events_c.py
@@ -20,6 +20,7 @@ def generate(events, backend, group):
if "disabled" not in e.properties]
out('/* This file is autogenerated by tracetool, do not edit. */',
+ '/* SPDX-License-Identifier: GPL-2.0-or-later */',
'',
'#include "qemu/osdep.h"',
'',
diff --git a/scripts/tracetool/format/ust_events_h.py b/scripts/tracetool/format/ust_events_h.py
index b99fe6896b..2a31fefeca 100644
--- a/scripts/tracetool/format/ust_events_h.py
+++ b/scripts/tracetool/format/ust_events_h.py
@@ -25,6 +25,7 @@ def generate(events, backend, group):
include = "trace-ust.h"
out('/* This file is autogenerated by tracetool, do not edit. */',
+ '/* SPDX-License-Identifier: GPL-2.0-or-later */',
'',
'#undef TRACEPOINT_PROVIDER',
'#define TRACEPOINT_PROVIDER qemu',
--
2.50.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
` (3 preceding siblings ...)
2025-08-06 16:48 ` [PATCH v3 4/8] tracetool: include SPDX-License-Identifier in generated files Daniel P. Berrangé
@ 2025-08-06 16:48 ` Daniel P. Berrangé
2025-08-07 19:46 ` Stefan Hajnoczi
2025-08-06 16:48 ` [PATCH v3 6/8] tracetool: add test suite for tracetool with reference output Daniel P. Berrangé
` (3 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-06 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, Daniel P. Berrangé
This avoids callers needing to use the UNIX-only /dev/stdout
workaround.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
scripts/tracetool/__init__.py | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 0f33758870..c8fd3a7ddc 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -38,8 +38,12 @@ def error(*lines):
def out_open(filename):
global out_filename, out_fobj
- out_filename = posix_relpath(filename)
- out_fobj = open(filename, 'wt')
+ if filename == "-":
+ out_filename = "[stdout]"
+ out_fobj = sys.stdout
+ else:
+ out_filename = posix_relpath(filename)
+ out_fobj = open(filename, 'wt')
def out(*lines, **kwargs):
"""Write a set of output lines.
--
2.50.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
2025-08-06 16:48 ` [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout Daniel P. Berrangé
@ 2025-08-07 19:46 ` Stefan Hajnoczi
2025-08-18 15:07 ` Daniel P. Berrangé
0 siblings, 1 reply; 18+ messages in thread
From: Stefan Hajnoczi @ 2025-08-07 19:46 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, oleg.tolmatcev
[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]
On Wed, Aug 06, 2025 at 05:48:29PM +0100, Daniel P. Berrangé wrote:
> This avoids callers needing to use the UNIX-only /dev/stdout
> workaround.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> scripts/tracetool/__init__.py | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> index 0f33758870..c8fd3a7ddc 100644
> --- a/scripts/tracetool/__init__.py
> +++ b/scripts/tracetool/__init__.py
> @@ -38,8 +38,12 @@ def error(*lines):
>
> def out_open(filename):
> global out_filename, out_fobj
> - out_filename = posix_relpath(filename)
> - out_fobj = open(filename, 'wt')
> + if filename == "-":
> + out_filename = "[stdout]"
A few lines above:
out_filename = '<none>'
out_fobj = sys.stdout
Stick to '<none>' here for consistency?
> + out_fobj = sys.stdout
> + else:
> + out_filename = posix_relpath(filename)
I have CCed Oleg in case he spots any portability issues, but I think
this should still work on Windows.
> + out_fobj = open(filename, 'wt')
>
> def out(*lines, **kwargs):
> """Write a set of output lines.
> --
> 2.50.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
2025-08-07 19:46 ` Stefan Hajnoczi
@ 2025-08-18 15:07 ` Daniel P. Berrangé
2025-08-18 17:55 ` Stefan Hajnoczi
0 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-18 15:07 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, oleg.tolmatcev
On Thu, Aug 07, 2025 at 03:46:08PM -0400, Stefan Hajnoczi wrote:
> On Wed, Aug 06, 2025 at 05:48:29PM +0100, Daniel P. Berrangé wrote:
> > This avoids callers needing to use the UNIX-only /dev/stdout
> > workaround.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > scripts/tracetool/__init__.py | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> > index 0f33758870..c8fd3a7ddc 100644
> > --- a/scripts/tracetool/__init__.py
> > +++ b/scripts/tracetool/__init__.py
> > @@ -38,8 +38,12 @@ def error(*lines):
> >
> > def out_open(filename):
> > global out_filename, out_fobj
> > - out_filename = posix_relpath(filename)
> > - out_fobj = open(filename, 'wt')
> > + if filename == "-":
> > + out_filename = "[stdout]"
>
> A few lines above:
>
> out_filename = '<none>'
> out_fobj = sys.stdout
>
> Stick to '<none>' here for consistency?
Curious - that suggests that it was intended to be able to write to
stdout by default, but tracetool.py unconditionally calls out_open()
so those default assignments are effectively dead code, unless this
internal code is called by something other than the tracetool.py main
entrypoint ?
I guess I'd be inclined to change the global initialization to just
be 'None' to make it explicit that out_open is expected to always be
called ?
> > + out_fobj = sys.stdout
> > + else:
> > + out_filename = posix_relpath(filename)
>
> I have CCed Oleg in case he spots any portability issues, but I think
> this should still work on Windows.
This use of posix_relpath() was pre-existing, so there shouldn't be
any new issues from this.
>
> > + out_fobj = open(filename, 'wt')
> >
> > def out(*lines, **kwargs):
> > """Write a set of output lines.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
2025-08-18 15:07 ` Daniel P. Berrangé
@ 2025-08-18 17:55 ` Stefan Hajnoczi
2025-08-19 15:58 ` Daniel P. Berrangé
0 siblings, 1 reply; 18+ messages in thread
From: Stefan Hajnoczi @ 2025-08-18 17:55 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, oleg.tolmatcev
[-- Attachment #1: Type: text/plain, Size: 3271 bytes --]
On Mon, Aug 18, 2025 at 04:07:28PM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 07, 2025 at 03:46:08PM -0400, Stefan Hajnoczi wrote:
> > On Wed, Aug 06, 2025 at 05:48:29PM +0100, Daniel P. Berrangé wrote:
> > > This avoids callers needing to use the UNIX-only /dev/stdout
> > > workaround.
> > >
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > > scripts/tracetool/__init__.py | 8 ++++++--
> > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> > > index 0f33758870..c8fd3a7ddc 100644
> > > --- a/scripts/tracetool/__init__.py
> > > +++ b/scripts/tracetool/__init__.py
> > > @@ -38,8 +38,12 @@ def error(*lines):
> > >
> > > def out_open(filename):
> > > global out_filename, out_fobj
> > > - out_filename = posix_relpath(filename)
> > > - out_fobj = open(filename, 'wt')
> > > + if filename == "-":
> > > + out_filename = "[stdout]"
> >
> > A few lines above:
> >
> > out_filename = '<none>'
> > out_fobj = sys.stdout
> >
> > Stick to '<none>' here for consistency?
>
> Curious - that suggests that it was intended to be able to write to
> stdout by default, but tracetool.py unconditionally calls out_open()
> so those default assignments are effectively dead code, unless this
> internal code is called by something other than the tracetool.py main
> entrypoint ?
>
> I guess I'd be inclined to change the global initialization to just
> be 'None' to make it explicit that out_open is expected to always be
> called ?
Originally the script wrote to stdout, but I added an explicit output
filename argument in commit c05012a365c2 ("tracetool: add output
filename command-line argument") because #line directives emitted by
tracetool need to know the output filename.
Your next patch tests/tracetool/tracetool-test.py uses "-" as the
output filename but leaves the existing meson.build files unchanged.
They will still specify an output filename.
This commit doesn't break anything, at least not in how this patch
series uses "-", but I see a contradiction with commit c05012a365c2
since we're now allowing the output filename to be effectively empty.
Could you avoid special casing stdout and instead pass a relative path
to the output file? The relative path is important so the test reference
output is portable across machines. Then you don't need this commit.
Stefan
>
> > > + out_fobj = sys.stdout
> > > + else:
> > > + out_filename = posix_relpath(filename)
> >
> > I have CCed Oleg in case he spots any portability issues, but I think
> > this should still work on Windows.
>
> This use of posix_relpath() was pre-existing, so there shouldn't be
> any new issues from this.
>
> >
> > > + out_fobj = open(filename, 'wt')
> > >
> > > def out(*lines, **kwargs):
> > > """Write a set of output lines.
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
2025-08-18 17:55 ` Stefan Hajnoczi
@ 2025-08-19 15:58 ` Daniel P. Berrangé
0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-19 15:58 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, oleg.tolmatcev
On Mon, Aug 18, 2025 at 01:55:48PM -0400, Stefan Hajnoczi wrote:
> On Mon, Aug 18, 2025 at 04:07:28PM +0100, Daniel P. Berrangé wrote:
> > On Thu, Aug 07, 2025 at 03:46:08PM -0400, Stefan Hajnoczi wrote:
> > > On Wed, Aug 06, 2025 at 05:48:29PM +0100, Daniel P. Berrangé wrote:
> > > > This avoids callers needing to use the UNIX-only /dev/stdout
> > > > workaround.
> > > >
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > ---
> > > > scripts/tracetool/__init__.py | 8 ++++++--
> > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> > > > index 0f33758870..c8fd3a7ddc 100644
> > > > --- a/scripts/tracetool/__init__.py
> > > > +++ b/scripts/tracetool/__init__.py
> > > > @@ -38,8 +38,12 @@ def error(*lines):
> > > >
> > > > def out_open(filename):
> > > > global out_filename, out_fobj
> > > > - out_filename = posix_relpath(filename)
> > > > - out_fobj = open(filename, 'wt')
> > > > + if filename == "-":
> > > > + out_filename = "[stdout]"
> > >
> > > A few lines above:
> > >
> > > out_filename = '<none>'
> > > out_fobj = sys.stdout
> > >
> > > Stick to '<none>' here for consistency?
> >
> > Curious - that suggests that it was intended to be able to write to
> > stdout by default, but tracetool.py unconditionally calls out_open()
> > so those default assignments are effectively dead code, unless this
> > internal code is called by something other than the tracetool.py main
> > entrypoint ?
> >
> > I guess I'd be inclined to change the global initialization to just
> > be 'None' to make it explicit that out_open is expected to always be
> > called ?
>
> Originally the script wrote to stdout, but I added an explicit output
> filename argument in commit c05012a365c2 ("tracetool: add output
> filename command-line argument") because #line directives emitted by
> tracetool need to know the output filename.
>
> Your next patch tests/tracetool/tracetool-test.py uses "-" as the
> output filename but leaves the existing meson.build files unchanged.
> They will still specify an output filename.
>
> This commit doesn't break anything, at least not in how this patch
> series uses "-", but I see a contradiction with commit c05012a365c2
> since we're now allowing the output filename to be effectively empty.
>
> Could you avoid special casing stdout and instead pass a relative path
> to the output file? The relative path is important so the test reference
> output is portable across machines. Then you don't need this commit.
If I copy the 'trace-events' file into the build-dir, then I can rely
on relative files for both the input & output, and avoid need to
support '-'.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 6/8] tracetool: add test suite for tracetool with reference output
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
` (4 preceding siblings ...)
2025-08-06 16:48 ` [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout Daniel P. Berrangé
@ 2025-08-06 16:48 ` Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 7/8] tracetool: drop the probe "__nocheck__" wrapping Daniel P. Berrangé
` (2 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-06 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, Daniel P. Berrangé
When reviewing tracetool patches it is often very unclear what the
expected output will be for the generated backends. Compounding
this is that a default build will only enable the 'log' trace
backend, so developers won't see generated code for other backends
without making a special effort. Some backends are also platform
specific, so can't be enabled in QEMU builds, even though tracetool
could generate the code.
To address this, introduce a test suite for tracetool which is
conceptually similar to the qapi-schema test. It is a simple
python program that runs tracetool and compares the actual output
to historical reference output kept in git. The test directly
emits TAP format logs for ease of integration with meson.
This can be run with
make check-tracetool
to make it easier for developers changing generated output, the
sample expected content can be auto-recreated
QEMU_TEST_REGENERATE=1 make check-tracetool
and the changes reviewed and added to the commit. This will also
assist reviewers interpreting the change.
Developers are reminded of this in the test output on failure:
$ make check-tracetool
1/6 qemu:tracetool / dtrace OK 0.14s
2/6 qemu:tracetool / ftrace FAIL 0.06s exit status 1
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
1..2
ok 1 - ftrace.c
#
not ok 1 - ftrace.h (set QEMU_TEST_REGENERATE=1 to recreate reference output if tracetool generator was intentionally changed)
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
3/6 qemu:tracetool / log OK 0.06s
4/6 qemu:tracetool / simple OK 0.06s
5/6 qemu:tracetool / syslog OK 0.06s
6/6 qemu:tracetool / ust OK 0.11s
Summary of Failures:
2/6 qemu:tracetool / ftrace FAIL 0.06s exit status 1
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
MAINTAINERS | 1 +
docs/devel/testing/main.rst | 28 ++++++++
tests/Makefile.include | 1 +
tests/meson.build | 1 +
tests/tracetool/dtrace.c | 32 +++++++++
tests/tracetool/dtrace.d | 10 +++
tests/tracetool/dtrace.h | 59 ++++++++++++++++
tests/tracetool/dtrace.log-stap | 15 ++++
tests/tracetool/dtrace.simpletrace-stap | 16 +++++
tests/tracetool/dtrace.stap | 14 ++++
tests/tracetool/ftrace.c | 32 +++++++++
tests/tracetool/ftrace.h | 73 ++++++++++++++++++++
tests/tracetool/log.c | 32 +++++++++
tests/tracetool/log.h | 57 ++++++++++++++++
tests/tracetool/meson.build | 25 +++++++
tests/tracetool/simple.c | 61 +++++++++++++++++
tests/tracetool/simple.h | 54 +++++++++++++++
tests/tracetool/syslog.c | 32 +++++++++
tests/tracetool/syslog.h | 57 ++++++++++++++++
tests/tracetool/trace-events | 5 ++
tests/tracetool/tracetool-test.py | 91 +++++++++++++++++++++++++
tests/tracetool/ust.c | 32 +++++++++
tests/tracetool/ust.h | 55 +++++++++++++++
tests/tracetool/ust.ust-events-c | 14 ++++
tests/tracetool/ust.ust-events-h | 56 +++++++++++++++
25 files changed, 853 insertions(+)
create mode 100644 tests/tracetool/dtrace.c
create mode 100644 tests/tracetool/dtrace.d
create mode 100644 tests/tracetool/dtrace.h
create mode 100644 tests/tracetool/dtrace.log-stap
create mode 100644 tests/tracetool/dtrace.simpletrace-stap
create mode 100644 tests/tracetool/dtrace.stap
create mode 100644 tests/tracetool/ftrace.c
create mode 100644 tests/tracetool/ftrace.h
create mode 100644 tests/tracetool/log.c
create mode 100644 tests/tracetool/log.h
create mode 100644 tests/tracetool/meson.build
create mode 100644 tests/tracetool/simple.c
create mode 100644 tests/tracetool/simple.h
create mode 100644 tests/tracetool/syslog.c
create mode 100644 tests/tracetool/syslog.h
create mode 100644 tests/tracetool/trace-events
create mode 100755 tests/tracetool/tracetool-test.py
create mode 100644 tests/tracetool/ust.c
create mode 100644 tests/tracetool/ust.h
create mode 100644 tests/tracetool/ust.ust-events-c
create mode 100644 tests/tracetool/ust.ust-events-h
diff --git a/MAINTAINERS b/MAINTAINERS
index 28cea34271..c08c51f4c6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3556,6 +3556,7 @@ F: scripts/tracetool/
F: scripts/qemu-trace-stap*
F: docs/tools/qemu-trace-stap.rst
F: docs/devel/tracing.rst
+F: tests/tracetool/
T: git https://github.com/stefanha/qemu.git tracing
Simpletrace
diff --git a/docs/devel/testing/main.rst b/docs/devel/testing/main.rst
index 2b5cb0c148..11f05c0006 100644
--- a/docs/devel/testing/main.rst
+++ b/docs/devel/testing/main.rst
@@ -178,6 +178,34 @@ parser (either fixing a bug or extending/modifying the syntax). To do this:
``qapi-schema += foo.json``
+.. _tracetool-tests:
+
+Tracetool tests
+~~~~~~~~~~~~~~~
+
+The tracetool tests validate the generated source files used for defining
+probes for various tracing backends and source formats. The test operates
+by running the tracetool program against a sample trace-events file, and
+comparing the generated output against known good reference output. The
+tests can be run with:
+
+.. code::
+
+ make check-tracetool
+
+The reference output is stored in files under tests/tracetool, and when
+the tracetool backend/format output is intentionally changed, the reference
+files need to be updated. This can be automated by setting the
+QEMU_TEST_REGENERATE=1 environment variable:
+
+.. code::
+
+ QEMU_TEST_REGENERATE=1 make check-tracetool
+
+The resulting changes must be reviewed by the author to ensure they match
+the intended results, before adding the updated reference output to the
+same commit that alters the generator code.
+
check-block
~~~~~~~~~~~
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 23fb722d42..3538c0c740 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -13,6 +13,7 @@ check-help:
@echo " $(MAKE) check-functional-TARGET Run functional tests for a given target"
@echo " $(MAKE) check-unit Run qobject tests"
@echo " $(MAKE) check-qapi-schema Run QAPI schema tests"
+ @echo " $(MAKE) check-tracetool Run tracetool generator tests"
@echo " $(MAKE) check-block Run block tests"
ifneq ($(filter $(all-check-targets), check-softfloat),)
@echo " $(MAKE) check-tcg Run TCG tests"
diff --git a/tests/meson.build b/tests/meson.build
index c59619220f..cbe7916241 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -88,3 +88,4 @@ subdir('qapi-schema')
subdir('qtest')
subdir('migration-stress')
subdir('functional')
+subdir('tracetool')
diff --git a/tests/tracetool/dtrace.c b/tests/tracetool/dtrace.c
new file mode 100644
index 0000000000..9f862fa14d
--- /dev/null
+++ b/tests/tracetool/dtrace.c
@@ -0,0 +1,32 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "trace-testsuite.h"
+
+uint16_t _TRACE_TEST_BLAH_DSTATE;
+uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+TraceEvent _TRACE_TEST_BLAH_EVENT = {
+ .id = 0,
+ .name = "test_blah",
+ .sstate = TRACE_TEST_BLAH_ENABLED,
+ .dstate = &_TRACE_TEST_BLAH_DSTATE
+};
+TraceEvent _TRACE_TEST_WIBBLE_EVENT = {
+ .id = 0,
+ .name = "test_wibble",
+ .sstate = TRACE_TEST_WIBBLE_ENABLED,
+ .dstate = &_TRACE_TEST_WIBBLE_DSTATE
+};
+TraceEvent *testsuite_trace_events[] = {
+ &_TRACE_TEST_BLAH_EVENT,
+ &_TRACE_TEST_WIBBLE_EVENT,
+ NULL,
+};
+
+static void trace_testsuite_register_events(void)
+{
+ trace_event_register_group(testsuite_trace_events);
+}
+trace_init(trace_testsuite_register_events)
diff --git a/tests/tracetool/dtrace.d b/tests/tracetool/dtrace.d
new file mode 100644
index 0000000000..5cc06f9f4f
--- /dev/null
+++ b/tests/tracetool/dtrace.d
@@ -0,0 +1,10 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+provider qemu {
+
+probe test_blah(void * context,const char * filename);
+
+probe test_wibble(void * context,int value);
+
+};
diff --git a/tests/tracetool/dtrace.h b/tests/tracetool/dtrace.h
new file mode 100644
index 0000000000..c2e5110672
--- /dev/null
+++ b/tests/tracetool/dtrace.h
@@ -0,0 +1,59 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef TRACE_TESTSUITE_GENERATED_TRACERS_H
+#define TRACE_TESTSUITE_GENERATED_TRACERS_H
+
+#include "trace/control.h"
+
+extern TraceEvent _TRACE_TEST_BLAH_EVENT;
+extern TraceEvent _TRACE_TEST_WIBBLE_EVENT;
+extern uint16_t _TRACE_TEST_BLAH_DSTATE;
+extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+#define TRACE_TEST_BLAH_ENABLED 1
+#define TRACE_TEST_WIBBLE_ENABLED 1
+#ifndef SDT_USE_VARIADIC
+#define SDT_USE_VARIADIC 1
+#endif
+#include "trace-dtrace-testsuite.h"
+
+#undef SDT_USE_VARIADIC
+#ifndef QEMU_TEST_BLAH_ENABLED
+#define QEMU_TEST_BLAH_ENABLED() true
+#endif
+#ifndef QEMU_TEST_WIBBLE_ENABLED
+#define QEMU_TEST_WIBBLE_ENABLED() true
+#endif
+
+#define TRACE_TEST_BLAH_BACKEND_DSTATE() ( \
+ QEMU_TEST_BLAH_ENABLED() || \
+ false)
+
+static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+{
+ QEMU_TEST_BLAH(context, filename);
+}
+
+static inline void trace_test_blah(void *context, const char *filename)
+{
+ if (true) {
+ _nocheck__trace_test_blah(context, filename);
+ }
+}
+
+#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
+ QEMU_TEST_WIBBLE_ENABLED() || \
+ false)
+
+static inline void _nocheck__trace_test_wibble(void *context, int value)
+{
+ QEMU_TEST_WIBBLE(context, value);
+}
+
+static inline void trace_test_wibble(void *context, int value)
+{
+ if (true) {
+ _nocheck__trace_test_wibble(context, value);
+ }
+}
+#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/dtrace.log-stap b/tests/tracetool/dtrace.log-stap
new file mode 100644
index 0000000000..092986e0b6
--- /dev/null
+++ b/tests/tracetool/dtrace.log-stap
@@ -0,0 +1,15 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+probe qemu.log.test_blah = qemu.test_blah ?
+{
+ try {
+ argfilename_str = filename ? user_string_n(filename, 512) : "<null>"
+ } catch {}
+ printf("%d@%d test_blah Blah context=%p filename=%s\n", pid(), gettimeofday_ns(), context, argfilename_str)
+}
+probe qemu.log.test_wibble = qemu.test_wibble ?
+{
+ printf("%d@%d test_wibble Wibble context=%p value=%d\n", pid(), gettimeofday_ns(), context, value)
+}
+
diff --git a/tests/tracetool/dtrace.simpletrace-stap b/tests/tracetool/dtrace.simpletrace-stap
new file mode 100644
index 0000000000..d064e3e286
--- /dev/null
+++ b/tests/tracetool/dtrace.simpletrace-stap
@@ -0,0 +1,16 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+probe qemu.simpletrace.test_blah = qemu.test_blah ?
+{
+ try {
+ argfilename_str = filename ? user_string_n(filename, 512) : "<null>"
+ } catch {}
+ argfilename_len = strlen(argfilename_str)
+ printf("%8b%8b%8b%4b%4b%8b%4b%.*s", 1, 0, gettimeofday_ns(), 24 + 8 + 4 + argfilename_len, pid(), context, argfilename_len, argfilename_len, argfilename_str)
+}
+probe qemu.simpletrace.test_wibble = qemu.test_wibble ?
+{
+ printf("%8b%8b%8b%4b%4b%8b%8b", 1, 1, gettimeofday_ns(), 24 + 8 + 8, pid(), context, value)
+}
+
diff --git a/tests/tracetool/dtrace.stap b/tests/tracetool/dtrace.stap
new file mode 100644
index 0000000000..9c5d8a527c
--- /dev/null
+++ b/tests/tracetool/dtrace.stap
@@ -0,0 +1,14 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+probe qemu.test_blah = process("qemu").mark("test_blah")
+{
+ context = $arg1;
+ filename = $arg2;
+}
+probe qemu.test_wibble = process("qemu").mark("test_wibble")
+{
+ context = $arg1;
+ value = $arg2;
+}
+
diff --git a/tests/tracetool/ftrace.c b/tests/tracetool/ftrace.c
new file mode 100644
index 0000000000..9f862fa14d
--- /dev/null
+++ b/tests/tracetool/ftrace.c
@@ -0,0 +1,32 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "trace-testsuite.h"
+
+uint16_t _TRACE_TEST_BLAH_DSTATE;
+uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+TraceEvent _TRACE_TEST_BLAH_EVENT = {
+ .id = 0,
+ .name = "test_blah",
+ .sstate = TRACE_TEST_BLAH_ENABLED,
+ .dstate = &_TRACE_TEST_BLAH_DSTATE
+};
+TraceEvent _TRACE_TEST_WIBBLE_EVENT = {
+ .id = 0,
+ .name = "test_wibble",
+ .sstate = TRACE_TEST_WIBBLE_ENABLED,
+ .dstate = &_TRACE_TEST_WIBBLE_DSTATE
+};
+TraceEvent *testsuite_trace_events[] = {
+ &_TRACE_TEST_BLAH_EVENT,
+ &_TRACE_TEST_WIBBLE_EVENT,
+ NULL,
+};
+
+static void trace_testsuite_register_events(void)
+{
+ trace_event_register_group(testsuite_trace_events);
+}
+trace_init(trace_testsuite_register_events)
diff --git a/tests/tracetool/ftrace.h b/tests/tracetool/ftrace.h
new file mode 100644
index 0000000000..b4daa96fe7
--- /dev/null
+++ b/tests/tracetool/ftrace.h
@@ -0,0 +1,73 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef TRACE_TESTSUITE_GENERATED_TRACERS_H
+#define TRACE_TESTSUITE_GENERATED_TRACERS_H
+
+#include "trace/control.h"
+
+extern TraceEvent _TRACE_TEST_BLAH_EVENT;
+extern TraceEvent _TRACE_TEST_WIBBLE_EVENT;
+extern uint16_t _TRACE_TEST_BLAH_DSTATE;
+extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+#define TRACE_TEST_BLAH_ENABLED 1
+#define TRACE_TEST_WIBBLE_ENABLED 1
+#include "trace/ftrace.h"
+
+
+#define TRACE_TEST_BLAH_BACKEND_DSTATE() ( \
+ trace_event_get_state_dynamic_by_id(TRACE_TEST_BLAH) || \
+ false)
+
+static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+{
+ {
+ char ftrace_buf[MAX_TRACE_STRLEN];
+ int unused __attribute__ ((unused));
+ int trlen;
+ if (trace_event_get_state(TRACE_TEST_BLAH)) {
+#line 4 "trace-events"
+ trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,
+ "test_blah " "Blah context=%p filename=%s" "\n" , context, filename);
+#line 33 "[stdout]"
+ trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);
+ unused = write(trace_marker_fd, ftrace_buf, trlen);
+ }
+ }
+}
+
+static inline void trace_test_blah(void *context, const char *filename)
+{
+ if (true) {
+ _nocheck__trace_test_blah(context, filename);
+ }
+}
+
+#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
+ trace_event_get_state_dynamic_by_id(TRACE_TEST_WIBBLE) || \
+ false)
+
+static inline void _nocheck__trace_test_wibble(void *context, int value)
+{
+ {
+ char ftrace_buf[MAX_TRACE_STRLEN];
+ int unused __attribute__ ((unused));
+ int trlen;
+ if (trace_event_get_state(TRACE_TEST_WIBBLE)) {
+#line 5 "trace-events"
+ trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,
+ "test_wibble " "Wibble context=%p value=%d" "\n" , context, value);
+#line 61 "[stdout]"
+ trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);
+ unused = write(trace_marker_fd, ftrace_buf, trlen);
+ }
+ }
+}
+
+static inline void trace_test_wibble(void *context, int value)
+{
+ if (true) {
+ _nocheck__trace_test_wibble(context, value);
+ }
+}
+#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/log.c b/tests/tracetool/log.c
new file mode 100644
index 0000000000..9f862fa14d
--- /dev/null
+++ b/tests/tracetool/log.c
@@ -0,0 +1,32 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "trace-testsuite.h"
+
+uint16_t _TRACE_TEST_BLAH_DSTATE;
+uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+TraceEvent _TRACE_TEST_BLAH_EVENT = {
+ .id = 0,
+ .name = "test_blah",
+ .sstate = TRACE_TEST_BLAH_ENABLED,
+ .dstate = &_TRACE_TEST_BLAH_DSTATE
+};
+TraceEvent _TRACE_TEST_WIBBLE_EVENT = {
+ .id = 0,
+ .name = "test_wibble",
+ .sstate = TRACE_TEST_WIBBLE_ENABLED,
+ .dstate = &_TRACE_TEST_WIBBLE_DSTATE
+};
+TraceEvent *testsuite_trace_events[] = {
+ &_TRACE_TEST_BLAH_EVENT,
+ &_TRACE_TEST_WIBBLE_EVENT,
+ NULL,
+};
+
+static void trace_testsuite_register_events(void)
+{
+ trace_event_register_group(testsuite_trace_events);
+}
+trace_init(trace_testsuite_register_events)
diff --git a/tests/tracetool/log.h b/tests/tracetool/log.h
new file mode 100644
index 0000000000..fce07786b6
--- /dev/null
+++ b/tests/tracetool/log.h
@@ -0,0 +1,57 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef TRACE_TESTSUITE_GENERATED_TRACERS_H
+#define TRACE_TESTSUITE_GENERATED_TRACERS_H
+
+#include "trace/control.h"
+
+extern TraceEvent _TRACE_TEST_BLAH_EVENT;
+extern TraceEvent _TRACE_TEST_WIBBLE_EVENT;
+extern uint16_t _TRACE_TEST_BLAH_DSTATE;
+extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+#define TRACE_TEST_BLAH_ENABLED 1
+#define TRACE_TEST_WIBBLE_ENABLED 1
+#include "qemu/log-for-trace.h"
+
+
+#define TRACE_TEST_BLAH_BACKEND_DSTATE() ( \
+ trace_event_get_state_dynamic_by_id(TRACE_TEST_BLAH) || \
+ false)
+
+static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+{
+ if (trace_event_get_state(TRACE_TEST_BLAH) && qemu_loglevel_mask(LOG_TRACE)) {
+#line 4 "trace-events"
+ qemu_log("test_blah " "Blah context=%p filename=%s" "\n", context, filename);
+#line 28 "[stdout]"
+ }
+}
+
+static inline void trace_test_blah(void *context, const char *filename)
+{
+ if (true) {
+ _nocheck__trace_test_blah(context, filename);
+ }
+}
+
+#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
+ trace_event_get_state_dynamic_by_id(TRACE_TEST_WIBBLE) || \
+ false)
+
+static inline void _nocheck__trace_test_wibble(void *context, int value)
+{
+ if (trace_event_get_state(TRACE_TEST_WIBBLE) && qemu_loglevel_mask(LOG_TRACE)) {
+#line 5 "trace-events"
+ qemu_log("test_wibble " "Wibble context=%p value=%d" "\n", context, value);
+#line 48 "[stdout]"
+ }
+}
+
+static inline void trace_test_wibble(void *context, int value)
+{
+ if (true) {
+ _nocheck__trace_test_wibble(context, value);
+ }
+}
+#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/meson.build b/tests/tracetool/meson.build
new file mode 100644
index 0000000000..d12378964f
--- /dev/null
+++ b/tests/tracetool/meson.build
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+test_env = environment()
+test_env.set('PYTHONPATH', meson.project_source_root() / 'scripts')
+test_env.set('PYTHONIOENCODING', 'utf-8')
+
+backends = [
+ 'dtrace',
+ 'ftrace',
+ 'log',
+ 'simple',
+ 'syslog',
+ 'ust'
+]
+
+foreach backend: backends
+ test(backend,
+ python,
+ args: [meson.current_source_dir() / 'tracetool-test.py',
+ meson.project_source_root() / 'scripts' / 'tracetool.py',
+ 'trace-events',
+ backend],
+ workdir: meson.current_source_dir(),
+ suite: ['tracetool'])
+endforeach
diff --git a/tests/tracetool/simple.c b/tests/tracetool/simple.c
new file mode 100644
index 0000000000..0484177481
--- /dev/null
+++ b/tests/tracetool/simple.c
@@ -0,0 +1,61 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "trace-testsuite.h"
+
+uint16_t _TRACE_TEST_BLAH_DSTATE;
+uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+TraceEvent _TRACE_TEST_BLAH_EVENT = {
+ .id = 0,
+ .name = "test_blah",
+ .sstate = TRACE_TEST_BLAH_ENABLED,
+ .dstate = &_TRACE_TEST_BLAH_DSTATE
+};
+TraceEvent _TRACE_TEST_WIBBLE_EVENT = {
+ .id = 0,
+ .name = "test_wibble",
+ .sstate = TRACE_TEST_WIBBLE_ENABLED,
+ .dstate = &_TRACE_TEST_WIBBLE_DSTATE
+};
+TraceEvent *testsuite_trace_events[] = {
+ &_TRACE_TEST_BLAH_EVENT,
+ &_TRACE_TEST_WIBBLE_EVENT,
+ NULL,
+};
+
+static void trace_testsuite_register_events(void)
+{
+ trace_event_register_group(testsuite_trace_events);
+}
+trace_init(trace_testsuite_register_events)
+#include "qemu/osdep.h"
+#include "trace/control.h"
+#include "trace/simple.h"
+
+void _simple_trace_test_blah(void *context, const char *filename)
+{
+ TraceBufferRecord rec;
+ size_t argfilename_len = filename ? MIN(strlen(filename), MAX_TRACE_STRLEN) : 0;
+
+ if (trace_record_start(&rec, _TRACE_TEST_BLAH_EVENT.id, 8 + 4 + argfilename_len)) {
+ return; /* Trace Buffer Full, Event Dropped ! */
+ }
+ trace_record_write_u64(&rec, (uintptr_t)(uint64_t *)context);
+ trace_record_write_str(&rec, filename, argfilename_len);
+ trace_record_finish(&rec);
+}
+
+void _simple_trace_test_wibble(void *context, int value)
+{
+ TraceBufferRecord rec;
+
+ if (trace_record_start(&rec, _TRACE_TEST_WIBBLE_EVENT.id, 8 + 8)) {
+ return; /* Trace Buffer Full, Event Dropped ! */
+ }
+ trace_record_write_u64(&rec, (uintptr_t)(uint64_t *)context);
+ trace_record_write_u64(&rec, (uint64_t)value);
+ trace_record_finish(&rec);
+}
+
diff --git a/tests/tracetool/simple.h b/tests/tracetool/simple.h
new file mode 100644
index 0000000000..3c9de68c43
--- /dev/null
+++ b/tests/tracetool/simple.h
@@ -0,0 +1,54 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef TRACE_TESTSUITE_GENERATED_TRACERS_H
+#define TRACE_TESTSUITE_GENERATED_TRACERS_H
+
+#include "trace/control.h"
+
+extern TraceEvent _TRACE_TEST_BLAH_EVENT;
+extern TraceEvent _TRACE_TEST_WIBBLE_EVENT;
+extern uint16_t _TRACE_TEST_BLAH_DSTATE;
+extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+#define TRACE_TEST_BLAH_ENABLED 1
+#define TRACE_TEST_WIBBLE_ENABLED 1
+void _simple_trace_test_blah(void *context, const char *filename);
+void _simple_trace_test_wibble(void *context, int value);
+
+
+#define TRACE_TEST_BLAH_BACKEND_DSTATE() ( \
+ trace_event_get_state_dynamic_by_id(TRACE_TEST_BLAH) || \
+ false)
+
+static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+{
+ if (trace_event_get_state(TRACE_TEST_BLAH)) {
+ _simple_trace_test_blah(context, filename);
+ }
+}
+
+static inline void trace_test_blah(void *context, const char *filename)
+{
+ if (true) {
+ _nocheck__trace_test_blah(context, filename);
+ }
+}
+
+#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
+ trace_event_get_state_dynamic_by_id(TRACE_TEST_WIBBLE) || \
+ false)
+
+static inline void _nocheck__trace_test_wibble(void *context, int value)
+{
+ if (trace_event_get_state(TRACE_TEST_WIBBLE)) {
+ _simple_trace_test_wibble(context, value);
+ }
+}
+
+static inline void trace_test_wibble(void *context, int value)
+{
+ if (true) {
+ _nocheck__trace_test_wibble(context, value);
+ }
+}
+#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/syslog.c b/tests/tracetool/syslog.c
new file mode 100644
index 0000000000..9f862fa14d
--- /dev/null
+++ b/tests/tracetool/syslog.c
@@ -0,0 +1,32 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "trace-testsuite.h"
+
+uint16_t _TRACE_TEST_BLAH_DSTATE;
+uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+TraceEvent _TRACE_TEST_BLAH_EVENT = {
+ .id = 0,
+ .name = "test_blah",
+ .sstate = TRACE_TEST_BLAH_ENABLED,
+ .dstate = &_TRACE_TEST_BLAH_DSTATE
+};
+TraceEvent _TRACE_TEST_WIBBLE_EVENT = {
+ .id = 0,
+ .name = "test_wibble",
+ .sstate = TRACE_TEST_WIBBLE_ENABLED,
+ .dstate = &_TRACE_TEST_WIBBLE_DSTATE
+};
+TraceEvent *testsuite_trace_events[] = {
+ &_TRACE_TEST_BLAH_EVENT,
+ &_TRACE_TEST_WIBBLE_EVENT,
+ NULL,
+};
+
+static void trace_testsuite_register_events(void)
+{
+ trace_event_register_group(testsuite_trace_events);
+}
+trace_init(trace_testsuite_register_events)
diff --git a/tests/tracetool/syslog.h b/tests/tracetool/syslog.h
new file mode 100644
index 0000000000..5c757a43fc
--- /dev/null
+++ b/tests/tracetool/syslog.h
@@ -0,0 +1,57 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef TRACE_TESTSUITE_GENERATED_TRACERS_H
+#define TRACE_TESTSUITE_GENERATED_TRACERS_H
+
+#include "trace/control.h"
+
+extern TraceEvent _TRACE_TEST_BLAH_EVENT;
+extern TraceEvent _TRACE_TEST_WIBBLE_EVENT;
+extern uint16_t _TRACE_TEST_BLAH_DSTATE;
+extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+#define TRACE_TEST_BLAH_ENABLED 1
+#define TRACE_TEST_WIBBLE_ENABLED 1
+#include <syslog.h>
+
+
+#define TRACE_TEST_BLAH_BACKEND_DSTATE() ( \
+ trace_event_get_state_dynamic_by_id(TRACE_TEST_BLAH) || \
+ false)
+
+static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+{
+ if (trace_event_get_state(TRACE_TEST_BLAH)) {
+#line 4 "trace-events"
+ syslog(LOG_INFO, "test_blah " "Blah context=%p filename=%s" , context, filename);
+#line 28 "[stdout]"
+ }
+}
+
+static inline void trace_test_blah(void *context, const char *filename)
+{
+ if (true) {
+ _nocheck__trace_test_blah(context, filename);
+ }
+}
+
+#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
+ trace_event_get_state_dynamic_by_id(TRACE_TEST_WIBBLE) || \
+ false)
+
+static inline void _nocheck__trace_test_wibble(void *context, int value)
+{
+ if (trace_event_get_state(TRACE_TEST_WIBBLE)) {
+#line 5 "trace-events"
+ syslog(LOG_INFO, "test_wibble " "Wibble context=%p value=%d" , context, value);
+#line 48 "[stdout]"
+ }
+}
+
+static inline void trace_test_wibble(void *context, int value)
+{
+ if (true) {
+ _nocheck__trace_test_wibble(context, value);
+ }
+}
+#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/trace-events b/tests/tracetool/trace-events
new file mode 100644
index 0000000000..72cf4d6f70
--- /dev/null
+++ b/tests/tracetool/trace-events
@@ -0,0 +1,5 @@
+# See docs/devel/tracing.rst for syntax documentation.
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+test_blah(void *context, const char *filename) "Blah context=%p filename=%s"
+test_wibble(void *context, int value) "Wibble context=%p value=%d"
diff --git a/tests/tracetool/tracetool-test.py b/tests/tracetool/tracetool-test.py
new file mode 100755
index 0000000000..85d36f87db
--- /dev/null
+++ b/tests/tracetool/tracetool-test.py
@@ -0,0 +1,91 @@
+#!/usr/bin/python3
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+from subprocess import check_output
+import sys
+
+
+def get_backends():
+ return [
+ ]
+
+def get_formats(backend):
+ formats = [
+ "c",
+ "h",
+ ]
+ if backend == "dtrace":
+ formats += [
+ "d",
+ "log-stap",
+ "simpletrace-stap",
+ "stap",
+ ]
+ if backend == "ust":
+ formats += [
+ "ust-events-c",
+ "ust-events-h",
+ ]
+ return formats
+
+def test_tracetool_one(tracetool, events, backend, fmt):
+ filename = backend + "." + fmt
+
+ args = [tracetool,
+ f"--format={fmt}",
+ f"--backends={backend}",
+ "--group=testsuite"]
+
+ if fmt.find("stap") != -1:
+ args += ["--binary=qemu",
+ "--probe-prefix=qemu"]
+
+ args += [events, "-"]
+
+ actual = check_output(args)
+
+ if os.getenv("QEMU_TEST_REGENERATE", False):
+ print(f"# regenerate {filename}")
+ with open(filename, "wb") as fh:
+ fh.write(actual)
+
+ with open(filename, "rb") as fh:
+ expect = fh.read()
+
+ assert(expect == actual)
+
+def test_tracetool(tracetool, events, backend):
+ fail = False
+ scenarios = len(get_formats(backend))
+
+ print(f"1..{scenarios}")
+
+ num = 1
+ for fmt in get_formats(backend):
+ status = "not ok"
+ hint = ""
+ try:
+ test_tracetool_one(tracetool, events, backend, fmt)
+ status = "ok"
+ except Exception as e:
+ print(f"# {e}")
+ fail = True
+ hint = " (set QEMU_TEST_REGENERATE=1 to recreate reference " + \
+ "output if tracetool generator was intentionally changed)"
+ finally:
+ print(f"{status} {num} - {backend}.{fmt}{hint}")
+
+ return fail
+
+
+if __name__ == '__main__':
+ if len(sys.argv) != 4:
+ argv0 = sys.argv[0]
+ print("syntax: {argv0} TRACE-TOOL TRACE-EVENTS BACKEND", file=sys.stderr)
+ sys.exit(1)
+
+ fail = test_tracetool(sys.argv[1], sys.argv[2], sys.argv[3])
+ if fail:
+ sys.exit(1)
+ sys.exit(0)
diff --git a/tests/tracetool/ust.c b/tests/tracetool/ust.c
new file mode 100644
index 0000000000..9f862fa14d
--- /dev/null
+++ b/tests/tracetool/ust.c
@@ -0,0 +1,32 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "trace-testsuite.h"
+
+uint16_t _TRACE_TEST_BLAH_DSTATE;
+uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+TraceEvent _TRACE_TEST_BLAH_EVENT = {
+ .id = 0,
+ .name = "test_blah",
+ .sstate = TRACE_TEST_BLAH_ENABLED,
+ .dstate = &_TRACE_TEST_BLAH_DSTATE
+};
+TraceEvent _TRACE_TEST_WIBBLE_EVENT = {
+ .id = 0,
+ .name = "test_wibble",
+ .sstate = TRACE_TEST_WIBBLE_ENABLED,
+ .dstate = &_TRACE_TEST_WIBBLE_DSTATE
+};
+TraceEvent *testsuite_trace_events[] = {
+ &_TRACE_TEST_BLAH_EVENT,
+ &_TRACE_TEST_WIBBLE_EVENT,
+ NULL,
+};
+
+static void trace_testsuite_register_events(void)
+{
+ trace_event_register_group(testsuite_trace_events);
+}
+trace_init(trace_testsuite_register_events)
diff --git a/tests/tracetool/ust.h b/tests/tracetool/ust.h
new file mode 100644
index 0000000000..1184ddd870
--- /dev/null
+++ b/tests/tracetool/ust.h
@@ -0,0 +1,55 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef TRACE_TESTSUITE_GENERATED_TRACERS_H
+#define TRACE_TESTSUITE_GENERATED_TRACERS_H
+
+#include "trace/control.h"
+
+extern TraceEvent _TRACE_TEST_BLAH_EVENT;
+extern TraceEvent _TRACE_TEST_WIBBLE_EVENT;
+extern uint16_t _TRACE_TEST_BLAH_DSTATE;
+extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
+#define TRACE_TEST_BLAH_ENABLED 1
+#define TRACE_TEST_WIBBLE_ENABLED 1
+#include <lttng/tracepoint.h>
+#include "trace-ust-testsuite.h"
+
+/* tracepoint_enabled() was introduced in LTTng UST 2.7 */
+#ifndef tracepoint_enabled
+#define tracepoint_enabled(a, b) true
+#endif
+
+
+#define TRACE_TEST_BLAH_BACKEND_DSTATE() ( \
+ tracepoint_enabled(qemu, test_blah) || \
+ false)
+
+static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+{
+ tracepoint(qemu, test_blah, context, filename);
+}
+
+static inline void trace_test_blah(void *context, const char *filename)
+{
+ if (true) {
+ _nocheck__trace_test_blah(context, filename);
+ }
+}
+
+#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
+ tracepoint_enabled(qemu, test_wibble) || \
+ false)
+
+static inline void _nocheck__trace_test_wibble(void *context, int value)
+{
+ tracepoint(qemu, test_wibble, context, value);
+}
+
+static inline void trace_test_wibble(void *context, int value)
+{
+ if (true) {
+ _nocheck__trace_test_wibble(context, value);
+ }
+}
+#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/ust.ust-events-c b/tests/tracetool/ust.ust-events-c
new file mode 100644
index 0000000000..db23224056
--- /dev/null
+++ b/tests/tracetool/ust.ust-events-c
@@ -0,0 +1,14 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+
+#define TRACEPOINT_DEFINE
+#define TRACEPOINT_CREATE_PROBES
+
+/* If gcc version 4.7 or older is used, LTTng ust gives a warning when compiling with
+ -Wredundant-decls.
+ */
+#pragma GCC diagnostic ignored "-Wredundant-decls"
+
+#include "trace-ust-all.h"
diff --git a/tests/tracetool/ust.ust-events-h b/tests/tracetool/ust.ust-events-h
new file mode 100644
index 0000000000..4621a995fc
--- /dev/null
+++ b/tests/tracetool/ust.ust-events-h
@@ -0,0 +1,56 @@
+/* This file is autogenerated by tracetool, do not edit. */
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#undef TRACEPOINT_PROVIDER
+#define TRACEPOINT_PROVIDER qemu
+
+#undef TRACEPOINT_INCLUDE
+#define TRACEPOINT_INCLUDE "./trace-ust.h"
+
+#if !defined (TRACE_TESTSUITE_GENERATED_UST_H) || \
+ defined(TRACEPOINT_HEADER_MULTI_READ)
+#define TRACE_TESTSUITE_GENERATED_UST_H
+
+#include <lttng/tracepoint.h>
+
+/*
+ * LTTng ust 2.0 does not allow you to use TP_ARGS(void) for tracepoints
+ * requiring no arguments. We define these macros introduced in more recent * versions of LTTng ust as a workaround
+ */
+#ifndef _TP_EXPROTO1
+#define _TP_EXPROTO1(a) void
+#endif
+#ifndef _TP_EXDATA_PROTO1
+#define _TP_EXDATA_PROTO1(a) void *__tp_data
+#endif
+#ifndef _TP_EXDATA_VAR1
+#define _TP_EXDATA_VAR1(a) __tp_data
+#endif
+#ifndef _TP_EXVAR1
+#define _TP_EXVAR1(a)
+#endif
+
+TRACEPOINT_EVENT(
+ qemu,
+ test_blah,
+ TP_ARGS(void *, context, const char *, filename),
+ TP_FIELDS(
+ ctf_integer_hex(void *, context, context)
+ ctf_string(filename, filename)
+ )
+)
+
+TRACEPOINT_EVENT(
+ qemu,
+ test_wibble,
+ TP_ARGS(void *, context, int, value),
+ TP_FIELDS(
+ ctf_integer_hex(void *, context, context)
+ ctf_integer(int, value, value)
+ )
+)
+
+#endif /* TRACE_TESTSUITE_GENERATED_UST_H */
+
+/* This part must be outside ifdef protection */
+#include <lttng/tracepoint-event.h>
--
2.50.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 7/8] tracetool: drop the probe "__nocheck__" wrapping
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
` (5 preceding siblings ...)
2025-08-06 16:48 ` [PATCH v3 6/8] tracetool: add test suite for tracetool with reference output Daniel P. Berrangé
@ 2025-08-06 16:48 ` Daniel P. Berrangé
2025-08-06 16:48 ` [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var Daniel P. Berrangé
2025-08-07 20:06 ` [PATCH v3 0/8] tracetool: add test suite to improve reviewability Stefan Hajnoczi
8 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-06 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, Daniel P. Berrangé
Every generated inline probe function is wrapped with a
trivial caller that has a hard-coded condition test:
static inline void _nocheck__trace_test_wibble(void * context, int value)
{
tracepoint(qemu, test_wibble, context, value);
}
static inline void trace_test_wibble(void * context, int value)
{
if (true) {
_nocheck__trace_test_wibble(context, value);
}
}
This was introduced for TCG probes back in
864a2178: trace: [tcg] Do not generate TCG code to trace dynamically-disabled events
but is obsolete since
126d4123 tracing: excise the tcg related from tracetool
This commit removes the wrapping such that we have
static inline void trace_test_wibble(void * context, int value)
{
tracepoint(qemu, test_wibble, context, value);
}
The default build of qemu-system-x86_64 on Fedora with the
'log' backend, has its size reduced by 1 MB
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
scripts/tracetool/__init__.py | 1 -
scripts/tracetool/format/h.py | 16 +---------------
tests/tracetool/dtrace.h | 18 ++----------------
tests/tracetool/ftrace.h | 20 +++-----------------
tests/tracetool/log.h | 20 +++-----------------
tests/tracetool/simple.h | 18 ++----------------
tests/tracetool/syslog.h | 20 +++-----------------
tests/tracetool/ust.h | 18 ++----------------
8 files changed, 16 insertions(+), 115 deletions(-)
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index c8fd3a7ddc..099b398076 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -342,7 +342,6 @@ def formats(self):
return self._FMT.findall(self.fmt)
QEMU_TRACE = "trace_%(name)s"
- QEMU_TRACE_NOCHECK = "_nocheck__" + QEMU_TRACE
QEMU_TRACE_TCG = QEMU_TRACE + "_tcg"
QEMU_DSTATE = "_TRACE_%(NAME)s_DSTATE"
QEMU_BACKEND_DSTATE = "TRACE_%(NAME)s_BACKEND_DSTATE"
diff --git a/scripts/tracetool/format/h.py b/scripts/tracetool/format/h.py
index a00ae475f7..b42a8268a8 100644
--- a/scripts/tracetool/format/h.py
+++ b/scripts/tracetool/format/h.py
@@ -64,7 +64,7 @@ def generate(events, backend, group):
out('',
'static inline void %(api)s(%(args)s)',
'{',
- api=e.api(e.QEMU_TRACE_NOCHECK),
+ api=e.api(),
args=e.args)
if "disable" not in e.properties:
@@ -72,20 +72,6 @@ def generate(events, backend, group):
out('}')
- cond = "true"
-
- out('',
- 'static inline void %(api)s(%(args)s)',
- '{',
- ' if (%(cond)s) {',
- ' %(api_nocheck)s(%(names)s);',
- ' }',
- '}',
- api=e.api(),
- api_nocheck=e.api(e.QEMU_TRACE_NOCHECK),
- args=e.args,
- names=", ".join(e.args.names()),
- cond=cond)
backend.generate_end(events, group)
diff --git a/tests/tracetool/dtrace.h b/tests/tracetool/dtrace.h
index c2e5110672..c8931a8d7b 100644
--- a/tests/tracetool/dtrace.h
+++ b/tests/tracetool/dtrace.h
@@ -29,31 +29,17 @@ extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
QEMU_TEST_BLAH_ENABLED() || \
false)
-static inline void _nocheck__trace_test_blah(void *context, const char *filename)
-{
- QEMU_TEST_BLAH(context, filename);
-}
-
static inline void trace_test_blah(void *context, const char *filename)
{
- if (true) {
- _nocheck__trace_test_blah(context, filename);
- }
+ QEMU_TEST_BLAH(context, filename);
}
#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
QEMU_TEST_WIBBLE_ENABLED() || \
false)
-static inline void _nocheck__trace_test_wibble(void *context, int value)
-{
- QEMU_TEST_WIBBLE(context, value);
-}
-
static inline void trace_test_wibble(void *context, int value)
{
- if (true) {
- _nocheck__trace_test_wibble(context, value);
- }
+ QEMU_TEST_WIBBLE(context, value);
}
#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/ftrace.h b/tests/tracetool/ftrace.h
index b4daa96fe7..aa8d720a92 100644
--- a/tests/tracetool/ftrace.h
+++ b/tests/tracetool/ftrace.h
@@ -19,7 +19,7 @@ extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
trace_event_get_state_dynamic_by_id(TRACE_TEST_BLAH) || \
false)
-static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+static inline void trace_test_blah(void *context, const char *filename)
{
{
char ftrace_buf[MAX_TRACE_STRLEN];
@@ -36,18 +36,11 @@ static inline void _nocheck__trace_test_blah(void *context, const char *filename
}
}
-static inline void trace_test_blah(void *context, const char *filename)
-{
- if (true) {
- _nocheck__trace_test_blah(context, filename);
- }
-}
-
#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
trace_event_get_state_dynamic_by_id(TRACE_TEST_WIBBLE) || \
false)
-static inline void _nocheck__trace_test_wibble(void *context, int value)
+static inline void trace_test_wibble(void *context, int value)
{
{
char ftrace_buf[MAX_TRACE_STRLEN];
@@ -57,17 +50,10 @@ static inline void _nocheck__trace_test_wibble(void *context, int value)
#line 5 "trace-events"
trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,
"test_wibble " "Wibble context=%p value=%d" "\n" , context, value);
-#line 61 "[stdout]"
+#line 54 "[stdout]"
trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);
unused = write(trace_marker_fd, ftrace_buf, trlen);
}
}
}
-
-static inline void trace_test_wibble(void *context, int value)
-{
- if (true) {
- _nocheck__trace_test_wibble(context, value);
- }
-}
#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/log.h b/tests/tracetool/log.h
index fce07786b6..8a58c0ff67 100644
--- a/tests/tracetool/log.h
+++ b/tests/tracetool/log.h
@@ -19,7 +19,7 @@ extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
trace_event_get_state_dynamic_by_id(TRACE_TEST_BLAH) || \
false)
-static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+static inline void trace_test_blah(void *context, const char *filename)
{
if (trace_event_get_state(TRACE_TEST_BLAH) && qemu_loglevel_mask(LOG_TRACE)) {
#line 4 "trace-events"
@@ -28,30 +28,16 @@ static inline void _nocheck__trace_test_blah(void *context, const char *filename
}
}
-static inline void trace_test_blah(void *context, const char *filename)
-{
- if (true) {
- _nocheck__trace_test_blah(context, filename);
- }
-}
-
#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
trace_event_get_state_dynamic_by_id(TRACE_TEST_WIBBLE) || \
false)
-static inline void _nocheck__trace_test_wibble(void *context, int value)
+static inline void trace_test_wibble(void *context, int value)
{
if (trace_event_get_state(TRACE_TEST_WIBBLE) && qemu_loglevel_mask(LOG_TRACE)) {
#line 5 "trace-events"
qemu_log("test_wibble " "Wibble context=%p value=%d" "\n", context, value);
-#line 48 "[stdout]"
- }
-}
-
-static inline void trace_test_wibble(void *context, int value)
-{
- if (true) {
- _nocheck__trace_test_wibble(context, value);
+#line 41 "[stdout]"
}
}
#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/simple.h b/tests/tracetool/simple.h
index 3c9de68c43..ec6fcb22c3 100644
--- a/tests/tracetool/simple.h
+++ b/tests/tracetool/simple.h
@@ -20,35 +20,21 @@ void _simple_trace_test_wibble(void *context, int value);
trace_event_get_state_dynamic_by_id(TRACE_TEST_BLAH) || \
false)
-static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+static inline void trace_test_blah(void *context, const char *filename)
{
if (trace_event_get_state(TRACE_TEST_BLAH)) {
_simple_trace_test_blah(context, filename);
}
}
-static inline void trace_test_blah(void *context, const char *filename)
-{
- if (true) {
- _nocheck__trace_test_blah(context, filename);
- }
-}
-
#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
trace_event_get_state_dynamic_by_id(TRACE_TEST_WIBBLE) || \
false)
-static inline void _nocheck__trace_test_wibble(void *context, int value)
+static inline void trace_test_wibble(void *context, int value)
{
if (trace_event_get_state(TRACE_TEST_WIBBLE)) {
_simple_trace_test_wibble(context, value);
}
}
-
-static inline void trace_test_wibble(void *context, int value)
-{
- if (true) {
- _nocheck__trace_test_wibble(context, value);
- }
-}
#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/syslog.h b/tests/tracetool/syslog.h
index 5c757a43fc..c449c9c86b 100644
--- a/tests/tracetool/syslog.h
+++ b/tests/tracetool/syslog.h
@@ -19,7 +19,7 @@ extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
trace_event_get_state_dynamic_by_id(TRACE_TEST_BLAH) || \
false)
-static inline void _nocheck__trace_test_blah(void *context, const char *filename)
+static inline void trace_test_blah(void *context, const char *filename)
{
if (trace_event_get_state(TRACE_TEST_BLAH)) {
#line 4 "trace-events"
@@ -28,30 +28,16 @@ static inline void _nocheck__trace_test_blah(void *context, const char *filename
}
}
-static inline void trace_test_blah(void *context, const char *filename)
-{
- if (true) {
- _nocheck__trace_test_blah(context, filename);
- }
-}
-
#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
trace_event_get_state_dynamic_by_id(TRACE_TEST_WIBBLE) || \
false)
-static inline void _nocheck__trace_test_wibble(void *context, int value)
+static inline void trace_test_wibble(void *context, int value)
{
if (trace_event_get_state(TRACE_TEST_WIBBLE)) {
#line 5 "trace-events"
syslog(LOG_INFO, "test_wibble " "Wibble context=%p value=%d" , context, value);
-#line 48 "[stdout]"
- }
-}
-
-static inline void trace_test_wibble(void *context, int value)
-{
- if (true) {
- _nocheck__trace_test_wibble(context, value);
+#line 41 "[stdout]"
}
}
#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
diff --git a/tests/tracetool/ust.h b/tests/tracetool/ust.h
index 1184ddd870..b7acd0c39b 100644
--- a/tests/tracetool/ust.h
+++ b/tests/tracetool/ust.h
@@ -25,31 +25,17 @@ extern uint16_t _TRACE_TEST_WIBBLE_DSTATE;
tracepoint_enabled(qemu, test_blah) || \
false)
-static inline void _nocheck__trace_test_blah(void *context, const char *filename)
-{
- tracepoint(qemu, test_blah, context, filename);
-}
-
static inline void trace_test_blah(void *context, const char *filename)
{
- if (true) {
- _nocheck__trace_test_blah(context, filename);
- }
+ tracepoint(qemu, test_blah, context, filename);
}
#define TRACE_TEST_WIBBLE_BACKEND_DSTATE() ( \
tracepoint_enabled(qemu, test_wibble) || \
false)
-static inline void _nocheck__trace_test_wibble(void *context, int value)
-{
- tracepoint(qemu, test_wibble, context, value);
-}
-
static inline void trace_test_wibble(void *context, int value)
{
- if (true) {
- _nocheck__trace_test_wibble(context, value);
- }
+ tracepoint(qemu, test_wibble, context, value);
}
#endif /* TRACE_TESTSUITE_GENERATED_TRACERS_H */
--
2.50.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
` (6 preceding siblings ...)
2025-08-06 16:48 ` [PATCH v3 7/8] tracetool: drop the probe "__nocheck__" wrapping Daniel P. Berrangé
@ 2025-08-06 16:48 ` Daniel P. Berrangé
2025-08-08 6:46 ` Markus Armbruster
2025-08-07 20:06 ` [PATCH v3 0/8] tracetool: add test suite to improve reviewability Stefan Hajnoczi
8 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-06 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth, Daniel P. Berrangé
The QAPI_TEST_UPDATE env var can be set when running the QAPI
schema tests to regenerate the reference output. For consistent
naming with the tracetool test, change the env var name to
QEMU_TEST_REGENERATE.
The test is modified to provide a hint about use of the new
env var and it is also added to the developer documentation.document its usage.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
docs/devel/testing/main.rst | 12 ++++++++++++
tests/qapi-schema/test-qapi.py | 7 +++++--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/docs/devel/testing/main.rst b/docs/devel/testing/main.rst
index 11f05c0006..fe233fcf7a 100644
--- a/docs/devel/testing/main.rst
+++ b/docs/devel/testing/main.rst
@@ -178,6 +178,18 @@ parser (either fixing a bug or extending/modifying the syntax). To do this:
``qapi-schema += foo.json``
+The reference output can be automatically updated to match the latest QAPI
+code generator by running the tests with the QEMU_TEST_REGENERATE environment
+variable set.
+
+.. code::
+
+ QEMU_TEST_REGENERATE=1 make check-qapi-schema
+
+The resulting changes must be reviewed by the author to ensure they match
+the intended results, before adding the updated reference output to the
+same commit that alters the generator code.
+
.. _tracetool-tests:
Tracetool tests
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 4be930228c..cf7fb8a6df 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
if actual_out == expected_out and actual_err == expected_err:
return 0
- print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
+ print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
file=sys.stderr)
out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
@@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
sys.stdout.writelines(err_diff)
if not update:
+ print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
+ "if the QAPI schema generator was intentionally changed") % test_name,
+ file=sys.stderr)
return 1
try:
@@ -197,7 +200,7 @@ def main(argv):
parser.add_argument('-d', '--dir', action='store', default='',
help="directory containing tests")
parser.add_argument('-u', '--update', action='store_true',
- default='QAPI_TEST_UPDATE' in os.environ,
+ default='QEMU_TEST_REGENERATE' in os.environ,
help="update expected test results")
parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
args = parser.parse_args()
--
2.50.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var
2025-08-06 16:48 ` [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var Daniel P. Berrangé
@ 2025-08-08 6:46 ` Markus Armbruster
2025-08-18 15:09 ` Daniel P. Berrangé
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2025-08-08 6:46 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Stefan Hajnoczi, Michael Roth, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth
Daniel P. Berrangé <berrange@redhat.com> writes:
> The QAPI_TEST_UPDATE env var can be set when running the QAPI
> schema tests to regenerate the reference output. For consistent
> naming with the tracetool test, change the env var name to
> QEMU_TEST_REGENERATE.
>
> The test is modified to provide a hint about use of the new
> env var and it is also added to the developer documentation.document its usage.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> docs/devel/testing/main.rst | 12 ++++++++++++
> tests/qapi-schema/test-qapi.py | 7 +++++--
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/docs/devel/testing/main.rst b/docs/devel/testing/main.rst
> index 11f05c0006..fe233fcf7a 100644
> --- a/docs/devel/testing/main.rst
> +++ b/docs/devel/testing/main.rst
> @@ -178,6 +178,18 @@ parser (either fixing a bug or extending/modifying the syntax). To do this:
>
> ``qapi-schema += foo.json``
>
> +The reference output can be automatically updated to match the latest QAPI
> +code generator by running the tests with the QEMU_TEST_REGENERATE environment
> +variable set.
> +
> +.. code::
> +
> + QEMU_TEST_REGENERATE=1 make check-qapi-schema
> +
> +The resulting changes must be reviewed by the author to ensure they match
> +the intended results, before adding the updated reference output to the
Not sure about the comma. I'm not a native speaker, though.
> +same commit that alters the generator code.
> +
> .. _tracetool-tests:
>
> Tracetool tests
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 4be930228c..cf7fb8a6df 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
> if actual_out == expected_out and actual_err == expected_err:
> return 0
>
> - print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> + print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
Is there a need for this, or is it just drive-by polishing?
> file=sys.stderr)
> out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
> err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
> @@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
> sys.stdout.writelines(err_diff)
>
> if not update:
> + print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
> + "if the QAPI schema generator was intentionally changed") % test_name,
> + file=sys.stderr)
> return 1
>
> try:
> @@ -197,7 +200,7 @@ def main(argv):
> parser.add_argument('-d', '--dir', action='store', default='',
> help="directory containing tests")
> parser.add_argument('-u', '--update', action='store_true',
> - default='QAPI_TEST_UPDATE' in os.environ,
> + default='QEMU_TEST_REGENERATE' in os.environ,
> help="update expected test results")
> parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
> args = parser.parse_args()
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var
2025-08-08 6:46 ` Markus Armbruster
@ 2025-08-18 15:09 ` Daniel P. Berrangé
2025-08-25 12:01 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-18 15:09 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Stefan Hajnoczi, Michael Roth, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth
On Fri, Aug 08, 2025 at 08:46:10AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > The QAPI_TEST_UPDATE env var can be set when running the QAPI
> > schema tests to regenerate the reference output. For consistent
> > naming with the tracetool test, change the env var name to
> > QEMU_TEST_REGENERATE.
> >
> > The test is modified to provide a hint about use of the new
> > env var and it is also added to the developer documentation.document its usage.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > docs/devel/testing/main.rst | 12 ++++++++++++
> > tests/qapi-schema/test-qapi.py | 7 +++++--
> > 2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/devel/testing/main.rst b/docs/devel/testing/main.rst
> > index 11f05c0006..fe233fcf7a 100644
> > --- a/docs/devel/testing/main.rst
> > +++ b/docs/devel/testing/main.rst
> > @@ -178,6 +178,18 @@ parser (either fixing a bug or extending/modifying the syntax). To do this:
> >
> > ``qapi-schema += foo.json``
> >
> > +The reference output can be automatically updated to match the latest QAPI
> > +code generator by running the tests with the QEMU_TEST_REGENERATE environment
> > +variable set.
> > +
> > +.. code::
> > +
> > + QEMU_TEST_REGENERATE=1 make check-qapi-schema
> > +
> > +The resulting changes must be reviewed by the author to ensure they match
> > +the intended results, before adding the updated reference output to the
>
> Not sure about the comma. I'm not a native speaker, though.
Yeah, the comma is redundant
> > +same commit that alters the generator code.
> > +
> > .. _tracetool-tests:
> >
> > Tracetool tests
> > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> > index 4be930228c..cf7fb8a6df 100755
> > --- a/tests/qapi-schema/test-qapi.py
> > +++ b/tests/qapi-schema/test-qapi.py
> > @@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
> > if actual_out == expected_out and actual_err == expected_err:
> > return 0
> >
> > - print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> > + print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
>
> Is there a need for this, or is it just drive-by polishing?
Just making it more consistent in style with other error print()
statements we have in the file, as well as this new one I'm
adding.
>
> > file=sys.stderr)
> > out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
> > err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
> > @@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
> > sys.stdout.writelines(err_diff)
> >
> > if not update:
> > + print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
> > + "if the QAPI schema generator was intentionally changed") % test_name,
> > + file=sys.stderr)
> > return 1
> >
> > try:
> > @@ -197,7 +200,7 @@ def main(argv):
> > parser.add_argument('-d', '--dir', action='store', default='',
> > help="directory containing tests")
> > parser.add_argument('-u', '--update', action='store_true',
> > - default='QAPI_TEST_UPDATE' in os.environ,
> > + default='QEMU_TEST_REGENERATE' in os.environ,
> > help="update expected test results")
> > parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
> > args = parser.parse_args()
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var
2025-08-18 15:09 ` Daniel P. Berrangé
@ 2025-08-25 12:01 ` Markus Armbruster
2025-08-27 14:11 ` Daniel P. Berrangé
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2025-08-25 12:01 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Stefan Hajnoczi, Michael Roth, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Fri, Aug 08, 2025 at 08:46:10AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > The QAPI_TEST_UPDATE env var can be set when running the QAPI
>> > schema tests to regenerate the reference output. For consistent
>> > naming with the tracetool test, change the env var name to
>> > QEMU_TEST_REGENERATE.
>> >
>> > The test is modified to provide a hint about use of the new
>> > env var and it is also added to the developer documentation.document its usage.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
[...]
>> > index 4be930228c..cf7fb8a6df 100755
>> > --- a/tests/qapi-schema/test-qapi.py
>> > +++ b/tests/qapi-schema/test-qapi.py
>> > @@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
>> > if actual_out == expected_out and actual_err == expected_err:
>> > return 0
>> >
>> > - print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
>> > + print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
>>
>> Is there a need for this, or is it just drive-by polishing?
>
> Just making it more consistent in style with other error print()
> statements we have in the file, as well as this new one I'm
> adding.
Which existing print()s do you mean?
>
>>
>> > file=sys.stderr)
>> > out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
>> > err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
>> > @@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
>> > sys.stdout.writelines(err_diff)
>> >
>> > if not update:
>> > + print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
>> > + "if the QAPI schema generator was intentionally changed") % test_name,
>> > + file=sys.stderr)
>> > return 1
>> >
>> > try:
>> > @@ -197,7 +200,7 @@ def main(argv):
>> > parser.add_argument('-d', '--dir', action='store', default='',
>> > help="directory containing tests")
>> > parser.add_argument('-u', '--update', action='store_true',
>> > - default='QAPI_TEST_UPDATE' in os.environ,
>> > + default='QEMU_TEST_REGENERATE' in os.environ,
>> > help="update expected test results")
>> > parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
>> > args = parser.parse_args()
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var
2025-08-25 12:01 ` Markus Armbruster
@ 2025-08-27 14:11 ` Daniel P. Berrangé
0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2025-08-27 14:11 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Stefan Hajnoczi, Michael Roth, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth
On Mon, Aug 25, 2025 at 02:01:35PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Fri, Aug 08, 2025 at 08:46:10AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >>
> >> > The QAPI_TEST_UPDATE env var can be set when running the QAPI
> >> > schema tests to regenerate the reference output. For consistent
> >> > naming with the tracetool test, change the env var name to
> >> > QEMU_TEST_REGENERATE.
> >> >
> >> > The test is modified to provide a hint about use of the new
> >> > env var and it is also added to the developer documentation.document its usage.
> >> >
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
> [...]
>
> >> > index 4be930228c..cf7fb8a6df 100755
> >> > --- a/tests/qapi-schema/test-qapi.py
> >> > +++ b/tests/qapi-schema/test-qapi.py
> >> > @@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
> >> > if actual_out == expected_out and actual_err == expected_err:
> >> > return 0
> >> >
> >> > - print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> >> > + print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> >>
> >> Is there a need for this, or is it just drive-by polishing?
> >
> > Just making it more consistent in style with other error print()
> > statements we have in the file, as well as this new one I'm
> > adding.
>
> Which existing print()s do you mean?
I was referring to the ones in this method
except OSError as err:
print("%s: can't open '%s': %s"
% (sys.argv[0], err.filename, err.strerror),
file=sys.stderr)
return 2
..
except OSError as err:
print("%s: can't write '%s': %s"
% (sys.argv[0], err.filename, err.strerror),
file=sys.stderr)
though I notice now they are using 'sys.argv[0]' which is not the
same as 'test_name', as well as using stderr. So the consistency
isn't actually helped
>
> >
> >>
> >> > file=sys.stderr)
> >> > out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
> >> > err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
> >> > @@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
> >> > sys.stdout.writelines(err_diff)
> >> >
> >> > if not update:
> >> > + print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
> >> > + "if the QAPI schema generator was intentionally changed") % test_name,
> >> > + file=sys.stderr)
> >> > return 1
> >> >
> >> > try:
> >> > @@ -197,7 +200,7 @@ def main(argv):
> >> > parser.add_argument('-d', '--dir', action='store', default='',
> >> > help="directory containing tests")
> >> > parser.add_argument('-u', '--update', action='store_true',
> >> > - default='QAPI_TEST_UPDATE' in os.environ,
> >> > + default='QEMU_TEST_REGENERATE' in os.environ,
> >> > help="update expected test results")
> >> > parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
> >> > args = parser.parse_args()
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/8] tracetool: add test suite to improve reviewability
2025-08-06 16:48 [PATCH v3 0/8] tracetool: add test suite to improve reviewability Daniel P. Berrangé
` (7 preceding siblings ...)
2025-08-06 16:48 ` [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var Daniel P. Berrangé
@ 2025-08-07 20:06 ` Stefan Hajnoczi
8 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2025-08-07 20:06 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Michael Roth, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé, Mads Ynddal, Alex Bennée,
Thomas Huth
[-- Attachment #1: Type: text/plain, Size: 5138 bytes --]
On Wed, Aug 06, 2025 at 05:48:24PM +0100, Daniel P. Berrangé wrote:
> To repeat the start of the commit message in patch 5....
>
> When reviewing tracetool patches it is often very unclear what the
> expected output will be for the generated backends. Compounding
> this is that a default build will only enable the 'log' trace
> backend, so developers won't see generated code for other backends
> without making a special effort. Some backends are also platform
> specific, so can't be enabled in QEMU builds, even though tracetool
> could generate the code.
>
> To address this, introduce a test suite for tracetool which is
> conceptually similar to the qapi-schema test. It is a simple
> python program that runs tracetool and compares the actual output
> to historical reference output kept in git.
>
> This was inspired by noticing the now pointless "__nocheck__"
> method wrapping in the inline probe functions.
>
> Changed in v3:
>
> - Also modify the QAPI schema test to use QEMU_TEST_REGENERATE=1
> env & document / hint this
> - Make tracetool accept '-' as shorthand for stdout to
> avoid relative paths in the reference output
>
> Changed in v2:
>
> - Add tracetool info to docs/devel/testing.rst
>
> Daniel P. Berrangé (8):
> checkpatch: cull trailing '*/' in SPDX check
> tracetool: eliminate trailing whitespace in C format
> tracetool: avoid space after "*" in arg types
> tracetool: include SPDX-License-Identifier in generated files
> tracetool: support "-" as a shorthand for stdout
> tracetool: add test suite for tracetool with reference output
> tracetool: drop the probe "__nocheck__" wrapping
> qapi: switch to use QEMU_TEST_REGENERATE env var
>
> MAINTAINERS | 1 +
> docs/devel/testing/main.rst | 40 +++++++++
> scripts/checkpatch.pl | 3 +
> scripts/tracetool/__init__.py | 17 +++-
> scripts/tracetool/format/c.py | 3 +-
> scripts/tracetool/format/d.py | 3 +-
> scripts/tracetool/format/h.py | 17 +---
> scripts/tracetool/format/log_stap.py | 1 +
> scripts/tracetool/format/simpletrace_stap.py | 1 +
> scripts/tracetool/format/stap.py | 1 +
> scripts/tracetool/format/ust_events_c.py | 1 +
> scripts/tracetool/format/ust_events_h.py | 1 +
> tests/Makefile.include | 1 +
> tests/meson.build | 1 +
> tests/qapi-schema/test-qapi.py | 7 +-
> tests/tracetool/dtrace.c | 32 +++++++
> tests/tracetool/dtrace.d | 10 +++
> tests/tracetool/dtrace.h | 45 ++++++++++
> tests/tracetool/dtrace.log-stap | 15 ++++
> tests/tracetool/dtrace.simpletrace-stap | 16 ++++
> tests/tracetool/dtrace.stap | 14 +++
> tests/tracetool/ftrace.c | 32 +++++++
> tests/tracetool/ftrace.h | 59 +++++++++++++
> tests/tracetool/log.c | 32 +++++++
> tests/tracetool/log.h | 43 +++++++++
> tests/tracetool/meson.build | 25 ++++++
> tests/tracetool/simple.c | 61 +++++++++++++
> tests/tracetool/simple.h | 40 +++++++++
> tests/tracetool/syslog.c | 32 +++++++
> tests/tracetool/syslog.h | 43 +++++++++
> tests/tracetool/trace-events | 5 ++
> tests/tracetool/tracetool-test.py | 91 ++++++++++++++++++++
> tests/tracetool/ust.c | 32 +++++++
> tests/tracetool/ust.h | 41 +++++++++
> tests/tracetool/ust.ust-events-c | 14 +++
> tests/tracetool/ust.ust-events-h | 56 ++++++++++++
> 36 files changed, 813 insertions(+), 23 deletions(-)
> create mode 100644 tests/tracetool/dtrace.c
> create mode 100644 tests/tracetool/dtrace.d
> create mode 100644 tests/tracetool/dtrace.h
> create mode 100644 tests/tracetool/dtrace.log-stap
> create mode 100644 tests/tracetool/dtrace.simpletrace-stap
> create mode 100644 tests/tracetool/dtrace.stap
> create mode 100644 tests/tracetool/ftrace.c
> create mode 100644 tests/tracetool/ftrace.h
> create mode 100644 tests/tracetool/log.c
> create mode 100644 tests/tracetool/log.h
> create mode 100644 tests/tracetool/meson.build
> create mode 100644 tests/tracetool/simple.c
> create mode 100644 tests/tracetool/simple.h
> create mode 100644 tests/tracetool/syslog.c
> create mode 100644 tests/tracetool/syslog.h
> create mode 100644 tests/tracetool/trace-events
> create mode 100755 tests/tracetool/tracetool-test.py
> create mode 100644 tests/tracetool/ust.c
> create mode 100644 tests/tracetool/ust.h
> create mode 100644 tests/tracetool/ust.ust-events-c
> create mode 100644 tests/tracetool/ust.ust-events-h
Aside from the stdout filename comment I posted:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread