Re: [PATCH 2/2] scripts/tracetool: Add plainlog backend

[BALATON Zoltan]

On Thu, 18 Jun 2020, Daniel P. Berrangé wrote:
> On Thu, Jun 18, 2020 at 08:31:24AM +0100, Stefan Hajnoczi wrote:
> > On Wed, Jun 17, 2020 at 03:36:29PM +0200, BALATON Zoltan wrote:
> > > Add a backend that is the same as the log backend but omits the
> > > process id and timestamp so logs are easier to read and diff-able.
> > >
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > ---
> > >  scripts/tracetool/backend/plainlog.py | 48 +++++++++++++++++++++++++++
> > >  1 file changed, 48 insertions(+)
> > >  create mode 100644 scripts/tracetool/backend/plainlog.py
> > >
> > > diff --git a/scripts/tracetool/backend/plainlog.py 
> > > b/scripts/tracetool/backend/plainlog.py
> > > new file mode 100644
> > > index 0000000000..40bbfa6d76
> > > --- /dev/null
> > > +++ b/scripts/tracetool/backend/plainlog.py
> > > @@ -0,0 +1,48 @@
> > > +# -*- coding: utf-8 -*-
> > > +
> > > +"""
> > > +Stderr built-in backend, plain log without proc ID and time.
> > > +"""
> > > +
> > > +__author__     = "Llu????s Vilanova <vilanova@ac.upc.edu>"
> > > +__copyright__  = "Copyright 2012-2017, Llu????s Vilanova <vilanova@ac.upc.edu>"
> >
> > There is a Unicode issue here, Lluís' name is not printed correctly.

It's just an issue with the email formatting, should be OK if you read it 
in UTF8 which is the default on most machines nowaday just forgot to add 
corresponding email header.

> > > +__license__    = "GPL version 2 or (at your option) any later version"
> > > +
> > > +__maintainer__ = "Stefan Hajnoczi"
> > > +__email__      = "stefanha@linux.vnet.ibm.com"
> > > +
> > > +
> > > +from tracetool import out
> > > +
> > > +
> > > +PUBLIC = True
> > > +
> > > +
> > > +def generate_h_begin(events, group):
> > > +    out('#include "qemu/log-for-trace.h"',
> > > +        '')
> > > +
> > > +
> > > +def generate_h(event, group):
> > > +    argnames = ", ".join(event.args.names())
> > > +    if len(event.args) > 0:
> > > +        argnames = ", " + argnames
> > > +
> > > +    if "vcpu" in event.properties:
> > > +        # already checked on the generic format code
> > > +        cond = "true"
> > > +    else:
> > > +        cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
> > > +
> > > +    out('    if (%(cond)s && qemu_loglevel_mask(LOG_TRACE)) {',
> > > +        '        qemu_log("%(name)s " %(fmt)s "\\n" %(argnames)s);',
> > > +        '    }',
> > > +        cond=cond,
> > > +        name=event.name,
> > > +        fmt=event.fmt.rstrip("\n"),
> > > +        argnames=argnames)
> >
> > It is not necessary to introduce a new backend. There could be an option
> > that controls whether or not the timestamp/tid is printed. For example,
> > -trace timestamp=off or maybe the timestmap/tid can be integrated into
> > qemu_log() itself so that it's used more consistently and a -d timestamp
> > option enables it.
>
> QEMU already has a "-msg timestamp=on|off" option that controls whether
> error reports on stderr get a timestamp. I think it is probably reasonable
> for this existing option to apply to anything QEMU prints to stdout/err,
> and thus we could wire it up for qemu_log().

I don't know how to do that so can you submit a patch please? It would be 
preferable if an additional command line option wasn't necessary to print 
logs without timestamps (so changing the defailt to that if you don't mind 
changing current trace behaviour) or the default could be set at configure 
time like with this patch --enable-trace-backends=plainlog sets that 
(although this does not have option to change it at runtime).

Regards,
BALATON Zoltan