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

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

[BALATON Zoltan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

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>"
+__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)
+
+
+def generate_h_backend_dstate(event, group):
+    out('    trace_event_get_state_dynamic_by_id(%(event_id)s) || \\',
+        event_id="TRACE_" + event.name.upper())
-- 
2.21.3

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

[Alex Bennée]

BALATON Zoltan <balaton@eik.bme.hu> writes:

Did this patch get separated from a larger series (2/2)?

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

I'd argue for this to be the behaviour of plain log (given it's mixing
with other log output). If not then maybe plainlog would be the default
log type if nothing is passed to configure?

>
> 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>"
> +__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)
> +
> +
> +def generate_h_backend_dstate(event, group):
> +    out('    trace_event_get_state_dynamic_by_id(%(event_id)s) || \\',
> +        event_id="TRACE_" + event.name.upper())


-- 
Alex Bennée

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

[Alex Bennée]

BALATON Zoltan <balaton@eik.bme.hu> writes:

Did this patch get separated from a larger series (2/2)?

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

I'd argue for this to be the behaviour of plain log (given it's mixing
with other log output). If not then maybe plainlog would be the default
log type if nothing is passed to configure?

>
> 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>"
> +__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)
> +
> +
> +def generate_h_backend_dstate(event, group):
> +    out('    trace_event_get_state_dynamic_by_id(%(event_id)s) || \\',
> +        event_id="TRACE_" + event.name.upper())


-- 
Alex Bennée

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On Wed, 17 Jun 2020, Alex Bennée wrote:
> Did this patch get separated from a larger series (2/2)?

No sorry, just used format-patch for two unrelated patches and forgot to 
remove this. This patch is standalone and Philippe pointed out the other 
one labelled 1/2 is not needed as there's already a similar patch queued.

>> 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.
>
> I'd argue for this to be the behaviour of plain log (given it's mixing
> with other log output). If not then maybe plainlog would be the default
> log type if nothing is passed to configure?

I'd like if the default log backend was not adding timestamps and random 
numbers to log messages for easier digesting and diffing but the current 
log backend does that and maybe there are people who like that default so 
instead of making them need to change their ways I'm proposing this 
backend that those who like plain logs can use instead. That's a less 
disruptive change than changing the default log backend but if others want 
that I'm fine with that too.

Regards,
BALATON Zoltan

>> 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>"
>> +__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)
>> +
>> +
>> +def generate_h_backend_dstate(event, group):
>> +    out('    trace_event_get_state_dynamic_by_id(%(event_id)s) || \\',
>> +        event_id="TRACE_" + event.name.upper())
>
>
>

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On Wed, 17 Jun 2020, Alex Bennée wrote:
> Did this patch get separated from a larger series (2/2)?

No sorry, just used format-patch for two unrelated patches and forgot to 
remove this. This patch is standalone and Philippe pointed out the other 
one labelled 1/2 is not needed as there's already a similar patch queued.

>> 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.
>
> I'd argue for this to be the behaviour of plain log (given it's mixing
> with other log output). If not then maybe plainlog would be the default
> log type if nothing is passed to configure?

I'd like if the default log backend was not adding timestamps and random 
numbers to log messages for easier digesting and diffing but the current 
log backend does that and maybe there are people who like that default so 
instead of making them need to change their ways I'm proposing this 
backend that those who like plain logs can use instead. That's a less 
disruptive change than changing the default log backend but if others want 
that I'm fine with that too.

Regards,
BALATON Zoltan

>> 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>"
>> +__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)
>> +
>> +
>> +def generate_h_backend_dstate(event, group):
>> +    out('    trace_event_get_state_dynamic_by_id(%(event_id)s) || \\',
>> +        event_id="TRACE_" + event.name.upper())
>
>
>

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

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.

> +__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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

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.

> +__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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Daniel P. Berrangé]

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.
> 
> > +__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().

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 2/2] scripts/tracetool: Add plainlog backend

[Daniel P. Berrangé]

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.
> 
> > +__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().

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 2/2] scripts/tracetool: Add plainlog backend

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3460 bytes --]

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

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3460 bytes --]

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]

On Thu, Jun 18, 2020 at 10:07:41AM +0100, 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.
> > 
> > > +__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 thought about that but the features are somewhat unrelated.

If we unify them, how about making the timestamp/tid apply to *all*
qemu_log() output, not just tracing?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]

On Thu, Jun 18, 2020 at 10:07:41AM +0100, 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.
> > 
> > > +__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 thought about that but the features are somewhat unrelated.

If we unify them, how about making the timestamp/tid apply to *all*
qemu_log() output, not just tracing?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Daniel P. Berrangé]

On Thu, Jun 18, 2020 at 04:35:16PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jun 18, 2020 at 10:07:41AM +0100, 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.
> > > 
> > > > +__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 thought about that but the features are somewhat unrelated.
> 
> If we unify them, how about making the timestamp/tid apply to *all*
> qemu_log() output, not just tracing?

That's exactly what I intended.

Essentially if QEMU is going to add timestamps to things it writes to
stdout/err, then it should do that universally for all parts of the code
base that use stdio. This means error_report(), qemu_log(), and any
other places that are relevant wrt stdio.

Having separate timestamp on/off switches for each feature is not
desirable.


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 2/2] scripts/tracetool: Add plainlog backend

[Daniel P. Berrangé]

On Thu, Jun 18, 2020 at 04:35:16PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jun 18, 2020 at 10:07:41AM +0100, 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.
> > > 
> > > > +__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 thought about that but the features are somewhat unrelated.
> 
> If we unify them, how about making the timestamp/tid apply to *all*
> qemu_log() output, not just tracing?

That's exactly what I intended.

Essentially if QEMU is going to add timestamps to things it writes to
stdout/err, then it should do that universally for all parts of the code
base that use stdio. This means error_report(), qemu_log(), and any
other places that are relevant wrt stdio.

Having separate timestamp on/off switches for each feature is not
desirable.


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 2/2] scripts/tracetool: Add plainlog backend

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4142 bytes --]

On Thu, Jun 18, 2020 at 04:43:23PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 18, 2020 at 04:35:16PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jun 18, 2020 at 10:07:41AM +0100, 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.
> > > > 
> > > > > +__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 thought about that but the features are somewhat unrelated.
> > 
> > If we unify them, how about making the timestamp/tid apply to *all*
> > qemu_log() output, not just tracing?
> 
> That's exactly what I intended.
> 
> Essentially if QEMU is going to add timestamps to things it writes to
> stdout/err, then it should do that universally for all parts of the code
> base that use stdio. This means error_report(), qemu_log(), and any
> other places that are relevant wrt stdio.
> 
> Having separate timestamp on/off switches for each feature is not
> desirable.

Okay. I'll take a look at this tomorrow.

Thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4142 bytes --]

On Thu, Jun 18, 2020 at 04:43:23PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 18, 2020 at 04:35:16PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jun 18, 2020 at 10:07:41AM +0100, 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.
> > > > 
> > > > > +__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 thought about that but the features are somewhat unrelated.
> > 
> > If we unify them, how about making the timestamp/tid apply to *all*
> > qemu_log() output, not just tracing?
> 
> That's exactly what I intended.
> 
> Essentially if QEMU is going to add timestamps to things it writes to
> stdout/err, then it should do that universally for all parts of the code
> base that use stdio. This means error_report(), qemu_log(), and any
> other places that are relevant wrt stdio.
> 
> Having separate timestamp on/off switches for each feature is not
> desirable.

Okay. I'll take a look at this tomorrow.

Thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]