From fc471c18d5d2ec713d5a019f9530398675494bc8 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:13 -0600 Subject: qapi: Consolidate QMP input visitor creation Rather than having two separate ways to create a QMP input visitor, where the safer approach has the more verbose name, it is better to consolidate things into a single function where the caller must explicitly choose whether to be strict or to ignore excess input. This patch is the strictly mechanical conversion; the next patch will then audit which uses can be made stricter. Signed-off-by: Eric Blake Message-Id: <1461879932-9020-6-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'scripts/qapi-commands.py') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index b570069..6261e44 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -115,7 +115,7 @@ def gen_marshal(name, arg_type, ret_type): if arg_type and arg_type.members: ret += mcgen(''' - QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); + QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); QapiDeallocVisitor *qdv; Visitor *v; %(c_name)s arg = {0}; -- cgit v1.1 From ed8415351941715f606749d6cdd64553b0de3f01 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:16 -0600 Subject: qapi-commands: Wrap argument visit in visit_start_struct The qmp-input visitor was allowing callers to play rather fast and loose: when visiting a QDict, you could grab members of the root dictionary without first pushing into the dict; among the culprit callers was the generated marshal code on the 'arguments' dictionary of a QMP command. But we are about to tighten the input visitor, at which point the generated marshal code MUST follow the same paradigms as everyone else, of pushing into the struct before grabbing its keys. Generated code grows as follows: |@@ -515,7 +641,12 @@ void qmp_marshal_blockdev_backup(QDict * | BlockdevBackup arg = {0}; | | v = qmp_input_get_visitor(qiv); |+ visit_start_struct(v, NULL, NULL, 0, &err); |+ if (err) { |+ goto out; |+ } | visit_type_BlockdevBackup_members(v, &arg, &err); |+ visit_end_struct(v, err ? NULL : &err); | if (err) { | goto out; | } |@@ -527,7 +715,9 @@ out: | qmp_input_visitor_cleanup(qiv); | qdv = qapi_dealloc_visitor_new(); | v = qapi_dealloc_get_visitor(qdv); |+ visit_start_struct(v, NULL, NULL, 0, NULL); | visit_type_BlockdevBackup_members(v, &arg, NULL); |+ visit_end_struct(v, NULL); | qapi_dealloc_visitor_cleanup(qdv); | } The use of 'err ? NULL : &err' is temporary; a later patch will clean that up when it splits visit_end_struct(). Prior to this patch, the fact that there was no final visit_end_struct() meant that even though we are using a strict input visit, the marshalling code was not detecting excess input at the top level (only in nested levels). Fortunately, we have code in monitor.c:qmp_check_client_args() that also checks for no excess arguments at the top level. But as the generated code is more compact than the manual check, a later patch will clean up monitor.c to drop the redundancy added here. Signed-off-by: Eric Blake Message-Id: <1461879932-9020-9-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-commands.py | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'scripts/qapi-commands.py') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 6261e44..04549fa 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -121,7 +121,12 @@ def gen_marshal(name, arg_type, ret_type): %(c_name)s arg = {0}; v = qmp_input_get_visitor(qiv); + visit_start_struct(v, NULL, NULL, 0, &err); + if (err) { + goto out; + } visit_type_%(c_name)s_members(v, &arg, &err); + visit_end_struct(v, err ? NULL : &err); if (err) { goto out; } @@ -150,7 +155,9 @@ out: qmp_input_visitor_cleanup(qiv); qdv = qapi_dealloc_visitor_new(); v = qapi_dealloc_get_visitor(qdv); + visit_start_struct(v, NULL, NULL, 0, NULL); visit_type_%(c_name)s_members(v, &arg, NULL); + visit_end_struct(v, NULL); qapi_dealloc_visitor_cleanup(qdv); ''', c_name=arg_type.c_name()) -- cgit v1.1 From 15c2f669e3fb2bc97f7b42d1871f595c0ac24af8 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:27 -0600 Subject: qapi: Split visit_end_struct() into pieces As mentioned in previous patches, we want to call visit_end_struct() functions unconditionally, so that visitors can release resources tied up since the matching visit_start_struct() without also having to worry about error priority if more than one error occurs. Even though error_propagate() can be safely used to ignore a second error during cleanup caused by a first error, it is simpler if the cleanup cannot set an error. So, split out the error checking portion (basically, input visitors checking for unvisited keys) into a new function visit_check_struct(), which can be safely skipped if any earlier errors are encountered, and leave the cleanup portion (which never fails, but must be called unconditionally if visit_start_struct() succeeded) in visit_end_struct(). Generated code in qapi-visit.c has diffs resembling: |@@ -59,10 +59,12 @@ void visit_type_ACPIOSTInfo(Visitor *v, | goto out_obj; | } | visit_type_ACPIOSTInfo_members(v, obj, &err); |- error_propagate(errp, err); |- err = NULL; |+ if (err) { |+ goto out_obj; |+ } |+ visit_check_struct(v, &err); | out_obj: |- visit_end_struct(v, &err); |+ visit_end_struct(v); | out: and in qapi-event.c: @@ -47,7 +47,10 @@ void qapi_event_send_acpi_device_ost(ACP | goto out; | } | visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, ¶m, &err); |- visit_end_struct(v, err ? NULL : &err); |+ if (!err) { |+ visit_check_struct(v, &err); |+ } |+ visit_end_struct(v); | if (err) { | goto out; Signed-off-by: Eric Blake Message-Id: <1461879932-9020-20-git-send-email-eblake@redhat.com> [Conflict with a doc fixup resolved] Signed-off-by: Markus Armbruster --- scripts/qapi-commands.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'scripts/qapi-commands.py') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 04549fa..8c6acb3 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -126,7 +126,10 @@ def gen_marshal(name, arg_type, ret_type): goto out; } visit_type_%(c_name)s_members(v, &arg, &err); - visit_end_struct(v, err ? NULL : &err); + if (!err) { + visit_check_struct(v, &err); + } + visit_end_struct(v); if (err) { goto out; } @@ -157,7 +160,7 @@ out: v = qapi_dealloc_get_visitor(qdv); visit_start_struct(v, NULL, NULL, 0, NULL); visit_type_%(c_name)s_members(v, &arg, NULL); - visit_end_struct(v, NULL); + visit_end_struct(v); qapi_dealloc_visitor_cleanup(qdv); ''', c_name=arg_type.c_name()) -- cgit v1.1