Re: [Qemu-devel] [PATCH] log-for-trace.h: Split out parts of log.h used by trace.h

[Peter Maydell]

On 13 February 2018 at 15:19, Eric Blake <address@hidden> 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.)

>> -        '                      "address@hidden:%(name)s " %(fmt)s
>> "\\n",',
>> -        '                      getpid(),',
>> -        '                      (size_t)_now.tv_sec,
>> (size_t)_now.tv_usec',
>> -        '                      %(argnames)s);',
>> +        '        qemu_log("address@hidden:%(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 <address@hidden>

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