1. Patchew
  2. QEMU
  3. [PATCH v3 0/8] tracetool: add test suite to improve reviewability
  4. View patch

[PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout

Daniel P. Berrangé posted 8 patches 4 months, 1 week ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Thomas Huth <thuth@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Mads Ynddal <mads@ynddal.dk>, Paolo Bonzini <pbonzini@redhat.com>, Markus Armbruster <armbru@redhat.com>, Michael Roth <michael.roth@amd.com>
There is a newer version of this series
[PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
Posted by Daniel P. Berrangé 4 months, 1 week ago 
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
Re: [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
Posted by Stefan Hajnoczi 4 months, 1 week ago 
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
>
Re: [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
Posted by Daniel P. Berrangé 3 months, 3 weeks ago 
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 :|
Re: [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
Posted by Stefan Hajnoczi 3 months, 3 weeks ago 
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 :|
>
Re: [PATCH v3 5/8] tracetool: support "-" as a shorthand for stdout
Posted by Daniel P. Berrangé 3 months, 3 weeks ago 
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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.