From 92b09babc11b60458c28cfe37eaa314de50e6241 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 29 Jan 2016 06:48:47 -0700 Subject: qapi: Track all failures between visit_start/stop Inside the generated code between visit_start_struct() and visit_end_struct(), we were blindly setting the error into the caller's errp parameter. But a future patch to split visit_end_struct() will require that we take action based on whether an error has occurred, which requires us to track all actions through a local err. Rewrite the visits to be more in line with the other generated calls. Generated code changes look like: | visit_start_struct(v, (void **)obj, "Abort", name, sizeof(Abort), &err); |- if (!err) { |- if (*obj) { |- visit_type_Abort_fields(v, obj, errp); |- } |- visit_end_struct(v, &err); |+ if (err) { |+ goto out; | } |+ if (!*obj) { |+ goto out_obj; |+ } |+ visit_type_Abort_fields(v, obj, &err); |+ error_propagate(errp, err); |+ err = NULL; |+out_obj: |+ visit_end_struct(v, &err); |+out: | error_propagate(errp, err); | } Signed-off-by: Eric Blake Message-Id: <1454075341-13658-12-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-visit.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'scripts/qapi-visit.py') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index b93690b..ec16e36 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -2,7 +2,7 @@ # QAPI visitor generator # # Copyright IBM, Corp. 2011 -# Copyright (C) 2014-2015 Red Hat, Inc. +# Copyright (C) 2014-2016 Red Hat, Inc. # # Authors: # Anthony Liguori @@ -123,12 +123,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error Error *err = NULL; visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); - if (!err) { - if (*obj) { - visit_type_%(c_name)s_fields(v, obj, errp); - } - visit_end_struct(v, &err); + if (err) { + goto out; + } + if (!*obj) { + goto out_obj; } + visit_type_%(c_name)s_fields(v, obj, &err); + error_propagate(errp, err); + err = NULL; +out_obj: + visit_end_struct(v, &err); +out: error_propagate(errp, err); } ''', -- cgit v1.1 From 7c91aabd8964cfdf637f302c579c95401f21ce92 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 29 Jan 2016 06:48:48 -0700 Subject: qapi-visit: Kill unused visit_end_union() The generated code can call visit_end_union() without having called visit_start_union(). Example: if (!*obj) { goto out_obj; } visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err); if (err) { goto out_obj; // if we go from here... } if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { goto out_obj; } switch ((*obj)->arch) { [...] } out_obj: // ... then *obj is true, and ... error_propagate(errp, err); err = NULL; if (*obj) { // we end up here visit_end_union(v, !!(*obj)->u.data, &err); } error_propagate(errp, err); Harmless only because no visitor implements end_union(). Clean it up anyway, by deleting the function as useless. Messed up since we have visit_end_union (commit cee2ded). Signed-off-by: Markus Armbruster Message-Id: <1453902888-20457-3-git-send-email-armbru@redhat.com> [expand scope of patch to delete rather than repair] Signed-off-by: Eric Blake Message-Id: <1454075341-13658-13-git-send-email-eblake@redhat.com> --- scripts/qapi-visit.py | 5 ----- 1 file changed, 5 deletions(-) (limited to 'scripts/qapi-visit.py') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index ec16e36..f98bb5f 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -320,11 +320,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error out_obj: error_propagate(errp, err); err = NULL; - if (*obj) { - visit_end_union(v, !!(*obj)->u.data, &err); - } - error_propagate(errp, err); - err = NULL; visit_end_struct(v, &err); out: error_propagate(errp, err); -- cgit v1.1 From 395a233f7c089f23e3c0d43ce34c709dc5acd7de Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 29 Jan 2016 06:48:52 -0700 Subject: qapi: Don't cast Enum* to int* MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C compilers are allowed to represent enums as a smaller type than int, if all enum values fit in the smaller type. There are even compiler flags that force the use of this smaller representation, although using them changes the ABI of a binary. Therefore, our generated code for visit_type_ENUM() (for all qapi enums) was wrong for casting Enum* to int* when calling visit_type_enum(). It appears that no one has been using compiler ABI switches for qemu, because if they had, we are potentially dereferencing beyond bounds or even risking a SIGBUS on platforms where unaligned pointer dereferencing is fatal. But it is still better to avoid the practice entirely, and just use the correct types. This matches the fix for alternate qapi types, done earlier in commit 0426d53 "qapi: Simplify visiting of alternate types", with generated code changing as: | void visit_type_QType(Visitor *v, QType *obj, const char *name, Error **errp) | { |- visit_type_enum(v, (int *)obj, QType_lookup, "QType", name, errp); |+ int value = *obj; |+ visit_type_enum(v, &value, QType_lookup, "QType", name, errp); |+ *obj = value; | } Signed-off-by: Eric Blake Reviewed-by: Marc-André Lureau Message-Id: <1454075341-13658-17-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-visit.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'scripts/qapi-visit.py') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index f98bb5f..ba75667 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -178,12 +178,13 @@ out: def gen_visit_enum(name): - # FIXME cast from enum *obj to int * invalidly assumes enum is int return mcgen(''' void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp) { - visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp); + int value = *obj; + visit_type_enum(v, &value, %(c_name)s_lookup, "%(name)s", name, errp); + *obj = value; } ''', c_name=c_name(name), name=name) -- cgit v1.1 From 51e72bc1dd6ace6e91d675f41a1f09bd00ab8043 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 29 Jan 2016 06:48:54 -0700 Subject: qapi: Swap visit_* arguments for consistent 'name' placement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JSON uses "name":value, but many of our visitor interfaces were called with visit_type_FOO(v, &value, name, errp). This can be a bit confusing to have to mentally swap the parameter order to match JSON order. It's particularly bad for visit_start_struct(), where the 'name' parameter is smack in the middle of the otherwise-related group of 'obj, kind, size' parameters! It's time to do a global swap of the parameter ordering, so that the 'name' parameter is always immediately after the Visitor argument. Additional reason in favor of the swap: the existing include/qjson.h prefers listing 'name' first in json_prop_*(), and I have plans to unify that file with the qapi visitors; listing 'name' first in qapi will minimize churn to the (admittedly few) qjson.h clients. Later patches will then fix docs, object.h, visitor-impl.h, and those clients to match. Done by first patching scripts/qapi*.py by hand to make generated files do what I want, then by running the following Coccinelle script to affect the rest of the code base: $ spatch --sp-file script `git grep -l '\bvisit_' -- '**/*.[ch]'` I then had to apply some touchups (Coccinelle insisted on TAB indentation in visitor.h, and botched the signature of visit_type_enum() by rewriting 'const char *const strings[]' to the syntactically invalid 'const char*const[] strings'). The movement of parameters is sufficient to provoke compiler errors if any callers were missed. // Part 1: Swap declaration order @@ type TV, TErr, TObj, T1, T2; identifier OBJ, ARG1, ARG2; @@ void visit_start_struct -(TV v, TObj OBJ, T1 ARG1, const char *name, T2 ARG2, TErr errp) +(TV v, const char *name, TObj OBJ, T1 ARG1, T2 ARG2, TErr errp) { ... } @@ type bool, TV, T1; identifier ARG1; @@ bool visit_optional -(TV v, T1 ARG1, const char *name) +(TV v, const char *name, T1 ARG1) { ... } @@ type TV, TErr, TObj, T1; identifier OBJ, ARG1; @@ void visit_get_next_type -(TV v, TObj OBJ, T1 ARG1, const char *name, TErr errp) +(TV v, const char *name, TObj OBJ, T1 ARG1, TErr errp) { ... } @@ type TV, TErr, TObj, T1, T2; identifier OBJ, ARG1, ARG2; @@ void visit_type_enum -(TV v, TObj OBJ, T1 ARG1, T2 ARG2, const char *name, TErr errp) +(TV v, const char *name, TObj OBJ, T1 ARG1, T2 ARG2, TErr errp) { ... } @@ type TV, TErr, TObj; identifier OBJ; identifier VISIT_TYPE =~ "^visit_type_"; @@ void VISIT_TYPE -(TV v, TObj OBJ, const char *name, TErr errp) +(TV v, const char *name, TObj OBJ, TErr errp) { ... } // Part 2: swap caller order @@ expression V, NAME, OBJ, ARG1, ARG2, ERR; identifier VISIT_TYPE =~ "^visit_type_"; @@ ( -visit_start_struct(V, OBJ, ARG1, NAME, ARG2, ERR) +visit_start_struct(V, NAME, OBJ, ARG1, ARG2, ERR) | -visit_optional(V, ARG1, NAME) +visit_optional(V, NAME, ARG1) | -visit_get_next_type(V, OBJ, ARG1, NAME, ERR) +visit_get_next_type(V, NAME, OBJ, ARG1, ERR) | -visit_type_enum(V, OBJ, ARG1, ARG2, NAME, ERR) +visit_type_enum(V, NAME, OBJ, ARG1, ARG2, ERR) | -VISIT_TYPE(V, OBJ, NAME, ERR) +VISIT_TYPE(V, NAME, OBJ, ERR) ) Signed-off-by: Eric Blake Reviewed-by: Marc-André Lureau Message-Id: <1454075341-13658-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-visit.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) (limited to 'scripts/qapi-visit.py') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index ba75667..35505ac 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -29,7 +29,7 @@ def gen_visit_decl(name, scalar=False): if not scalar: c_type += '*' return mcgen(''' -void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error **errp); +void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **errp); ''', c_name=c_name(name), c_type=c_type) @@ -118,11 +118,11 @@ def gen_visit_struct(name, base, members): # call qapi_free_FOO() to avoid a memory leak of the partial FOO. ret += mcgen(''' -void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp) +void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { Error *err = NULL; - visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); + visit_start_struct(v, name, (void **)obj, "%(name)s", sizeof(%(c_name)s), &err); if (err) { goto out; } @@ -150,7 +150,7 @@ def gen_visit_list(name, element_type): # call qapi_free_FOOList() to avoid a memory leak of the partial FOOList. return mcgen(''' -void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp) +void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { Error *err = NULL; GenericList *i, **prev; @@ -164,7 +164,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error !err && (i = visit_next_list(v, prev, &err)) != NULL; prev = &i) { %(c_name)s *native_i = (%(c_name)s *)i; - visit_type_%(c_elt_type)s(v, &native_i->value, NULL, &err); + visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err); } error_propagate(errp, err); @@ -180,10 +180,10 @@ out: def gen_visit_enum(name): return mcgen(''' -void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp) +void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error **errp) { int value = *obj; - visit_type_enum(v, &value, %(c_name)s_lookup, "%(name)s", name, errp); + visit_type_enum(v, name, &value, %(c_name)s_lookup, "%(name)s", errp); *obj = value; } ''', @@ -198,7 +198,7 @@ def gen_visit_alternate(name, variants): ret = mcgen(''' -void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp) +void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { Error *err = NULL; @@ -206,7 +206,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error if (err) { goto out; } - visit_get_next_type(v, &(*obj)->type, %(promote_int)s, name, &err); + visit_get_next_type(v, name, &(*obj)->type, %(promote_int)s, &err); if (err) { goto out_obj; } @@ -217,7 +217,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error for var in variants.variants: ret += mcgen(''' case %(case)s: - visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err); + visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); break; ''', case=var.type.alternate_qtype(), @@ -255,11 +255,11 @@ def gen_visit_union(name, base, variants): ret += mcgen(''' -void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp) +void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { Error *err = NULL; - visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); + visit_start_struct(v, name, (void **)obj, "%(name)s", sizeof(%(c_name)s), &err); if (err) { goto out; } @@ -276,7 +276,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error c_name=base.c_name()) else: ret += mcgen(''' - visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err); + visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, &err); ''', c_type=variants.tag_member.type.c_name(), c_name=c_name(variants.tag_member.name), @@ -300,7 +300,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error var.name)) if simple_union_type: ret += mcgen(''' - visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, "data", &err); + visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err); ''', c_type=simple_union_type.c_name(), c_name=c_name(var.name)) -- cgit v1.1 From 337283dffbb5ad5860ed00408a5fd0665c21be07 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 29 Jan 2016 06:48:57 -0700 Subject: qapi: Drop unused 'kind' for struct/enum visit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit visit_start_struct() and visit_type_enum() had a 'kind' argument that was usually set to either the stringized version of the corresponding qapi type name, or to NULL (although some clients didn't even get that right). But nothing ever used the argument. It's even hard to argue that it would be useful in a debugger, as a stack backtrace also tells which type is being visited. Therefore, drop the 'kind' argument as dead. Signed-off-by: Eric Blake Reviewed-by: Marc-André Lureau Message-Id: <1454075341-13658-22-git-send-email-eblake@redhat.com> [Harmless rebase mistake cleaned up] Signed-off-by: Markus Armbruster --- scripts/qapi-visit.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'scripts/qapi-visit.py') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 35505ac..308000f 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -122,7 +122,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error { Error *err = NULL; - visit_start_struct(v, name, (void **)obj, "%(name)s", sizeof(%(c_name)s), &err); + visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err); if (err) { goto out; } @@ -138,7 +138,7 @@ out: error_propagate(errp, err); } ''', - name=name, c_name=c_name(name)) + c_name=c_name(name)) return ret @@ -183,11 +183,11 @@ def gen_visit_enum(name): void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error **errp) { int value = *obj; - visit_type_enum(v, name, &value, %(c_name)s_lookup, "%(name)s", errp); + visit_type_enum(v, name, &value, %(c_name)s_lookup, errp); *obj = value; } ''', - c_name=c_name(name), name=name) + c_name=c_name(name)) def gen_visit_alternate(name, variants): @@ -259,7 +259,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error { Error *err = NULL; - visit_start_struct(v, name, (void **)obj, "%(name)s", sizeof(%(c_name)s), &err); + visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err); if (err) { goto out; } @@ -267,7 +267,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error goto out_obj; } ''', - c_name=c_name(name), name=name) + c_name=c_name(name)) if base: ret += mcgen(''' -- cgit v1.1 From 08f9541dec51700abef0c37994213164ca4e4fc9 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 29 Jan 2016 06:48:59 -0700 Subject: qapi: Drop unused error argument for list and implicit struct No backend was setting an error when ending the visit of a list or implicit struct, or when moving to the next list node. Make the callers a bit easier to follow by making this a part of the contract, and removing the errp argument - callers can then unconditionally end an object as part of cleanup without having to think about whether a second error is dominated by a first, because there is no second error. A later patch will then tackle the larger task of splitting visit_end_struct(), which can indeed set an error. Signed-off-by: Eric Blake Message-Id: <1454075341-13658-24-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-visit.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) (limited to 'scripts/qapi-visit.py') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 308000f..0fdcebc 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -62,7 +62,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err); if (!err) { visit_type_%(c_type)s_fields(v, obj, errp); - visit_end_implicit_struct(v, &err); + visit_end_implicit_struct(v); } error_propagate(errp, err); } @@ -161,15 +161,13 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } for (prev = (GenericList **)obj; - !err && (i = visit_next_list(v, prev, &err)) != NULL; + !err && (i = visit_next_list(v, prev)) != NULL; prev = &i) { %(c_name)s *native_i = (%(c_name)s *)i; visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err); } - error_propagate(errp, err); - err = NULL; - visit_end_list(v, &err); + visit_end_list(v); out: error_propagate(errp, err); } @@ -230,9 +228,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error "%(name)s"); } out_obj: - error_propagate(errp, err); - err = NULL; - visit_end_implicit_struct(v, &err); + visit_end_implicit_struct(v); out: error_propagate(errp, err); } -- cgit v1.1