Re: [Qemu-devel] [PATCH v3 4/4] trace: add ability to do simple printf logging via systemtap

[Daniel P . Berrangé]

On Tue, Jan 22, 2019 at 11:42:31AM -0600, Eric Blake wrote:
> On 1/22/19 10:49 AM, Daniel P. Berrangé wrote:
> > The dtrace systemtap trace backend for QEMU is very powerful but it is
> > also somewhat unfriendly to users who aren't familiar with systemtap,
> > or who don't need its power right now.
> > 
> >   stap -e "....some strange script...."
> > 
> 
> > By default it monitors all existing running processes and all future
> > launched proceses. This can be restricted to a specific PID using the
> > --pid arg
> > 
> >  $ qemu-trace-stap run --pid 2532 qemu-system-x86_64 'qio*'
> > 
> 
> Can --pid mode be smart enough to check /proc/NNN/exe without the user
> having to specify the explicit qemu-system-x86_64 argument?  (Could be
> followup patch)

It could do that, but this complicates the parsing of command line
arguments, as the binary is a positional arg.

> > +++ b/scripts/qemu-trace-stap
> > @@ -0,0 +1,174 @@
> 
> > +#  QEMU SystemTap Trace Tool
> > +#
> > +#  This program is free software; you can redistribute it and/or modify
> > +#  it under the terms of the GNU General Public License as published by
> > +#  the Free Software Foundation; under version 2 of the License.
> > +#
> 
> Why GPLv2-only?

A mistake, it should be v2-or-later

> 
> > +def main():
> > +    parser = argparse.ArgumentParser(description="QEMU SystemTap trace 
> > tool")
> > +    parser.add_argument("-v", "--verbose", help="Print verbose progress 
> > info",
> > +                        action='store_true')
> > +
> > +    subparser = parser.add_subparsers(help="commands")
> > +    subparser.required = True
> > +    subparser.dest = "command"
> > +
> > +    runparser = subparser.add_parser("run", help="Run a trace session",
> > +                                     
> > formatter_class=argparse.RawDescriptionHelpFormatter,
> > +                                     epilog="""
> > +
> > +To watch all trace points on the qemu-system-x86_64 binary:
> > +
> > +   %(argv0)s run qemu-system-x86_64
> > +
> > +To only watch the trace points matching the qio* and qcrypto* patterns
> > +
> > +   %(argv0)s run qemu-system-x86_64 'qio*' 'qcrypto*'
> > +""" % {"argv0": sys.argv[0]})
> > +    runparser.set_defaults(func=cmd_run)
> > +    runparser.add_argument("--pid", "-p", dest="pid",
> > +                           help="Restrict tracing to a specific process 
> > ID")
> > +    runparser.add_argument("binary", help="QEMU system or user emulator 
> > binary")
> > +    runparser.add_argument("probes", help="Probe names or wildcards",
> > +                           nargs=argparse.REMAINDER)
> > +
> > +    listparser = subparser.add_parser("list", help="List probe points",
> > +                                      
> > formatter_class=argparse.RawDescriptionHelpFormatter,
> > +                                      epilog="""
> > +
> > +To list all trace points on the qemu-system-x86_64 binary:
> > +
> > +   %(argv0)s list qemu-system-x86_64
> 
> No mention of --pid mode in the --help output?

getopt generates --help output listing all arguments. The epilog is
just a bit of text to augment what it already prints



> Typos are trivial, licensing less so, but I trust you can fix licensing
> without me having to see a respin (as it won't change code).  So:
> Reviewed-by: Eric Blake <address@hidden>

Will incorporate these changes


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 :|