25.01.2022 12:07, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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
