18.01.2022 13:27, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
Add and option to generate trace events. We should generate both trace
events and trace-events files for further trace events code generation.
Can you explain why we want trace generation to be optional?
Because I failed make it work for tests and qga.. And seems there no good
reason for it: there now trace events for now neither in tests nor in qga.
I've now tried again.
It doesn't work, as I understand, the problem is qga subdir goes after trace
subdir, so when we generate trace headers, we didn't yet processed qga subdir.
And I can't just move qga above trace: qga depends on qemutil variable so it
should go after it. And if I put 'trace' subdir under qemuutil declaration it
doesn't work too (seems because qemuutil depends on trace_ss)..
So, supporting auto-generated trace points for qga qmp commands requires some
deeper refactoring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
scripts/qapi/commands.py | 91 ++++++++++++++++++++++++++++++++++------
scripts/qapi/main.py | 10 +++--
2 files changed, 85 insertions(+), 16 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 21001bbd6b..8cd1aa41ce 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,65 @@ 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));
Humor me: blank line between declarations and statements, please.
+ 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)
pycodestyle-3 gripes:
scripts/qapi/commands.py:92:17: E128 continuation line under-indented for
visual indent
+
+ 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);
Humor me: blank line between declarations and statements, please.
+ trace_qmp_exit_%(name)s(ret_json->str, true);
+ }
+ ''',
+ upper=upper, name=name)
scripts/qapi/commands.py:126:22: E128 continuation line under-indented for
visual indent
+ else:
+ ret += mcgen('''
+
+ trace_qmp_exit_%(name)s("{}", true);
+ ''',
+ name=name)
+
return ret
The generated code changes like this when trace generation is enabled
(next patch):
@@ -52,14 +55,25 @@ void qmp_marshal_query_acpi_ospm_status(
goto out;
}
+ if
(trace_event_get_state_backends(TRACE_QMP_ENTER_QUERY_ACPI_OSPM_STATUS)) {
+ g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
+ trace_qmp_enter_query_acpi_ospm_status(req_json->str);
+ }
+
retval = qmp_query_acpi_ospm_status(&err);
if (err) {
+ trace_qmp_exit_query_acpi_ospm_status(error_get_pretty(err),
false);
error_propagate(errp, err);
goto out;
}
qmp_marshal_output_ACPIOSTInfoList(retval, ret, errp);
+ if
(trace_event_get_state_backends(TRACE_QMP_EXIT_QUERY_ACPI_OSPM_STATUS)) {
+ g_autoptr(GString) ret_json = qobject_to_json(*ret);
+ trace_qmp_exit_query_acpi_ospm_status(ret_json->str, true);
+ }
+
out:
visit_free(v);
v = qapi_dealloc_visitor_new();
The trace_qmp_enter_query_acpi_ospm_status() and the second
trace_qmp_exit_query_acpi_ospm_status() is guarded by
trace_event_get_state_backends(), the first is not. Intentional?
Yes, I care to avoid json generation when trace event is disabled.
Have you considered something like
@@ -52,14 +55,25 @@ void qmp_marshal_query_acpi_ospm_status(
goto out;
}
+ if
(trace_event_get_state_backends(TRACE_QMP_ENTER_QUERY_ACPI_OSPM_STATUS)) {
+ g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
+ trace_qmp_enter_query_acpi_ospm_status(req_json->str);
+ }
+
retval = qmp_query_acpi_ospm_status(&err);
if (err) {
error_propagate(errp, err);
goto out;
}
qmp_marshal_output_ACPIOSTInfoList(retval, ret, errp);
out:
+ if
(trace_event_get_state_backends(TRACE_QMP_EXIT_QUERY_ACPI_OSPM_STATUS)) {
+ g_autoptr(GString) result_json
+ = qobject_to_json(err ? error_get_pretty(err) : *ret);
Hmm can qobject_to_json() work with simple string passed (returned by
error_get_pretty() ?
and it should not be automatically cleared..
And here err object is cleared (propagated to errp)...
But we can move error_propagate() call after trace_qmp_exit_ , and it shoud work
So, it should look like this:
if (trace_event_get_state...) {
g_autoptr(GString) result_json = NULL;
const char *result_str;
if (err) {
result_str = error_get_pretty(err);
} else {
result_json = qobject_to_json(*ret);
result_str = result_json->str;
}
trace_qmp_exit_query_acpi_ospm_status(result_str, !err);
}
error_propagate(errp, err);
IMHO, my original variant looks nicer.
+
+ trace_qmp_exit_query_acpi_ospm_status(ret_json->str, !err);
+ }
+
visit_free(v);
v = qapi_dealloc_visitor_new();
@@ -122,10 +167,17 @@ def gen_marshal_decl(name: str) -> str:
proto=build_marshal_proto(name))
+def gen_trace(name: str) -> str:
+ name = c_name(name)
+ return f"""\
+qmp_enter_{name}(const char *json) "%s"\n
+qmp_exit_{name}(const char *result, bool succeeded) "%s %d"\n"""
Why not mcgen()?
Hmm.. Here we don't need any indentation for sure. Do you think we still want
mcgen for consistancy and not use f-string?
The generated FOO.trace-events look like this:
$ cat bld-clang/qapi/qapi-commands-control.trace-events
qmp_enter_qmp_capabilities(const char *json) "%s"
qmp_exit_qmp_capabilities(const char *result, bool succeeded) "%s %d"
qmp_enter_query_version(const char *json) "%s"
qmp_exit_query_version(const char *result, bool succeeded) "%s %d"
qmp_enter_query_commands(const char *json) "%s"
qmp_exit_query_commands(const char *result, bool succeeded) "%s %d"
qmp_enter_quit(const char *json) "%s"
qmp_exit_quit(const char *result, bool succeeded) "%s %d"
Either drop the blank lines, or put them between the pairs instead of
within. I'd do the former.
We generate lots of empty FOO.trace-events. I guess that's okay.
+
scripts/qapi/commands.py:176:1: E302 expected 2 blank lines, found 1
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 +232,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 +290,12 @@ 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__)
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 +314,15 @@ def _begin_user_module(self, name: str) -> None:
''',
commands=commands, visit=visit))
+
+ if self.add_trace_events and c_name(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)))
Why c_name(commands), and not just commands?
Because generated files has underscores instead of '-'. Looking at code, I
think it's because underscorify() in trace/meson.build when we create
group_name variable.
+
self._genh.add(mcgen('''
#include "%(types)s.h"
@@ -322,7 +384,9 @@ 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))
+ 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 +396,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..7fab71401c 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)
+ 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('--add-trace-events', action='store_true',
+ help="add 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=args.add_trace_events)
except QAPIError as err:
print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
return 1
Missing: documentation for the tracing feature in
docs/devel/qapi-code-gen.rst. We can talk about the level of detail
last.
Also missing is the example update:
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;
}
--
Best regards,
Vladimir
