| Message ID | 20191118210458.11959-4-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Remove dynamic field width from trace events | expand |
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.
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.
diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt index 8c0376fefa..6c01ce801e 100644 --- a/docs/devel/tracing.txt +++ 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). 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 _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 '%*'") if len(fmt_trans) > 0: fmt = [fmt_trans, fmt]
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> --- v3: - use better regex provided by Eric, - instead of re.match(), use re.search() which takes unanchored regex, - added a comment in tracing.txt --- docs/devel/tracing.txt | 3 ++- scripts/tracetool/__init__.py | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-)