| Message ID | 20220121162234.2707906-2-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trace qmp commands | expand |
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
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.
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__()
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.
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
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(-)