Re: [PATCH-for-4.2 v3 3/3] trace: Forbid dynamic field width in event format

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 11/18/19 10:26 PM, Eric Blake wrote:
> On 11/18/19 3:04 PM, Philippe Mathieu-Daudé wrote:
>> Since not all trace backends support dynamic field width in
>> format (dtrace via stap does not), forbid them.
>>
>> Add a check to refuse field width in new formats:
>>
>>    $ make
>>    [...]
>>      GEN     hw/block/trace.h
>>    Traceback (most recent call last):
>>      File "scripts/tracetool.py", line 152, in <module>
>>        main(sys.argv)
>>      File "scripts/tracetool.py", line 143, in main
>>        events.extend(tracetool.read_events(fh, arg))
>>      File "scripts/tracetool/__init__.py", line 371, in read_events
>>        event = Event.build(line)
>>      File "scripts/tracetool/__init__.py", line 285, in build
>>        raise ValueError("Event format must not contain field width '%*'")
>>    ValueError: Error at hw/block/trace-events:11: Event format must 
>> not contain field width '%*'
>>
>> Reported-by: Eric Blake <eblake@redhat.com>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
> 
>> +++ b/docs/devel/tracing.txt
>> @@ -113,7 +113,8 @@ Format strings should reflect the types defined in 
>> the trace event.  Take
>>   special care to use PRId64 and PRIu64 for int64_t and uint64_t types,
>>   respectively.  This ensures portability between 32- and 64-bit 
>> platforms.
>>   Format strings must not end with a newline character.  It is the 
>> responsibility
>> -of backends to adapt line ending for proper logging.
>> +of backends to adapt line ending for proper logging.  Format strings 
>> must not
>> +use numeric field width dynamic precision (SystemTap does not support 
>> them).
> 
> Reads awkwardly - a dynamic precision is not numeric in the format 
> string (but '*' instead).  Better might be:
> 
> Format strings must not use dynamic field width or precision ('*'), as 
> at least SystemTap does not support them.
> 
> Or even:
> 
> Format strings may use numeric field width or precision, but must not 
> use dynamic forms ('*') as at least SystemTap does not support that.
> 
>>   Each event declaration will start with the event name, then its 
>> arguments,
>>   finally a format string for pretty-printing. For example:
>> diff --git a/scripts/tracetool/__init__.py 
>> b/scripts/tracetool/__init__.py
>> index 44c118bc2a..ec7fe9fa4a 100644
>> --- a/scripts/tracetool/__init__.py
>> +++ b/scripts/tracetool/__init__.py
>> @@ -206,6 +206,7 @@ class Event(object):
>>                         "\s*"
>>                         "(?:(?:(?P<fmt_trans>\".+),)?\s*(?P<fmt>\".+))?"
>>                         "\s*")
>> +    _DFWRE = re.compile(r"%[\d\.\- +#']*\*") # dynamic width precision
> 
> The comment is slightly off - this catches both dynamic field width (any 
> '*' before '.') and dynamic precision (any '*' after '.'), maybe the fix 
> is just s/width/width or/
> 
>>       _VALID_PROPS = set(["disable", "tcg", "tcg-trans", "tcg-exec", 
>> "vcpu"])
>> @@ -280,6 +281,8 @@ class Event(object):
>>           if fmt.endswith(r'\n"'):
>>               raise ValueError("Event format must not end with a 
>> newline "
>>                                "character")
>> +        if Event._DFWRE.search(fmt):
>> +            raise ValueError("Event format must not contain field 
>> width '%*'")
> 
> and I don't know if you want to tweak the error message, maybe:
>   Event format must not use dynamic '*'
> 
> If we're trying to get stuff in 4.2-rc2, patch 1 and 2 are actual bug 
> fixes and deserve to go in; patch 3 is nice-to-have but doesn't affect 
> the build if it is omitted (as there are no other offenders left), so 
> slipping it into 5.0 for a v4 to clean it up slightly doesn't hurt.  I 
> don't know who would send the pull request, though, and slipping 1 and 2 
> into -rc3 just because of 3 is not ideal.

Wise thought. I'll respin 1-2 with the Fixes: tag in case qemu-block@ or 
qemu-trivial@ have pending pull-request for tomorrow, they can queue it.
Else they can go via the mips-next tree, since both patch affect the 
Malta board.

Thanks!

Phil.