On 13 February 2018 at 15:19, Eric Blake <ebl...@redhat.com> wrote:
> On 02/13/2018 08:00 AM, Peter Maydell wrote:
>> +++ b/scripts/tracetool/backend/log.py
>> @@ -20,7 +20,7 @@ PUBLIC = True
>> def generate_h_begin(events, group):
>> - out('#include "qemu/log.h"',
>> + out('#include "qemu/log-for-trace.h"',
>> '')
>> @@ -35,14 +35,13 @@ def generate_h(event, group):
>> else:
>> cond = "trace_event_get_state(%s)" % ("TRACE_" +
>> event.name.upper())
>> - out(' if (%(cond)s) {',
>> + out(' if (%(cond)s && qemu_loglevel_mask(LOG_TRACE)) {',
>> ' struct timeval _now;',
>> ' gettimeofday(&_now, NULL);',
>> - ' qemu_log_mask(LOG_TRACE,',
>
>
> Oh, nice side effect - the old code was unconditionally calling
> gettimeofday() even when qemu_loglevel_mask(LOG_TRACE) fails; the new code
> limits the call when the logging is actually going to happen.
True, but I think in practice if the trace_event_get_state()
check succeeds then LOG_TRACE will always be on.
(Slightly oddly, qemu_str_to_log_mask() only sets LOG_TRACE if
a trace:foo event was enabled, but qemu_set_log() forces LOG_TRACE
to on even if no trace events were enabled.)
>> - ' "%%d@%%zd.%%06zd:%(name)s " %(fmt)s
>> "\\n",',
>> - ' getpid(),',
>> - ' (size_t)_now.tv_sec,
>> (size_t)_now.tv_usec',
>> - ' %(argnames)s);',
>> + ' qemu_log("%%d@%%zd.%%06zd:%(name)s " %(fmt)s "\\n",',
>> + ' getpid(),',
>> + ' (size_t)_now.tv_sec, (size_t)_now.tv_usec',
>> + ' %(argnames)s);',
>> ' }',
>> cond=cond,
>> name=event.name,
>>
>
> If you don't think the extra preprocessor magic to prevent accidental
> inclusion of the internal header is necessary, then
> Reviewed-by: Eric Blake <ebl...@redhat.com>
It doesn't seem very likely in practice that anybody would
include the obscure internal header. We can add the magic
later if the mistake seems to happen in practice...
thanks
-- PMM
