* [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module
2022-01-21 16:22 [PATCH v4 0/3] trace qmp commands Vladimir Sementsov-Ogievskiy
@ 2022-01-21 16:22 ` Vladimir Sementsov-Ogievskiy
2022-01-21 17:56 ` John Snow
2022-01-25 9:07 ` Markus Armbruster
2022-01-21 16:22 ` [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg Vladimir Sementsov-Ogievskiy
2022-01-21 16:22 ` [PATCH v4 3/3] meson: generate trace events for qmp commands Vladimir Sementsov-Ogievskiy
2 siblings, 2 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-21 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, michael.roth, armbru, vsementsov, jsnow, hreitz, kwolf,
pbonzini
We are going to generate trace events for qmp commands. We should
generate both trace_*() function calls and trace-events files listing
events for trace generator.
So, add an output module FOO.trace-events for each FOO schema module.
Still, we'll need these .trace-events files only for
QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
So, make this possibility optional, to avoid generating extra empty
files for all other successors of QAPISchemaModularCVisitor.
We can't simply add the new feature directly to
QAPISchemaGenCommandVisitor: this means we'll have to reimplement
a kind of ._module / .write() functionality of parent class in the
successor, which seems worse than extending base class functionality.
Currently nobody set add_trace_events to True, so new functionality is
formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
in further commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
scripts/qapi/gen.py | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 995a97d2b8..def52f021e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -192,6 +192,11 @@ def _bottom(self) -> str:
return guardend(self.fname)
+class QAPIGenTrace(QAPIGen):
+ def _top(self):
+ return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n'
+
+
@contextmanager
def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
"""
@@ -244,15 +249,18 @@ def __init__(self,
what: str,
user_blurb: str,
builtin_blurb: Optional[str],
- pydoc: str):
+ pydoc: str,
+ add_trace_events: bool = False):
self._prefix = prefix
self._what = what
self._user_blurb = user_blurb
self._builtin_blurb = builtin_blurb
self._pydoc = pydoc
self._current_module: Optional[str] = None
- self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
+ self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH,
+ Optional[QAPIGenTrace]]] = {}
self._main_module: Optional[str] = None
+ self.add_trace_events = add_trace_events
@property
def _genc(self) -> QAPIGenC:
@@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
assert self._current_module is not None
return self._module[self._current_module][1]
+ @property
+ def _gent(self) -> QAPIGenTrace:
+ assert self.add_trace_events
+ assert self._current_module is not None
+ gent = self._module[self._current_module][2]
+ assert gent is not None
+ return gent
+
@staticmethod
def _module_dirname(name: str) -> str:
if QAPISchemaModule.is_user_module(name):
@@ -293,7 +309,12 @@ def _add_module(self, name: str, blurb: str) -> None:
basename = self._module_filename(self._what, name)
genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
- self._module[name] = (genc, genh)
+ if self.add_trace_events:
+ gent = QAPIGenTrace(basename + '.trace-events')
+ else:
+ gent = None
+
+ self._module[name] = (genc, genh, gent)
self._current_module = name
@contextmanager
@@ -304,11 +325,13 @@ def _temp_module(self, name: str) -> Iterator[None]:
self._current_module = old_module
def write(self, output_dir: str, opt_builtins: bool = False) -> None:
- for name, (genc, genh) in self._module.items():
+ for name, (genc, genh, gent) in self._module.items():
if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
continue
genc.write(output_dir)
genh.write(output_dir)
+ if gent is not None:
+ gent.write(output_dir)
def _begin_builtin_module(self) -> None:
pass
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module
2022-01-21 16:22 ` [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module Vladimir Sementsov-Ogievskiy
@ 2022-01-21 17:56 ` John Snow
2022-01-25 9:07 ` Markus Armbruster
1 sibling, 0 replies; 15+ messages in thread
From: John Snow @ 2022-01-21 17:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, Michael Roth, qemu-devel, Markus Armbruster,
Hanna Reitz, Stefan Hajnoczi, Paolo Bonzini
On Fri, Jan 21, 2022 at 11:22 AM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> We are going to generate trace events for qmp commands. We should
> generate both trace_*() function calls and trace-events files listing
> events for trace generator.
>
> So, add an output module FOO.trace-events for each FOO schema module.
>
> Still, we'll need these .trace-events files only for
> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
> So, make this possibility optional, to avoid generating extra empty
> files for all other successors of QAPISchemaModularCVisitor.
>
> We can't simply add the new feature directly to
> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
> a kind of ._module / .write() functionality of parent class in the
> successor, which seems worse than extending base class functionality.
>
> Currently nobody set add_trace_events to True, so new functionality is
> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
> in further commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> scripts/qapi/gen.py | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 995a97d2b8..def52f021e 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -192,6 +192,11 @@ def _bottom(self) -> str:
> return guardend(self.fname)
>
>
> +class QAPIGenTrace(QAPIGen):
> + def _top(self):
-> str:
> + return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n'
> +
> +
> @contextmanager
> def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
> """
> @@ -244,15 +249,18 @@ def __init__(self,
> what: str,
> user_blurb: str,
> builtin_blurb: Optional[str],
> - pydoc: str):
> + pydoc: str,
> + add_trace_events: bool = False):
> self._prefix = prefix
> self._what = what
> self._user_blurb = user_blurb
> self._builtin_blurb = builtin_blurb
> self._pydoc = pydoc
> self._current_module: Optional[str] = None
> - self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
> + self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH,
> + Optional[QAPIGenTrace]]] = {}
> self._main_module: Optional[str] = None
> + self.add_trace_events = add_trace_events
>
> @property
> def _genc(self) -> QAPIGenC:
> @@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
> assert self._current_module is not None
> return self._module[self._current_module][1]
>
> + @property
> + def _gent(self) -> QAPIGenTrace:
> + assert self.add_trace_events
> + assert self._current_module is not None
> + gent = self._module[self._current_module][2]
> + assert gent is not None
> + return gent
> +
> @staticmethod
> def _module_dirname(name: str) -> str:
> if QAPISchemaModule.is_user_module(name):
> @@ -293,7 +309,12 @@ def _add_module(self, name: str, blurb: str) -> None:
> basename = self._module_filename(self._what, name)
> genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
> genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> - self._module[name] = (genc, genh)
gent: Optional[QAPIGenTrace]
> + if self.add_trace_events:
> + gent = QAPIGenTrace(basename + '.trace-events')
> + else:
> + gent = None
> +
> + self._module[name] = (genc, genh, gent)
> self._current_module = name
>
> @contextmanager
> @@ -304,11 +325,13 @@ def _temp_module(self, name: str) -> Iterator[None]:
> self._current_module = old_module
>
> def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> - for name, (genc, genh) in self._module.items():
> + for name, (genc, genh, gent) in self._module.items():
> if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
> continue
> genc.write(output_dir)
> genh.write(output_dir)
> + if gent is not None:
> + gent.write(output_dir)
>
> def _begin_builtin_module(self) -> None:
> pass
> --
> 2.31.1
>
... Sorry, I didn't finish typing this module 100%, so the scripts
aren't in the tree yet. I'll have to resume that project soon, I am
just trying to juggle a lot of things simultaneously. forgive me!
but, these should work:
> cd ~/src/qemu/scripts
> mypy --config-file=qapi/mypy.ini qapi/
> flake8 --config=qapi/.flake8 qapi/
pylint and isort has had some small regressions, so don't worry about
those as much:
> pylint --rcfile=qapi/pylintrc qapi/
************* Module qapi.events
qapi/events.py:112:8: C0103: Variable name "f" doesn't conform to
snake_case naming style (invalid-name)
--js
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module
2022-01-21 16:22 ` [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module Vladimir Sementsov-Ogievskiy
2022-01-21 17:56 ` John Snow
@ 2022-01-25 9:07 ` Markus Armbruster
2022-01-25 9:50 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2022-01-25 9:07 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, michael.roth, qemu-devel, armbru, hreitz, stefanha,
pbonzini, jsnow
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> We are going to generate trace events for qmp commands. We should
QMP
> generate both trace_*() function calls and trace-events files listing
> events for trace generator.
>
> So, add an output module FOO.trace-events for each FOO schema module.
>
> Still, we'll need these .trace-events files only for
> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
> So, make this possibility optional, to avoid generating extra empty
> files for all other successors of QAPISchemaModularCVisitor.
Suggest to make this slightly less technical for easier reading:
Since we're going to add trace events only to command marshallers,
make the trace-events output optional, so we don't generate so many
useless empty files.
> We can't simply add the new feature directly to
> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
> a kind of ._module / .write() functionality of parent class in the
> successor, which seems worse than extending base class functionality.
Do you mean something like
The alternative would be adding the the new feature directly to
QAPISchemaGenCommandVisitor, but then we'd have to reimplement the
._module / .write() functionality of its parent class
QAPISchemaModularCVisitor, which seems worse than extending the parent
class.
?
If yes: I'm not sure about "worse". But keeping it in the parent class
feels right to me anyway, as trace events could be useful in other child
classes, too.
> Currently nobody set add_trace_events to True, so new functionality is
> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
Drop "formally".
> in further commit.
"in a further commit", or "in the next commit".
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> scripts/qapi/gen.py | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 995a97d2b8..def52f021e 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -192,6 +192,11 @@ def _bottom(self) -> str:
> return guardend(self.fname)
>
>
> +class QAPIGenTrace(QAPIGen):
> + def _top(self):
> + return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n'
> +
> +
> @contextmanager
> def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
> """
> @@ -244,15 +249,18 @@ def __init__(self,
> what: str,
> user_blurb: str,
> builtin_blurb: Optional[str],
> - pydoc: str):
> + pydoc: str,
> + add_trace_events: bool = False):
I'd prefer naming this gen_trace_events. Happy to tweak this in my tree.
> self._prefix = prefix
> self._what = what
> self._user_blurb = user_blurb
> self._builtin_blurb = builtin_blurb
> self._pydoc = pydoc
> self._current_module: Optional[str] = None
> - self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
> + self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH,
> + Optional[QAPIGenTrace]]] = {}
> self._main_module: Optional[str] = None
> + self.add_trace_events = add_trace_events
By convention, names of private attributes start with a single _.
>
> @property
> def _genc(self) -> QAPIGenC:
> @@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
> assert self._current_module is not None
> return self._module[self._current_module][1]
>
> + @property
> + def _gent(self) -> QAPIGenTrace:
> + assert self.add_trace_events
> + assert self._current_module is not None
> + gent = self._module[self._current_module][2]
> + assert gent is not None
> + return gent
> +
> @staticmethod
> def _module_dirname(name: str) -> str:
> if QAPISchemaModule.is_user_module(name):
> @@ -293,7 +309,12 @@ def _add_module(self, name: str, blurb: str) -> None:
> basename = self._module_filename(self._what, name)
> genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
> genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> - self._module[name] = (genc, genh)
> + if self.add_trace_events:
> + gent = QAPIGenTrace(basename + '.trace-events')
> + else:
> + gent = None
> +
> + self._module[name] = (genc, genh, gent)
> self._current_module = name
>
> @contextmanager
> @@ -304,11 +325,13 @@ def _temp_module(self, name: str) -> Iterator[None]:
> self._current_module = old_module
>
> def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> - for name, (genc, genh) in self._module.items():
> + for name, (genc, genh, gent) in self._module.items():
> if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
> continue
> genc.write(output_dir)
> genh.write(output_dir)
> + if gent is not None:
> + gent.write(output_dir)
>
> def _begin_builtin_module(self) -> None:
> pass
I wonder whether we really need a new __init__() parameter. Could we
have ._gent() create the module on demand? This is *not* a demand.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module
2022-01-25 9:07 ` Markus Armbruster
@ 2022-01-25 9:50 ` Vladimir Sementsov-Ogievskiy
2022-01-26 6:51 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-25 9:50 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, stefanha, michael.roth, jsnow, hreitz, kwolf,
pbonzini
25.01.2022 12:07, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> We are going to generate trace events for qmp commands. We should
>
> QMP
>
>> generate both trace_*() function calls and trace-events files listing
>> events for trace generator.
>>
>> So, add an output module FOO.trace-events for each FOO schema module.
>>
>> Still, we'll need these .trace-events files only for
>> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
>> So, make this possibility optional, to avoid generating extra empty
>> files for all other successors of QAPISchemaModularCVisitor.
>
> Suggest to make this slightly less technical for easier reading:
>
> Since we're going to add trace events only to command marshallers,
> make the trace-events output optional, so we don't generate so many
> useless empty files.
Sounds good
>
>> We can't simply add the new feature directly to
>> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
>> a kind of ._module / .write() functionality of parent class in the
>> successor, which seems worse than extending base class functionality.
>
> Do you mean something like
>
> The alternative would be adding the the new feature directly to
> QAPISchemaGenCommandVisitor, but then we'd have to reimplement the
> ._module / .write() functionality of its parent class
> QAPISchemaModularCVisitor, which seems worse than extending the parent
> class.
>
> ?
Yes.
>
> If yes: I'm not sure about "worse".
Hmm. *shrug* ) I'm new to this code, that's why it seems unobvious to me, and that's why I'm afraid of deeper refactoring)
> But keeping it in the parent class
> feels right to me anyway, as trace events could be useful in other child
> classes, too.
If it is OK, we can simply drop this paragraph.
>
>> Currently nobody set add_trace_events to True, so new functionality is
>> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
>
> Drop "formally".
>
>> in further commit.
>
> "in a further commit", or "in the next commit".
>
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> scripts/qapi/gen.py | 31 +++++++++++++++++++++++++++----
>> 1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index 995a97d2b8..def52f021e 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -192,6 +192,11 @@ def _bottom(self) -> str:
>> return guardend(self.fname)
>>
>>
>> +class QAPIGenTrace(QAPIGen):
>> + def _top(self):
>> + return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n'
>> +
>> +
>> @contextmanager
>> def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
>> """
>> @@ -244,15 +249,18 @@ def __init__(self,
>> what: str,
>> user_blurb: str,
>> builtin_blurb: Optional[str],
>> - pydoc: str):
>> + pydoc: str,
>> + add_trace_events: bool = False):
>
> I'd prefer naming this gen_trace_events. Happy to tweak this in my tree.
>
>> self._prefix = prefix
>> self._what = what
>> self._user_blurb = user_blurb
>> self._builtin_blurb = builtin_blurb
>> self._pydoc = pydoc
>> self._current_module: Optional[str] = None
>> - self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
>> + self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH,
>> + Optional[QAPIGenTrace]]] = {}
>> self._main_module: Optional[str] = None
>> + self.add_trace_events = add_trace_events
>
> By convention, names of private attributes start with a single _.
>
>>
>> @property
>> def _genc(self) -> QAPIGenC:
>> @@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
>> assert self._current_module is not None
>> return self._module[self._current_module][1]
>>
>> + @property
>> + def _gent(self) -> QAPIGenTrace:
>> + assert self.add_trace_events
>> + assert self._current_module is not None
>> + gent = self._module[self._current_module][2]
>> + assert gent is not None
>> + return gent
>> +
>> @staticmethod
>> def _module_dirname(name: str) -> str:
>> if QAPISchemaModule.is_user_module(name):
>> @@ -293,7 +309,12 @@ def _add_module(self, name: str, blurb: str) -> None:
>> basename = self._module_filename(self._what, name)
>> genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>> genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
>> - self._module[name] = (genc, genh)
>> + if self.add_trace_events:
>> + gent = QAPIGenTrace(basename + '.trace-events')
>> + else:
>> + gent = None
>> +
>> + self._module[name] = (genc, genh, gent)
>> self._current_module = name
>>
>> @contextmanager
>> @@ -304,11 +325,13 @@ def _temp_module(self, name: str) -> Iterator[None]:
>> self._current_module = old_module
>>
>> def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>> - for name, (genc, genh) in self._module.items():
>> + for name, (genc, genh, gent) in self._module.items():
>> if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
>> continue
>> genc.write(output_dir)
>> genh.write(output_dir)
>> + if gent is not None:
>> + gent.write(output_dir)
>>
>> def _begin_builtin_module(self) -> None:
>> pass
>
> I wonder whether we really need a new __init__() parameter. Could we
> have ._gent() create the module on demand? This is *not* a demand.
>
My first attempt to drop extra empty generated .trace-events files was to teach QAPIGenTrace() not to generate file when it is empty. But in this case some empty .trace-events for commands are not generated, and "include" line fails to compile. And at time when include line is generated, I don't know will corresponding .trace-events be empty or not. So I decided to make a new parameter for __init__()
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module
2022-01-25 9:50 ` Vladimir Sementsov-Ogievskiy
@ 2022-01-26 6:51 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2022-01-26 6:51 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, michael.roth, qemu-devel, hreitz, stefanha, pbonzini,
jsnow
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 25.01.2022 12:07, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> We are going to generate trace events for qmp commands. We should
>>
>> QMP
>>
>>> generate both trace_*() function calls and trace-events files listing
>>> events for trace generator.
>>>
>>> So, add an output module FOO.trace-events for each FOO schema module.
>>>
>>> Still, we'll need these .trace-events files only for
>>> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
>>> So, make this possibility optional, to avoid generating extra empty
>>> files for all other successors of QAPISchemaModularCVisitor.
>>
>> Suggest to make this slightly less technical for easier reading:
>>
>> Since we're going to add trace events only to command marshallers,
>> make the trace-events output optional, so we don't generate so many
>> useless empty files.
>
> Sounds good
>
>>
>>> We can't simply add the new feature directly to
>>> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
>>> a kind of ._module / .write() functionality of parent class in the
>>> successor, which seems worse than extending base class functionality.
>>
>> Do you mean something like
>>
>> The alternative would be adding the the new feature directly to
>> QAPISchemaGenCommandVisitor, but then we'd have to reimplement the
>> ._module / .write() functionality of its parent class
>> QAPISchemaModularCVisitor, which seems worse than extending the parent
>> class.
>>
>> ?
>
> Yes.
>
>>
>> If yes: I'm not sure about "worse".
>
> Hmm. *shrug* ) I'm new to this code, that's why it seems unobvious to me, and that's why I'm afraid of deeper refactoring)
>
>> But keeping it in the parent class
>> feels right to me anyway, as trace events could be useful in other child
>> classes, too.
>
> If it is OK, we can simply drop this paragraph.
Works for me.
Keeping it would work, too. "Seems worse" is an opinion, not wrong.
>>> Currently nobody set add_trace_events to True, so new functionality is
>>> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
>>
>> Drop "formally".
>>
>>> in further commit.
>>
>> "in a further commit", or "in the next commit".
>>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[...]
>> I wonder whether we really need a new __init__() parameter. Could we
>> have ._gent() create the module on demand? This is *not* a demand.
>>
>
> My first attempt to drop extra empty generated .trace-events files was to teach QAPIGenTrace() not to generate file when it is empty. But in this case some empty .trace-events for commands are not generated, and "include" line fails to compile. And at time when include line is generated, I don't know will corresponding .trace-events be empty or not. So I decided to make a new parameter for __init__()
Okay. We can always improve on top if we care.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg
2022-01-21 16:22 [PATCH v4 0/3] trace qmp commands Vladimir Sementsov-Ogievskiy
2022-01-21 16:22 ` [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module Vladimir Sementsov-Ogievskiy
@ 2022-01-21 16:22 ` Vladimir Sementsov-Ogievskiy
2022-01-25 10:02 ` Markus Armbruster
2022-01-21 16:22 ` [PATCH v4 3/3] meson: generate trace events for qmp commands Vladimir Sementsov-Ogievskiy
2 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-21 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, michael.roth, armbru, vsementsov, jsnow, hreitz, kwolf,
pbonzini
We are going to generate trace events for qmp commands. We should
generate both trace_*() function calls and trace-events files listing
events for trace generator.
Now implement that possibility in gen_commands() function.
The functionality will be used in the following commit, and now comment
in _begin_user_module() about c_name() is a bit premature.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
scripts/qapi/commands.py | 101 +++++++++++++++++++++++++++++++++------
scripts/qapi/main.py | 2 +-
2 files changed, 88 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 21001bbd6b..166bf5dcbc 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -53,7 +53,8 @@ def gen_command_decl(name: str,
def gen_call(name: str,
arg_type: Optional[QAPISchemaObjectType],
boxed: bool,
- ret_type: Optional[QAPISchemaType]) -> str:
+ ret_type: Optional[QAPISchemaType],
+ add_trace_events: bool) -> str:
ret = ''
argstr = ''
@@ -71,21 +72,67 @@ def gen_call(name: str,
if ret_type:
lhs = 'retval = '
- ret = mcgen('''
+ name = c_name(name)
+ upper = name.upper()
- %(lhs)sqmp_%(c_name)s(%(args)s&err);
- error_propagate(errp, err);
-''',
- c_name=c_name(name), args=argstr, lhs=lhs)
- if ret_type:
+ if add_trace_events:
ret += mcgen('''
+
+ if (trace_event_get_state_backends(TRACE_QMP_ENTER_%(upper)s)) {
+ g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
+
+ trace_qmp_enter_%(name)s(req_json->str);
+ }
+ ''',
+ upper=upper, name=name)
+
+ ret += mcgen('''
+
+ %(lhs)sqmp_%(name)s(%(args)s&err);
+''',
+ name=name, args=argstr, lhs=lhs)
+
+ ret += mcgen('''
if (err) {
+''')
+
+ if add_trace_events:
+ ret += mcgen('''
+ trace_qmp_exit_%(name)s(error_get_pretty(err), false);
+''',
+ name=name)
+
+ ret += mcgen('''
+ error_propagate(errp, err);
goto out;
}
+''')
+
+ if ret_type:
+ ret += mcgen('''
qmp_marshal_output_%(c_name)s(retval, ret, errp);
''',
c_name=ret_type.c_name())
+
+ if add_trace_events:
+ if ret_type:
+ ret += mcgen('''
+
+ if (trace_event_get_state_backends(TRACE_QMP_EXIT_%(upper)s)) {
+ g_autoptr(GString) ret_json = qobject_to_json(*ret);
+
+ trace_qmp_exit_%(name)s(ret_json->str, true);
+ }
+ ''',
+ upper=upper, name=name)
+ else:
+ ret += mcgen('''
+
+ trace_qmp_exit_%(name)s("{}", true);
+ ''',
+ name=name)
+
return ret
@@ -122,10 +169,19 @@ def gen_marshal_decl(name: str) -> str:
proto=build_marshal_proto(name))
+def gen_trace(name: str) -> str:
+ return mcgen('''
+qmp_enter_%(name)s(const char *json) "%%s"
+qmp_exit_%(name)s(const char *result, bool succeeded) "%%s %%d"
+''',
+ name=c_name(name))
+
+
def gen_marshal(name: str,
arg_type: Optional[QAPISchemaObjectType],
boxed: bool,
- ret_type: Optional[QAPISchemaType]) -> str:
+ ret_type: Optional[QAPISchemaType],
+ add_trace_events: bool) -> str:
have_args = boxed or (arg_type and not arg_type.is_empty())
if have_args:
assert arg_type is not None
@@ -180,7 +236,7 @@ def gen_marshal(name: str,
}
''')
- ret += gen_call(name, arg_type, boxed, ret_type)
+ ret += gen_call(name, arg_type, boxed, ret_type, add_trace_events)
ret += mcgen('''
@@ -238,11 +294,13 @@ def gen_register_command(name: str,
class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
- def __init__(self, prefix: str):
+ def __init__(self, prefix: str, add_trace_events: bool):
super().__init__(
prefix, 'qapi-commands',
- ' * Schema-defined QAPI/QMP commands', None, __doc__)
+ ' * Schema-defined QAPI/QMP commands', None, __doc__,
+ add_trace_events=add_trace_events)
self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
+ self.add_trace_events = add_trace_events
def _begin_user_module(self, name: str) -> None:
self._visited_ret_types[self._genc] = set()
@@ -261,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
''',
commands=commands, visit=visit))
+
+ if self.add_trace_events and commands != 'qapi-commands':
+ self._genc.add(mcgen('''
+#include "trace/trace-qapi.h"
+#include "qapi/qmp/qjson.h"
+#include "trace/trace-%(nm)s_trace_events.h"
+''',
+ nm=c_name(commands, protect=False)))
+ # We use c_name(commands, protect=False) to turn '-' into '_', to
+ # match .underscorify() in trace/meson.build
+
self._genh.add(mcgen('''
#include "%(types)s.h"
@@ -322,7 +391,10 @@ def visit_command(self,
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
self._genh.add(gen_marshal_decl(name))
- self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
+ self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
+ self.add_trace_events))
+ if self.add_trace_events:
+ self._gent.add(gen_trace(name))
with self._temp_module('./init'):
with ifcontext(ifcond, self._genh, self._genc):
self._genc.add(gen_register_command(
@@ -332,7 +404,8 @@ def visit_command(self,
def gen_commands(schema: QAPISchema,
output_dir: str,
- prefix: str) -> None:
- vis = QAPISchemaGenCommandVisitor(prefix)
+ prefix: str,
+ add_trace_events: bool) -> None:
+ vis = QAPISchemaGenCommandVisitor(prefix, add_trace_events)
schema.visit(vis)
vis.write(output_dir)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index f2ea6e0ce4..2e61409f04 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -49,7 +49,7 @@ def generate(schema_file: str,
schema = QAPISchema(schema_file)
gen_types(schema, output_dir, prefix, builtins)
gen_visit(schema, output_dir, prefix, builtins)
- gen_commands(schema, output_dir, prefix)
+ gen_commands(schema, output_dir, prefix, False)
gen_events(schema, output_dir, prefix)
gen_introspect(schema, output_dir, prefix, unmask)
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg
2022-01-21 16:22 ` [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg Vladimir Sementsov-Ogievskiy
@ 2022-01-25 10:02 ` Markus Armbruster
2022-01-25 10:20 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2022-01-25 10:02 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, michael.roth, qemu-devel, hreitz, stefanha, pbonzini,
jsnow
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> We are going to generate trace events for qmp commands. We should
> generate both trace_*() function calls and trace-events files listing
> events for trace generator.
>
> Now implement that possibility in gen_commands() function.
>
> The functionality will be used in the following commit, and now comment
> in _begin_user_module() about c_name() is a bit premature.
Neglects to explain why it needs to be optional.
Suggest
qapi/commands: Optionally generate trace for QMP commands
Add trace generation disabled. The next commit will enable it for
qapi/, but not for qga/ and tests/. Making it work for the latter two
would involve some Meson hackery to ensure we generate the
trace-events files before trace-tool uses them. Since we don't
actually support tracing there, we'll bypass that problem.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> scripts/qapi/commands.py | 101 +++++++++++++++++++++++++++++++++------
> scripts/qapi/main.py | 2 +-
> 2 files changed, 88 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 21001bbd6b..166bf5dcbc 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -53,7 +53,8 @@ def gen_command_decl(name: str,
> def gen_call(name: str,
> arg_type: Optional[QAPISchemaObjectType],
> boxed: bool,
> - ret_type: Optional[QAPISchemaType]) -> str:
> + ret_type: Optional[QAPISchemaType],
> + add_trace_events: bool) -> str:
> ret = ''
>
> argstr = ''
> @@ -71,21 +72,67 @@ def gen_call(name: str,
> if ret_type:
> lhs = 'retval = '
>
> - ret = mcgen('''
> + name = c_name(name)
> + upper = name.upper()
>
> - %(lhs)sqmp_%(c_name)s(%(args)s&err);
> - error_propagate(errp, err);
> -''',
> - c_name=c_name(name), args=argstr, lhs=lhs)
> - if ret_type:
> + if add_trace_events:
> ret += mcgen('''
> +
> + if (trace_event_get_state_backends(TRACE_QMP_ENTER_%(upper)s)) {
> + g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
> +
> + trace_qmp_enter_%(name)s(req_json->str);
> + }
> + ''',
> + upper=upper, name=name)
> +
> + ret += mcgen('''
> +
> + %(lhs)sqmp_%(name)s(%(args)s&err);
> +''',
> + name=name, args=argstr, lhs=lhs)
> +
> + ret += mcgen('''
> if (err) {
> +''')
> +
> + if add_trace_events:
> + ret += mcgen('''
> + trace_qmp_exit_%(name)s(error_get_pretty(err), false);
> +''',
> + name=name)
> +
> + ret += mcgen('''
> + error_propagate(errp, err);
> goto out;
> }
> +''')
Before the patch, we generate
error_propagate(errp, err);
and if there's any code between here and out:
if (err) {
goto out;
}
After the patch, we always generate
if (err) {
error_propagate(errp, err);
goto out;
}
But with trace generation enabled (next patch), it changes to
if (err) {
trace_qmp_exit_FOO(error_get_pretty(err), false);
error_propagate(errp, err);
goto out;
}
trace_qmp_exit_FOO("{}", true);
Okay.
> +
> + if ret_type:
> + ret += mcgen('''
>
> qmp_marshal_output_%(c_name)s(retval, ret, errp);
> ''',
> c_name=ret_type.c_name())
> +
> + if add_trace_events:
> + if ret_type:
> + ret += mcgen('''
> +
> + if (trace_event_get_state_backends(TRACE_QMP_EXIT_%(upper)s)) {
> + g_autoptr(GString) ret_json = qobject_to_json(*ret);
> +
> + trace_qmp_exit_%(name)s(ret_json->str, true);
> + }
> + ''',
> + upper=upper, name=name)
> + else:
> + ret += mcgen('''
> +
> + trace_qmp_exit_%(name)s("{}", true);
> + ''',
> + name=name)
> +
> return ret
>
>
> @@ -122,10 +169,19 @@ def gen_marshal_decl(name: str) -> str:
> proto=build_marshal_proto(name))
>
>
> +def gen_trace(name: str) -> str:
> + return mcgen('''
> +qmp_enter_%(name)s(const char *json) "%%s"
> +qmp_exit_%(name)s(const char *result, bool succeeded) "%%s %%d"
> +''',
> + name=c_name(name))
> +
> +
> def gen_marshal(name: str,
> arg_type: Optional[QAPISchemaObjectType],
> boxed: bool,
> - ret_type: Optional[QAPISchemaType]) -> str:
> + ret_type: Optional[QAPISchemaType],
> + add_trace_events: bool) -> str:
> have_args = boxed or (arg_type and not arg_type.is_empty())
> if have_args:
> assert arg_type is not None
> @@ -180,7 +236,7 @@ def gen_marshal(name: str,
> }
> ''')
>
> - ret += gen_call(name, arg_type, boxed, ret_type)
> + ret += gen_call(name, arg_type, boxed, ret_type, add_trace_events)
>
> ret += mcgen('''
>
> @@ -238,11 +294,13 @@ def gen_register_command(name: str,
>
>
> class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> - def __init__(self, prefix: str):
> + def __init__(self, prefix: str, add_trace_events: bool):
> super().__init__(
> prefix, 'qapi-commands',
> - ' * Schema-defined QAPI/QMP commands', None, __doc__)
> + ' * Schema-defined QAPI/QMP commands', None, __doc__,
> + add_trace_events=add_trace_events)
> self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> + self.add_trace_events = add_trace_events
>
> def _begin_user_module(self, name: str) -> None:
> self._visited_ret_types[self._genc] = set()
> @@ -261,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
>
> ''',
> commands=commands, visit=visit))
> +
> + if self.add_trace_events and commands != 'qapi-commands':
> + self._genc.add(mcgen('''
> +#include "trace/trace-qapi.h"
> +#include "qapi/qmp/qjson.h"
> +#include "trace/trace-%(nm)s_trace_events.h"
> +''',
> + nm=c_name(commands, protect=False)))
> + # We use c_name(commands, protect=False) to turn '-' into '_', to
> + # match .underscorify() in trace/meson.build
> +
> self._genh.add(mcgen('''
> #include "%(types)s.h"
>
> @@ -322,7 +391,10 @@ def visit_command(self,
> with ifcontext(ifcond, self._genh, self._genc):
> self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> self._genh.add(gen_marshal_decl(name))
> - self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> + self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
> + self.add_trace_events))
> + if self.add_trace_events:
> + self._gent.add(gen_trace(name))
> with self._temp_module('./init'):
> with ifcontext(ifcond, self._genh, self._genc):
> self._genc.add(gen_register_command(
> @@ -332,7 +404,8 @@ def visit_command(self,
>
> def gen_commands(schema: QAPISchema,
> output_dir: str,
> - prefix: str) -> None:
> - vis = QAPISchemaGenCommandVisitor(prefix)
> + prefix: str,
> + add_trace_events: bool) -> None:
> + vis = QAPISchemaGenCommandVisitor(prefix, add_trace_events)
> schema.visit(vis)
> vis.write(output_dir)
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index f2ea6e0ce4..2e61409f04 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -49,7 +49,7 @@ def generate(schema_file: str,
> schema = QAPISchema(schema_file)
> gen_types(schema, output_dir, prefix, builtins)
> gen_visit(schema, output_dir, prefix, builtins)
> - gen_commands(schema, output_dir, prefix)
> + gen_commands(schema, output_dir, prefix, False)
> gen_events(schema, output_dir, prefix)
> gen_introspect(schema, output_dir, prefix, unmask)
Missing:
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index a3b5473089..feafed79b5 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1690,8 +1690,8 @@ Example::
}
retval = qmp_my_command(arg.arg1, &err);
- error_propagate(errp, err);
if (err) {
+ error_propagate(errp, err);
goto out;
}
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg
2022-01-25 10:02 ` Markus Armbruster
@ 2022-01-25 10:20 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-25 10:20 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, stefanha, michael.roth, jsnow, hreitz, kwolf,
pbonzini
25.01.2022 13:02, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> We are going to generate trace events for qmp commands. We should
>> generate both trace_*() function calls and trace-events files listing
>> events for trace generator.
>>
>> Now implement that possibility in gen_commands() function.
>>
>> The functionality will be used in the following commit, and now comment
>> in _begin_user_module() about c_name() is a bit premature.
>
> Neglects to explain why it needs to be optional.
>
> Suggest
>
> qapi/commands: Optionally generate trace for QMP commands
>
> Add trace generation disabled. The next commit will enable it for
> qapi/, but not for qga/ and tests/. Making it work for the latter two
> would involve some Meson hackery to ensure we generate the
> trace-events files before trace-tool uses them. Since we don't
> actually support tracing there, we'll bypass that problem.
>
Thanks, sounds good.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> scripts/qapi/commands.py | 101 +++++++++++++++++++++++++++++++++------
>> scripts/qapi/main.py | 2 +-
>> 2 files changed, 88 insertions(+), 15 deletions(-)
>>
>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> index 21001bbd6b..166bf5dcbc 100644
>> --- a/scripts/qapi/commands.py
>> +++ b/scripts/qapi/commands.py
>> @@ -53,7 +53,8 @@ def gen_command_decl(name: str,
>> def gen_call(name: str,
>> arg_type: Optional[QAPISchemaObjectType],
>> boxed: bool,
>> - ret_type: Optional[QAPISchemaType]) -> str:
>> + ret_type: Optional[QAPISchemaType],
>> + add_trace_events: bool) -> str:
>> ret = ''
>>
>> argstr = ''
>> @@ -71,21 +72,67 @@ def gen_call(name: str,
>> if ret_type:
>> lhs = 'retval = '
>>
>> - ret = mcgen('''
>> + name = c_name(name)
>> + upper = name.upper()
>>
>> - %(lhs)sqmp_%(c_name)s(%(args)s&err);
>> - error_propagate(errp, err);
>> -''',
>> - c_name=c_name(name), args=argstr, lhs=lhs)
>> - if ret_type:
>> + if add_trace_events:
>> ret += mcgen('''
>> +
>> + if (trace_event_get_state_backends(TRACE_QMP_ENTER_%(upper)s)) {
>> + g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
>> +
>> + trace_qmp_enter_%(name)s(req_json->str);
>> + }
>> + ''',
>> + upper=upper, name=name)
>> +
>> + ret += mcgen('''
>> +
>> + %(lhs)sqmp_%(name)s(%(args)s&err);
>> +''',
>> + name=name, args=argstr, lhs=lhs)
>> +
>> + ret += mcgen('''
>> if (err) {
>> +''')
>> +
>> + if add_trace_events:
>> + ret += mcgen('''
>> + trace_qmp_exit_%(name)s(error_get_pretty(err), false);
>> +''',
>> + name=name)
>> +
>> + ret += mcgen('''
>> + error_propagate(errp, err);
>> goto out;
>> }
>> +''')
>
> Before the patch, we generate
>
> error_propagate(errp, err);
>
> and if there's any code between here and out:
>
> if (err) {
> goto out;
> }
>
> After the patch, we always generate
>
> if (err) {
> error_propagate(errp, err);
> goto out;
> }
>
> But with trace generation enabled (next patch), it changes to
>
> if (err) {
> trace_qmp_exit_FOO(error_get_pretty(err), false);
> error_propagate(errp, err);
> goto out;
> }
>
> trace_qmp_exit_FOO("{}", true);
>
> Okay.
>
>> +
>> + if ret_type:
>> + ret += mcgen('''
>>
>> qmp_marshal_output_%(c_name)s(retval, ret, errp);
>> ''',
>> c_name=ret_type.c_name())
>> +
>> + if add_trace_events:
>> + if ret_type:
>> + ret += mcgen('''
>> +
>> + if (trace_event_get_state_backends(TRACE_QMP_EXIT_%(upper)s)) {
>> + g_autoptr(GString) ret_json = qobject_to_json(*ret);
>> +
>> + trace_qmp_exit_%(name)s(ret_json->str, true);
>> + }
>> + ''',
>> + upper=upper, name=name)
>> + else:
>> + ret += mcgen('''
>> +
>> + trace_qmp_exit_%(name)s("{}", true);
>> + ''',
>> + name=name)
>> +
>> return ret
>>
>>
>> @@ -122,10 +169,19 @@ def gen_marshal_decl(name: str) -> str:
>> proto=build_marshal_proto(name))
>>
>>
>> +def gen_trace(name: str) -> str:
>> + return mcgen('''
>> +qmp_enter_%(name)s(const char *json) "%%s"
>> +qmp_exit_%(name)s(const char *result, bool succeeded) "%%s %%d"
>> +''',
>> + name=c_name(name))
>> +
>> +
>> def gen_marshal(name: str,
>> arg_type: Optional[QAPISchemaObjectType],
>> boxed: bool,
>> - ret_type: Optional[QAPISchemaType]) -> str:
>> + ret_type: Optional[QAPISchemaType],
>> + add_trace_events: bool) -> str:
>> have_args = boxed or (arg_type and not arg_type.is_empty())
>> if have_args:
>> assert arg_type is not None
>> @@ -180,7 +236,7 @@ def gen_marshal(name: str,
>> }
>> ''')
>>
>> - ret += gen_call(name, arg_type, boxed, ret_type)
>> + ret += gen_call(name, arg_type, boxed, ret_type, add_trace_events)
>>
>> ret += mcgen('''
>>
>> @@ -238,11 +294,13 @@ def gen_register_command(name: str,
>>
>>
>> class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>> - def __init__(self, prefix: str):
>> + def __init__(self, prefix: str, add_trace_events: bool):
>> super().__init__(
>> prefix, 'qapi-commands',
>> - ' * Schema-defined QAPI/QMP commands', None, __doc__)
>> + ' * Schema-defined QAPI/QMP commands', None, __doc__,
>> + add_trace_events=add_trace_events)
>> self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
>> + self.add_trace_events = add_trace_events
>>
>> def _begin_user_module(self, name: str) -> None:
>> self._visited_ret_types[self._genc] = set()
>> @@ -261,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
>>
>> ''',
>> commands=commands, visit=visit))
>> +
>> + if self.add_trace_events and commands != 'qapi-commands':
>> + self._genc.add(mcgen('''
>> +#include "trace/trace-qapi.h"
>> +#include "qapi/qmp/qjson.h"
>> +#include "trace/trace-%(nm)s_trace_events.h"
>> +''',
>> + nm=c_name(commands, protect=False)))
>> + # We use c_name(commands, protect=False) to turn '-' into '_', to
>> + # match .underscorify() in trace/meson.build
>> +
>> self._genh.add(mcgen('''
>> #include "%(types)s.h"
>>
>> @@ -322,7 +391,10 @@ def visit_command(self,
>> with ifcontext(ifcond, self._genh, self._genc):
>> self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
>> self._genh.add(gen_marshal_decl(name))
>> - self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
>> + self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
>> + self.add_trace_events))
>> + if self.add_trace_events:
>> + self._gent.add(gen_trace(name))
>> with self._temp_module('./init'):
>> with ifcontext(ifcond, self._genh, self._genc):
>> self._genc.add(gen_register_command(
>> @@ -332,7 +404,8 @@ def visit_command(self,
>>
>> def gen_commands(schema: QAPISchema,
>> output_dir: str,
>> - prefix: str) -> None:
>> - vis = QAPISchemaGenCommandVisitor(prefix)
>> + prefix: str,
>> + add_trace_events: bool) -> None:
>> + vis = QAPISchemaGenCommandVisitor(prefix, add_trace_events)
>> schema.visit(vis)
>> vis.write(output_dir)
>> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
>> index f2ea6e0ce4..2e61409f04 100644
>> --- a/scripts/qapi/main.py
>> +++ b/scripts/qapi/main.py
>> @@ -49,7 +49,7 @@ def generate(schema_file: str,
>> schema = QAPISchema(schema_file)
>> gen_types(schema, output_dir, prefix, builtins)
>> gen_visit(schema, output_dir, prefix, builtins)
>> - gen_commands(schema, output_dir, prefix)
>> + gen_commands(schema, output_dir, prefix, False)
>> gen_events(schema, output_dir, prefix)
>> gen_introspect(schema, output_dir, prefix, unmask)
>
> Missing:
>
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index a3b5473089..feafed79b5 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -1690,8 +1690,8 @@ Example::
> }
>
> retval = qmp_my_command(arg.arg1, &err);
> - error_propagate(errp, err);
> if (err) {
> + error_propagate(errp, err);
> goto out;
> }
>
>
Ah right. I fix doc in the next commit, but you are right, the behavior for disabled trace events is modified already in this commit.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/3] meson: generate trace events for qmp commands
2022-01-21 16:22 [PATCH v4 0/3] trace qmp commands Vladimir Sementsov-Ogievskiy
2022-01-21 16:22 ` [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module Vladimir Sementsov-Ogievskiy
2022-01-21 16:22 ` [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg Vladimir Sementsov-Ogievskiy
@ 2022-01-21 16:22 ` Vladimir Sementsov-Ogievskiy
2022-01-21 17:59 ` John Snow
2022-01-25 10:25 ` Markus Armbruster
2 siblings, 2 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-21 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, michael.roth, armbru, vsementsov, jsnow, hreitz, kwolf,
pbonzini
1. Add --no-trace-events to suppress trace events generation in some
cases, and make trace events be generated by default.
2. Add corresponding .trace-events files as outputs in qapi_files
custom target
3. Define global qapi_trace_events list of .trace-events file targets,
to fill in trace/qapi.build and to use in trace/meson.build
4. In trace/meson.build use the new array as an additional source of
.trace_events files to be processed
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
meson.build | 3 +++
qapi/meson.build | 7 +++++++
qga/meson.build | 11 ++++++++++-
scripts/qapi/main.py | 10 +++++++---
tests/meson.build | 11 ++++++++++-
trace/meson.build | 11 ++++++++---
7 files changed, 66 insertions(+), 10 deletions(-)
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index a3b5473089..a3430740bd 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1619,7 +1619,10 @@ Code generated for commands
These are the marshaling/dispatch functions for the commands defined
in the schema. The generated code provides qmp_marshal_COMMAND(), and
-declares qmp_COMMAND() that the user must implement.
+declares qmp_COMMAND() that the user must implement. The generated code
+contains trace events code. Corresponding .trace-events file with list
+of trace events is generated too, and should be parsed by trace generator
+later to generate trace event code, see `tracing <tracing.html>`.
The following files are generated:
@@ -1630,6 +1633,9 @@ The following files are generated:
``$(prefix)qapi-commands.h``
Function prototypes for the QMP commands specified in the schema
+ ``$(prefix)qapi-commands.trace-events``
+ Trace events file for trace generator, see `tracing <tracing.html>`.
+
``$(prefix)qapi-init-commands.h``
Command initialization prototype
@@ -1689,14 +1695,27 @@ Example::
goto out;
}
+ if (trace_event_get_state_backends(TRACE_QMP_ENTER_MY_COMMAND)) {
+ g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
+
+ trace_qmp_enter_my_command(req_json->str);
+ }
+
retval = qmp_my_command(arg.arg1, &err);
- error_propagate(errp, err);
if (err) {
+ trace_qmp_exit_my_command(error_get_pretty(err), false);
+ error_propagate(errp, err);
goto out;
}
qmp_marshal_output_UserDefOne(retval, ret, errp);
+ if (trace_event_get_state_backends(TRACE_QMP_EXIT_MY_COMMAND)) {
+ g_autoptr(GString) ret_json = qobject_to_json(*ret);
+
+ trace_qmp_exit_my_command(ret_json->str, true);
+ }
+
out:
visit_free(v);
v = qapi_dealloc_visitor_new();
diff --git a/meson.build b/meson.build
index 833fd6bc4c..e0cfafe8d9 100644
--- a/meson.build
+++ b/meson.build
@@ -41,6 +41,7 @@ qemu_icondir = get_option('datadir') / 'icons'
config_host_data = configuration_data()
genh = []
+qapi_trace_events = []
target_dirs = config_host['TARGET_DIRS'].split()
have_linux_user = false
@@ -2557,6 +2558,8 @@ if 'CONFIG_VHOST_USER' in config_host
vhost_user = libvhost_user.get_variable('vhost_user_dep')
endif
+# NOTE: the trace/ subdirectory needs the qapi_trace_events variable
+# that is filled in by qapi/.
subdir('qapi')
subdir('qobject')
subdir('stubs')
diff --git a/qapi/meson.build b/qapi/meson.build
index c0c49c15e4..656ef0e039 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -114,6 +114,7 @@ foreach module : qapi_all_modules
'qapi-events-@0@.h'.format(module),
'qapi-commands-@0@.c'.format(module),
'qapi-commands-@0@.h'.format(module),
+ 'qapi-commands-@0@.trace-events'.format(module),
]
endif
if module.endswith('-target')
@@ -137,6 +138,9 @@ foreach output : qapi_util_outputs
if output.endswith('.h')
genh += qapi_files[i]
endif
+ if output.endswith('.trace-events')
+ qapi_trace_events += qapi_files[i]
+ endif
util_ss.add(qapi_files[i])
i = i + 1
endforeach
@@ -145,6 +149,9 @@ foreach output : qapi_specific_outputs + qapi_nonmodule_outputs
if output.endswith('.h')
genh += qapi_files[i]
endif
+ if output.endswith('.trace-events')
+ qapi_trace_events += qapi_files[i]
+ endif
specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: qapi_files[i])
i = i + 1
endforeach
diff --git a/qga/meson.build b/qga/meson.build
index cfb1fbc085..1f2818a1b9 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -15,10 +15,19 @@ qga_qapi_outputs = [
'qga-qapi-visit.h',
]
+# We don't generate trace-events, just because it's not simple. For do it,
+# we also should add .trace-events file into qga_qapi_outputs, and than
+# add corresponding element of qga_qapi_files into qapi_trace_events
+# global list, which is processed than in trace/meson.build.
+# This means, that we'll have to move subdir('qga') above subdir('trace')
+# in root meson.build. But that in turn will break the dependency of
+# qga on qemuutil (which depends on trace_ss).
+# So, resolving these dependencies and drop --no-trace-events is a TODO.
qga_qapi_files = custom_target('QGA QAPI files',
output: qga_qapi_outputs,
input: 'qapi-schema.json',
- command: [ qapi_gen, '-o', 'qga', '-p', 'qga-', '@INPUT0@' ],
+ command: [ qapi_gen, '-o', 'qga', '-p', 'qga-', '@INPUT0@',
+ '--no-trace-events' ],
depend_files: qapi_gen_depends)
qga_ss = ss.source_set()
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 2e61409f04..d53e5b800c 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -32,7 +32,8 @@ def generate(schema_file: str,
output_dir: str,
prefix: str,
unmask: bool = False,
- builtins: bool = False) -> None:
+ builtins: bool = False,
+ add_trace_events: bool = False) -> None:
"""
Generate C code for the given schema into the target directory.
@@ -49,7 +50,7 @@ def generate(schema_file: str,
schema = QAPISchema(schema_file)
gen_types(schema, output_dir, prefix, builtins)
gen_visit(schema, output_dir, prefix, builtins)
- gen_commands(schema, output_dir, prefix, False)
+ gen_commands(schema, output_dir, prefix, add_trace_events)
gen_events(schema, output_dir, prefix)
gen_introspect(schema, output_dir, prefix, unmask)
@@ -74,6 +75,8 @@ def main() -> int:
parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
dest='unmask',
help="expose non-ABI names in introspection")
+ parser.add_argument('--no-trace-events', action='store_true',
+ help="suppress adding trace events to qmp marshals")
parser.add_argument('schema', action='store')
args = parser.parse_args()
@@ -88,7 +91,8 @@ def main() -> int:
output_dir=args.output_dir,
prefix=args.prefix,
unmask=args.unmask,
- builtins=args.builtins)
+ builtins=args.builtins,
+ add_trace_events=not args.no_trace_events)
except QAPIError as err:
print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
return 1
diff --git a/tests/meson.build b/tests/meson.build
index 3f3882748a..b4b95cc198 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -31,13 +31,22 @@ test_qapi_outputs = [
'test-qapi-visit.h',
]
+# We don't generate trace-events, just because it's not simple. For do it,
+# we also should add .trace-events file into test_qapi_outputs, and than
+# add corresponding element of test_qapi_files into qapi_trace_events
+# global list, which is processed than in trace/meson.build.
+# This means, that we'll have to move subdir('tests') above subdir('trace')
+# in root meson.build. But that in turn will break the dependency of
+# tests on qemuutil (which depends on trace_ss).
+# So, resolving these dependencies and drop --no-trace-events is a TODO.
test_qapi_files = custom_target('Test QAPI files',
output: test_qapi_outputs,
input: files('qapi-schema/qapi-schema-test.json',
'qapi-schema/include/sub-module.json',
'qapi-schema/sub-sub-module.json'),
command: [ qapi_gen, '-o', meson.current_build_dir(),
- '-b', '-p', 'test-', '@INPUT0@' ],
+ '-b', '-p', 'test-', '@INPUT0@',
+ '--no-trace-events' ],
depend_files: qapi_gen_depends)
# meson doesn't like generated output in other directories
diff --git a/trace/meson.build b/trace/meson.build
index 573dd699c6..c4794a1f2a 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -2,10 +2,15 @@
specific_ss.add(files('control-target.c'))
trace_events_files = []
-foreach dir : [ '.' ] + trace_events_subdirs
- trace_events_file = meson.project_source_root() / dir / 'trace-events'
+foreach item : [ '.' ] + trace_events_subdirs + qapi_trace_events
+ if item in qapi_trace_events
+ trace_events_file = item
+ group_name = item.full_path().split('/')[-1].underscorify()
+ else
+ trace_events_file = meson.project_source_root() / item / 'trace-events'
+ group_name = item == '.' ? 'root' : item.underscorify()
+ endif
trace_events_files += [ trace_events_file ]
- group_name = dir == '.' ? 'root' : dir.underscorify()
group = '--group=' + group_name
fmt = '@0@-' + group_name + '.@1@'
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/3] meson: generate trace events for qmp commands
2022-01-21 16:22 ` [PATCH v4 3/3] meson: generate trace events for qmp commands Vladimir Sementsov-Ogievskiy
@ 2022-01-21 17:59 ` John Snow
2022-01-25 10:25 ` Markus Armbruster
1 sibling, 0 replies; 15+ messages in thread
From: John Snow @ 2022-01-21 17:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, Michael Roth, qemu-devel, Markus Armbruster,
Hanna Reitz, Stefan Hajnoczi, Paolo Bonzini
On Fri, Jan 21, 2022 at 11:23 AM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 1. Add --no-trace-events to suppress trace events generation in some
> cases, and make trace events be generated by default.
> 2. Add corresponding .trace-events files as outputs in qapi_files
> custom target
> 3. Define global qapi_trace_events list of .trace-events file targets,
> to fill in trace/qapi.build and to use in trace/meson.build
> 4. In trace/meson.build use the new array as an additional source of
> .trace_events files to be processed
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
patch 2 and 3 come out clean under the kinda-sorta-linting I've been doing:
Acked-by: John Snow <jsnow@redhat.com>
(No time to do a bigger review, I'm drowning in backlog ... you and
Markus know where to find me if you need something urgently, though.)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] meson: generate trace events for qmp commands
2022-01-21 16:22 ` [PATCH v4 3/3] meson: generate trace events for qmp commands Vladimir Sementsov-Ogievskiy
2022-01-21 17:59 ` John Snow
@ 2022-01-25 10:25 ` Markus Armbruster
2022-01-25 11:03 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2022-01-25 10:25 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, michael.roth, qemu-devel, hreitz, stefanha, pbonzini,
jsnow
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 1. Add --no-trace-events to suppress trace events generation in some
> cases, and make trace events be generated by default.
> 2. Add corresponding .trace-events files as outputs in qapi_files
> custom target
> 3. Define global qapi_trace_events list of .trace-events file targets,
> to fill in trace/qapi.build and to use in trace/meson.build
> 4. In trace/meson.build use the new array as an additional source of
> .trace_events files to be processed
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
The doc update isn't mentioned in the commit message.
> meson.build | 3 +++
> qapi/meson.build | 7 +++++++
> qga/meson.build | 11 ++++++++++-
> scripts/qapi/main.py | 10 +++++++---
> tests/meson.build | 11 ++++++++++-
> trace/meson.build | 11 ++++++++---
> 7 files changed, 66 insertions(+), 10 deletions(-)
This commit consists of a small QAPI code generator change, build system
work to put it to use, and QAPI documentation update for the series'
feature.
I'd reshuffle as follows:
* Squash the main.py change into the previous commit.
* Split off the doc update into its own commit.
This way, build system experts can provide an R-by in good conscience
without reviewing the doc update, and vice versa.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] meson: generate trace events for qmp commands
2022-01-25 10:25 ` Markus Armbruster
@ 2022-01-25 11:03 ` Vladimir Sementsov-Ogievskiy
2022-01-25 11:13 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-25 11:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, stefanha, michael.roth, jsnow, hreitz, kwolf,
pbonzini
25.01.2022 13:25, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 1. Add --no-trace-events to suppress trace events generation in some
>> cases, and make trace events be generated by default.
>> 2. Add corresponding .trace-events files as outputs in qapi_files
>> custom target
>> 3. Define global qapi_trace_events list of .trace-events file targets,
>> to fill in trace/qapi.build and to use in trace/meson.build
>> 4. In trace/meson.build use the new array as an additional source of
>> .trace_events files to be processed
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
>
> The doc update isn't mentioned in the commit message.
>
>> meson.build | 3 +++
>> qapi/meson.build | 7 +++++++
>> qga/meson.build | 11 ++++++++++-
>> scripts/qapi/main.py | 10 +++++++---
>> tests/meson.build | 11 ++++++++++-
>> trace/meson.build | 11 ++++++++---
>> 7 files changed, 66 insertions(+), 10 deletions(-)
>
> This commit consists of a small QAPI code generator change, build system
> work to put it to use, and QAPI documentation update for the series'
> feature.
>
> I'd reshuffle as follows:
>
> * Squash the main.py change into the previous commit.
>
> * Split off the doc update into its own commit.
>
> This way, build system experts can provide an R-by in good conscience
> without reviewing the doc update, and vice versa.
>
But I think this way build will fail on previous commit. Or we should still keep trace-generation disabled in previous commit, and enable it only together with meson changes.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] meson: generate trace events for qmp commands
2022-01-25 11:03 ` Vladimir Sementsov-Ogievskiy
@ 2022-01-25 11:13 ` Vladimir Sementsov-Ogievskiy
2022-01-25 11:31 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-25 11:13 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, stefanha, michael.roth, jsnow, hreitz, kwolf,
pbonzini
25.01.2022 14:03, Vladimir Sementsov-Ogievskiy wrote:
> 25.01.2022 13:25, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 1. Add --no-trace-events to suppress trace events generation in some
>>> cases, and make trace events be generated by default.
>>> 2. Add corresponding .trace-events files as outputs in qapi_files
>>> custom target
>>> 3. Define global qapi_trace_events list of .trace-events file targets,
>>> to fill in trace/qapi.build and to use in trace/meson.build
>>> 4. In trace/meson.build use the new array as an additional source of
>>> .trace_events files to be processed
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>> docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
>>
>> The doc update isn't mentioned in the commit message.
>>
>>> meson.build | 3 +++
>>> qapi/meson.build | 7 +++++++
>>> qga/meson.build | 11 ++++++++++-
>>> scripts/qapi/main.py | 10 +++++++---
>>> tests/meson.build | 11 ++++++++++-
>>> trace/meson.build | 11 ++++++++---
>>> 7 files changed, 66 insertions(+), 10 deletions(-)
>>
>> This commit consists of a small QAPI code generator change, build system
>> work to put it to use, and QAPI documentation update for the series'
>> feature.
>>
>> I'd reshuffle as follows:
>>
>> * Squash the main.py change into the previous commit.
>>
>> * Split off the doc update into its own commit.
>>
>> This way, build system experts can provide an R-by in good conscience
>> without reviewing the doc update, and vice versa.
>>
>
> But I think this way build will fail on previous commit. Or we should still keep trace-generation disabled in previous commit, and enable it only together with meson changes.
>
May be keep positive option --gen-trace-events in previous commit, like in my previous version? This way meson-changing commit becomes self-sufficient. And then in additional commit change the default and drop --gen-trace-events option and add --no-trace-events instead.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] meson: generate trace events for qmp commands
2022-01-25 11:13 ` Vladimir Sementsov-Ogievskiy
@ 2022-01-25 11:31 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2022-01-25 11:31 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, michael.roth, qemu-devel, hreitz, stefanha, pbonzini,
jsnow
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 25.01.2022 14:03, Vladimir Sementsov-Ogievskiy wrote:
>> 25.01.2022 13:25, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> 1. Add --no-trace-events to suppress trace events generation in some
>>>> cases, and make trace events be generated by default.
>>>> 2. Add corresponding .trace-events files as outputs in qapi_files
>>>> custom target
>>>> 3. Define global qapi_trace_events list of .trace-events file targets,
>>>> to fill in trace/qapi.build and to use in trace/meson.build
>>>> 4. In trace/meson.build use the new array as an additional source of
>>>> .trace_events files to be processed
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>> docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
>>>
>>> The doc update isn't mentioned in the commit message.
>>>
>>>> meson.build | 3 +++
>>>> qapi/meson.build | 7 +++++++
>>>> qga/meson.build | 11 ++++++++++-
>>>> scripts/qapi/main.py | 10 +++++++---
>>>> tests/meson.build | 11 ++++++++++-
>>>> trace/meson.build | 11 ++++++++---
>>>> 7 files changed, 66 insertions(+), 10 deletions(-)
>>>
>>> This commit consists of a small QAPI code generator change, build system
>>> work to put it to use, and QAPI documentation update for the series'
>>> feature.
>>>
>>> I'd reshuffle as follows:
>>>
>>> * Squash the main.py change into the previous commit.
>>>
>>> * Split off the doc update into its own commit.
>>>
>>> This way, build system experts can provide an R-by in good conscience
>>> without reviewing the doc update, and vice versa.
>>>
>> But I think this way build will fail on previous commit. Or we
>> should still keep trace-generation disabled in previous commit, and
>> enable it only together with meson changes.
>>
>
> May be keep positive option --gen-trace-events in previous commit, like in my previous version? This way meson-changing commit becomes self-sufficient. And then in additional commit change the default and drop --gen-trace-events option and add --no-trace-events instead.
You choose. But I'd spell it --gen-trace.
^ permalink raw reply [flat|nested] 15+ messages in thread