qemu-e2k/scripts/qapi/visit.py

354 lines
9.2 KiB
Python
Raw Normal View History

"""
QAPI visitor generator
Copyright IBM, Corp. 2011
Copyright (C) 2014-2018 Red Hat, Inc.
Authors:
Anthony Liguori <aliguori@us.ibm.com>
Michael Roth <mdroth@linux.vnet.ibm.com>
Markus Armbruster <armbru@redhat.com>
This work is licensed under the terms of the GNU GPL, version 2.
See the COPYING file in the top-level directory.
"""
from qapi.common import *
def gen_visit_decl(name, scalar=False):
c_type = c_name(name) + ' *'
if not scalar:
c_type += '*'
return mcgen('''
qapi: Swap visit_* arguments for consistent 'name' placement 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 <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-01-29 14:48:54 +01:00
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)
def gen_visit_members_decl(name):
return mcgen('''
void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
''',
c_name=c_name(name))
def gen_visit_object_members(name, base, members, variants):
ret = mcgen('''
void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
{
Error *err = NULL;
''',
c_name=c_name(name))
if base:
ret += mcgen('''
visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err);
if (err) {
goto out;
}
''',
qapi: Unbox base members Rather than storing a base class as a pointer to a box, just store the fields of that base class in the same order, so that a child struct can be directly cast to its parent. This gives less malloc overhead, less pointer dereferencing, and even less generated code. Compare to the earlier commit 1e6c1616a "qapi: Generate a nicer struct for flat unions" (although that patch had fewer places to change, as less of qemu was directly using qapi structs for flat unions). It also allows us to turn on automatic type-safe wrappers for upcasting to the base class of a struct. Changes to the generated code look like this in qapi-types.h: | struct SpiceChannel { |- SpiceBasicInfo *base; |+ /* Members inherited from SpiceBasicInfo: */ |+ char *host; |+ char *port; |+ NetworkAddressFamily family; |+ /* Own members: */ | int64_t connection_id; as well as additional upcast functions like qapi_SpiceChannel_base(). Meanwhile, changes to qapi-visit.c look like: | static void visit_type_SpiceChannel_fields(Visitor *v, SpiceChannel **obj, Error **errp) | { | Error *err = NULL; | |- visit_type_implicit_SpiceBasicInfo(v, &(*obj)->base, &err); |+ visit_type_SpiceBasicInfo_fields(v, (SpiceBasicInfo **)obj, &err); | if (err) { (the cast is necessary, since our upcast wrappers only deal with a single pointer, not pointer-to-pointer); plus the wholesale elimination of some now-unused visit_type_implicit_FOO() functions. Without boxing, the corner case of one empty struct having another empty struct as its base type now requires inserting a dummy member (previously, the 'Base *base' member sufficed). And now that we no longer consume a 'base' member in the generated C struct, we can delete the former negative struct-base-clash-base test. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1445898903-12082-11-git-send-email-eblake@redhat.com> [Commit message tweaked slightly] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-10-26 23:34:49 +01:00
c_type=base.c_name())
for memb in members:
if memb.optional:
ret += mcgen('''
if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
''',
name=memb.name, c_name=c_name(memb.name))
push_indent()
ret += mcgen('''
visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, &err);
if (err) {
goto out;
}
''',
c_type=memb.type.c_name(), name=memb.name,
c_name=c_name(memb.name))
if memb.optional:
pop_indent()
ret += mcgen('''
}
''')
qapi: Visit variants in visit_type_FOO_fields() We initially created the static visit_type_FOO_fields() helper function for reuse of code - we have cases where the initial setup for a visit has different allocation (depending on whether the fields represent a stand-alone type or are embedded as part of a larger type), but where the actual field visits are identical once a pointer is available. Up until the previous patch, visit_type_FOO_fields() was only used for structs (no variants), so it was covering every field for each type where it was emitted. Meanwhile, the code for visiting unions looks like: static visit_type_U_fields() { visit base; visit local_members; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit variants; visit_end_struct(); } which splits the fields of the union visit across two functions. Move the code to visit variants to live inside visit_type_U_fields(), while making it conditional on having variants so that all other instances of the helper function remain unchanged. This is also a step closer towards unifying struct and union visits, and towards allowing one union type to be the branch of another flat union. The resulting diff to the generated code is a bit hard to read, but it can be verified that it touches only union types, and that the end result is the following general structure: static visit_type_U_fields() { visit base; visit local_members; visit variants; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit_end_struct(); } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com> [gen_visit_struct_fields() parameter variants made mandatory] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:20 +01:00
if variants:
ret += mcgen('''
switch (obj->%(c_name)s) {
qapi: Visit variants in visit_type_FOO_fields() We initially created the static visit_type_FOO_fields() helper function for reuse of code - we have cases where the initial setup for a visit has different allocation (depending on whether the fields represent a stand-alone type or are embedded as part of a larger type), but where the actual field visits are identical once a pointer is available. Up until the previous patch, visit_type_FOO_fields() was only used for structs (no variants), so it was covering every field for each type where it was emitted. Meanwhile, the code for visiting unions looks like: static visit_type_U_fields() { visit base; visit local_members; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit variants; visit_end_struct(); } which splits the fields of the union visit across two functions. Move the code to visit variants to live inside visit_type_U_fields(), while making it conditional on having variants so that all other instances of the helper function remain unchanged. This is also a step closer towards unifying struct and union visits, and towards allowing one union type to be the branch of another flat union. The resulting diff to the generated code is a bit hard to read, but it can be verified that it touches only union types, and that the end result is the following general structure: static visit_type_U_fields() { visit base; visit local_members; visit variants; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit_end_struct(); } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com> [gen_visit_struct_fields() parameter variants made mandatory] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:20 +01:00
''',
c_name=c_name(variants.tag_member.name))
for var in variants.variants:
ret += mcgen('''
case %(case)s:
qapi: Don't special-case simple union wrappers Simple unions were carrying a special case that hid their 'data' QMP member from the resulting C struct, via the hack method QAPISchemaObjectTypeVariant.simple_union_type(). But by using the work we started by unboxing flat union and alternate branches, coupled with the ability to visit the members of an implicit type, we can now expose the simple union's implicit type in qapi-types.h: | struct q_obj_ImageInfoSpecificQCow2_wrapper { | ImageInfoSpecificQCow2 *data; | }; | | struct q_obj_ImageInfoSpecificVmdk_wrapper { | ImageInfoSpecificVmdk *data; | }; ... | struct ImageInfoSpecific { | ImageInfoSpecificKind type; | union { /* union tag is @type */ | void *data; |- ImageInfoSpecificQCow2 *qcow2; |- ImageInfoSpecificVmdk *vmdk; |+ q_obj_ImageInfoSpecificQCow2_wrapper qcow2; |+ q_obj_ImageInfoSpecificVmdk_wrapper vmdk; | } u; | }; Doing this removes asymmetry between QAPI's QMP side and its C side (both sides now expose 'data'), and means that the treatment of a simple union as sugar for a flat union is now equivalent in both languages (previously the two approaches used a different layer of dereferencing, where the simple union could be converted to a flat union with equivalent C layout but different {} on the wire, or to an equivalent QMP wire form but with different C representation). Using the implicit type also lets us get rid of the simple_union_type() hack. Of course, now all clients of simple unions have to adjust from using su->u.member to using su->u.member.data; while this touches a number of files in the tree, some earlier cleanup patches helped minimize the change to the initialization of a temporary variable rather than every single member access. The generated qapi-visit.c code is also affected by the layout change: |@@ -7393,10 +7393,10 @@ void visit_type_ImageInfoSpecific_member | } | switch (obj->type) { | case IMAGE_INFO_SPECIFIC_KIND_QCOW2: |- visit_type_ImageInfoSpecificQCow2(v, "data", &obj->u.qcow2, &err); |+ visit_type_q_obj_ImageInfoSpecificQCow2_wrapper_members(v, &obj->u.qcow2, &err); | break; | case IMAGE_INFO_SPECIFIC_KIND_VMDK: |- visit_type_ImageInfoSpecificVmdk(v, "data", &obj->u.vmdk, &err); |+ visit_type_q_obj_ImageInfoSpecificVmdk_wrapper_members(v, &obj->u.vmdk, &err); | break; | default: | abort(); Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1458254921-17042-13-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-03-17 23:48:37 +01:00
visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
break;
qapi: Visit variants in visit_type_FOO_fields() We initially created the static visit_type_FOO_fields() helper function for reuse of code - we have cases where the initial setup for a visit has different allocation (depending on whether the fields represent a stand-alone type or are embedded as part of a larger type), but where the actual field visits are identical once a pointer is available. Up until the previous patch, visit_type_FOO_fields() was only used for structs (no variants), so it was covering every field for each type where it was emitted. Meanwhile, the code for visiting unions looks like: static visit_type_U_fields() { visit base; visit local_members; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit variants; visit_end_struct(); } which splits the fields of the union visit across two functions. Move the code to visit variants to live inside visit_type_U_fields(), while making it conditional on having variants so that all other instances of the helper function remain unchanged. This is also a step closer towards unifying struct and union visits, and towards allowing one union type to be the branch of another flat union. The resulting diff to the generated code is a bit hard to read, but it can be verified that it touches only union types, and that the end result is the following general structure: static visit_type_U_fields() { visit base; visit local_members; visit variants; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit_end_struct(); } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com> [gen_visit_struct_fields() parameter variants made mandatory] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:20 +01:00
''',
case=c_enum_const(variants.tag_member.type.name,
var.name,
qapi: Don't special-case simple union wrappers Simple unions were carrying a special case that hid their 'data' QMP member from the resulting C struct, via the hack method QAPISchemaObjectTypeVariant.simple_union_type(). But by using the work we started by unboxing flat union and alternate branches, coupled with the ability to visit the members of an implicit type, we can now expose the simple union's implicit type in qapi-types.h: | struct q_obj_ImageInfoSpecificQCow2_wrapper { | ImageInfoSpecificQCow2 *data; | }; | | struct q_obj_ImageInfoSpecificVmdk_wrapper { | ImageInfoSpecificVmdk *data; | }; ... | struct ImageInfoSpecific { | ImageInfoSpecificKind type; | union { /* union tag is @type */ | void *data; |- ImageInfoSpecificQCow2 *qcow2; |- ImageInfoSpecificVmdk *vmdk; |+ q_obj_ImageInfoSpecificQCow2_wrapper qcow2; |+ q_obj_ImageInfoSpecificVmdk_wrapper vmdk; | } u; | }; Doing this removes asymmetry between QAPI's QMP side and its C side (both sides now expose 'data'), and means that the treatment of a simple union as sugar for a flat union is now equivalent in both languages (previously the two approaches used a different layer of dereferencing, where the simple union could be converted to a flat union with equivalent C layout but different {} on the wire, or to an equivalent QMP wire form but with different C representation). Using the implicit type also lets us get rid of the simple_union_type() hack. Of course, now all clients of simple unions have to adjust from using su->u.member to using su->u.member.data; while this touches a number of files in the tree, some earlier cleanup patches helped minimize the change to the initialization of a temporary variable rather than every single member access. The generated qapi-visit.c code is also affected by the layout change: |@@ -7393,10 +7393,10 @@ void visit_type_ImageInfoSpecific_member | } | switch (obj->type) { | case IMAGE_INFO_SPECIFIC_KIND_QCOW2: |- visit_type_ImageInfoSpecificQCow2(v, "data", &obj->u.qcow2, &err); |+ visit_type_q_obj_ImageInfoSpecificQCow2_wrapper_members(v, &obj->u.qcow2, &err); | break; | case IMAGE_INFO_SPECIFIC_KIND_VMDK: |- visit_type_ImageInfoSpecificVmdk(v, "data", &obj->u.vmdk, &err); |+ visit_type_q_obj_ImageInfoSpecificVmdk_wrapper_members(v, &obj->u.vmdk, &err); | break; | default: | abort(); Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1458254921-17042-13-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-03-17 23:48:37 +01:00
variants.tag_member.type.prefix),
c_type=var.type.c_name(), c_name=c_name(var.name))
qapi: Visit variants in visit_type_FOO_fields() We initially created the static visit_type_FOO_fields() helper function for reuse of code - we have cases where the initial setup for a visit has different allocation (depending on whether the fields represent a stand-alone type or are embedded as part of a larger type), but where the actual field visits are identical once a pointer is available. Up until the previous patch, visit_type_FOO_fields() was only used for structs (no variants), so it was covering every field for each type where it was emitted. Meanwhile, the code for visiting unions looks like: static visit_type_U_fields() { visit base; visit local_members; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit variants; visit_end_struct(); } which splits the fields of the union visit across two functions. Move the code to visit variants to live inside visit_type_U_fields(), while making it conditional on having variants so that all other instances of the helper function remain unchanged. This is also a step closer towards unifying struct and union visits, and towards allowing one union type to be the branch of another flat union. The resulting diff to the generated code is a bit hard to read, but it can be verified that it touches only union types, and that the end result is the following general structure: static visit_type_U_fields() { visit base; visit local_members; visit variants; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit_end_struct(); } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com> [gen_visit_struct_fields() parameter variants made mandatory] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:20 +01:00
ret += mcgen('''
default:
abort();
}
''')
# 'goto out' produced for base, for each member, and if variants were
# present
qapi: Visit variants in visit_type_FOO_fields() We initially created the static visit_type_FOO_fields() helper function for reuse of code - we have cases where the initial setup for a visit has different allocation (depending on whether the fields represent a stand-alone type or are embedded as part of a larger type), but where the actual field visits are identical once a pointer is available. Up until the previous patch, visit_type_FOO_fields() was only used for structs (no variants), so it was covering every field for each type where it was emitted. Meanwhile, the code for visiting unions looks like: static visit_type_U_fields() { visit base; visit local_members; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit variants; visit_end_struct(); } which splits the fields of the union visit across two functions. Move the code to visit variants to live inside visit_type_U_fields(), while making it conditional on having variants so that all other instances of the helper function remain unchanged. This is also a step closer towards unifying struct and union visits, and towards allowing one union type to be the branch of another flat union. The resulting diff to the generated code is a bit hard to read, but it can be verified that it touches only union types, and that the end result is the following general structure: static visit_type_U_fields() { visit base; visit local_members; visit variants; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit_end_struct(); } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com> [gen_visit_struct_fields() parameter variants made mandatory] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:20 +01:00
if base or members or variants:
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
ret += mcgen('''
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
out:
''')
ret += mcgen('''
error_propagate(errp, err);
}
''')
return ret
def gen_visit_list(name, element_type):
return mcgen('''
qapi: Swap visit_* arguments for consistent 'name' placement 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 <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-01-29 14:48:54 +01:00
void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
{
Error *err = NULL;
qapi: Simplify semantics of visit_next_list() The semantics of the list visit are somewhat baroque, with the following pseudocode when FooList is used: start() for (prev = head; cur = next(prev); prev = &cur) { visit(&cur->value) } Note that these semantics (advance before visit) requires that the first call to next() return the list head, while all other calls return the next element of the list; that is, every visitor implementation is required to track extra state to decide whether to return the input as-is, or to advance. It also requires an argument of 'GenericList **' to next(), solely because the first iteration might need to modify the caller's GenericList head, so that all other calls have to do a layer of dereferencing. Thankfully, we only have two uses of list visits in the entire code base: one in spapr_drc (which completely avoids visit_next_list(), feeding in integers from a different source than uint8List), and one in qapi-visit.py. That is, all other list visitors are generated in qapi-visit.c, and share the same paradigm based on a qapi FooList type, so we can refactor how lists are laid out with minimal churn among clients. We can greatly simplify things by hoisting the special case into the start() routine, and flipping the order in the loop to visit before advance: start(head) for (tail = *head; tail; tail = next(tail)) { visit(&tail->value) } With the simpler semantics, visitors have less state to track, the argument to next() is reduced to 'GenericList *', and it also becomes obvious whether an input visitor is allocating a FooList during visit_start_list() (rather than the old way of not knowing if an allocation happened until the first visit_next_list()). As a minor drawback, we now allocate in two functions instead of one, and have to pass the size to both functions (unless we were to tweak the input visitors to cache the size to start_list for reuse during next_list, but that defeats the goal of less visitor state). The signature of visit_start_list() is chosen to match visit_start_struct(), with the new parameters after 'name'. The spapr_drc case is a virtual visit, done by passing NULL for list, similarly to how NULL is passed to visit_start_struct() when a qapi type is not used in those visits. It was easy to provide these semantics for qmp-output and dealloc visitors, and a bit harder for qmp-input (several prerequisite patches refactored things to make this patch straightforward). But it turned out that the string and opts visitors munge enough other state during visit_next_list() to make it easier to just document and require a GenericList visit for now; an assertion will remind us to adjust things if we need the semantics in the future. Several pre-requisite cleanup patches made the reshuffling of the various visitors easier; particularly the qmp input visitor. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-24-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:31 +02:00
%(c_name)s *tail;
size_t size = sizeof(**obj);
qapi: Simplify semantics of visit_next_list() The semantics of the list visit are somewhat baroque, with the following pseudocode when FooList is used: start() for (prev = head; cur = next(prev); prev = &cur) { visit(&cur->value) } Note that these semantics (advance before visit) requires that the first call to next() return the list head, while all other calls return the next element of the list; that is, every visitor implementation is required to track extra state to decide whether to return the input as-is, or to advance. It also requires an argument of 'GenericList **' to next(), solely because the first iteration might need to modify the caller's GenericList head, so that all other calls have to do a layer of dereferencing. Thankfully, we only have two uses of list visits in the entire code base: one in spapr_drc (which completely avoids visit_next_list(), feeding in integers from a different source than uint8List), and one in qapi-visit.py. That is, all other list visitors are generated in qapi-visit.c, and share the same paradigm based on a qapi FooList type, so we can refactor how lists are laid out with minimal churn among clients. We can greatly simplify things by hoisting the special case into the start() routine, and flipping the order in the loop to visit before advance: start(head) for (tail = *head; tail; tail = next(tail)) { visit(&tail->value) } With the simpler semantics, visitors have less state to track, the argument to next() is reduced to 'GenericList *', and it also becomes obvious whether an input visitor is allocating a FooList during visit_start_list() (rather than the old way of not knowing if an allocation happened until the first visit_next_list()). As a minor drawback, we now allocate in two functions instead of one, and have to pass the size to both functions (unless we were to tweak the input visitors to cache the size to start_list for reuse during next_list, but that defeats the goal of less visitor state). The signature of visit_start_list() is chosen to match visit_start_struct(), with the new parameters after 'name'. The spapr_drc case is a virtual visit, done by passing NULL for list, similarly to how NULL is passed to visit_start_struct() when a qapi type is not used in those visits. It was easy to provide these semantics for qmp-output and dealloc visitors, and a bit harder for qmp-input (several prerequisite patches refactored things to make this patch straightforward). But it turned out that the string and opts visitors munge enough other state during visit_next_list() to make it easier to just document and require a GenericList visit for now; an assertion will remind us to adjust things if we need the semantics in the future. Several pre-requisite cleanup patches made the reshuffling of the various visitors easier; particularly the qmp input visitor. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-24-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:31 +02:00
visit_start_list(v, name, (GenericList **)obj, size, &err);
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
if (err) {
goto out;
}
qapi: Simplify semantics of visit_next_list() The semantics of the list visit are somewhat baroque, with the following pseudocode when FooList is used: start() for (prev = head; cur = next(prev); prev = &cur) { visit(&cur->value) } Note that these semantics (advance before visit) requires that the first call to next() return the list head, while all other calls return the next element of the list; that is, every visitor implementation is required to track extra state to decide whether to return the input as-is, or to advance. It also requires an argument of 'GenericList **' to next(), solely because the first iteration might need to modify the caller's GenericList head, so that all other calls have to do a layer of dereferencing. Thankfully, we only have two uses of list visits in the entire code base: one in spapr_drc (which completely avoids visit_next_list(), feeding in integers from a different source than uint8List), and one in qapi-visit.py. That is, all other list visitors are generated in qapi-visit.c, and share the same paradigm based on a qapi FooList type, so we can refactor how lists are laid out with minimal churn among clients. We can greatly simplify things by hoisting the special case into the start() routine, and flipping the order in the loop to visit before advance: start(head) for (tail = *head; tail; tail = next(tail)) { visit(&tail->value) } With the simpler semantics, visitors have less state to track, the argument to next() is reduced to 'GenericList *', and it also becomes obvious whether an input visitor is allocating a FooList during visit_start_list() (rather than the old way of not knowing if an allocation happened until the first visit_next_list()). As a minor drawback, we now allocate in two functions instead of one, and have to pass the size to both functions (unless we were to tweak the input visitors to cache the size to start_list for reuse during next_list, but that defeats the goal of less visitor state). The signature of visit_start_list() is chosen to match visit_start_struct(), with the new parameters after 'name'. The spapr_drc case is a virtual visit, done by passing NULL for list, similarly to how NULL is passed to visit_start_struct() when a qapi type is not used in those visits. It was easy to provide these semantics for qmp-output and dealloc visitors, and a bit harder for qmp-input (several prerequisite patches refactored things to make this patch straightforward). But it turned out that the string and opts visitors munge enough other state during visit_next_list() to make it easier to just document and require a GenericList visit for now; an assertion will remind us to adjust things if we need the semantics in the future. Several pre-requisite cleanup patches made the reshuffling of the various visitors easier; particularly the qmp input visitor. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-24-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:31 +02:00
for (tail = *obj; tail;
tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size)) {
visit_type_%(c_elt_type)s(v, NULL, &tail->value, &err);
if (err) {
break;
}
}
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
if (!err) {
visit_check_list(v, &err);
}
2016-06-09 18:48:34 +02:00
visit_end_list(v, (void **)obj);
qapi: Change visit_type_FOO() to no longer return partial objects Returning a partial object on error is an invitation for a careless caller to leak memory. We already fixed things in an earlier patch to guarantee NULL if visit_start fails ("qapi: Guarantee NULL obj on input visitor callback error"), but that does not help the case where visit_start succeeds but some other failure happens before visit_end, such that we leak a partially constructed object outside visit_type_FOO(). As no one outside the testsuite was actually relying on these semantics, it is cleaner to just document and guarantee that ALL pointer-based visit_type_FOO() functions always leave a safe value in *obj during an input visitor (either the new object on success, or NULL if an error is encountered), so callers can now unconditionally use qapi_free_FOO() to clean up regardless of whether an error occurred. The decision is done by adding visit_is_input(), then updating the generated code to check if additional cleanup is needed based on the type of visitor in use. Note that we still leave *obj unchanged after a scalar-based visit_type_FOO(); I did not feel like auditing all uses of visit_type_Enum() to see if the callers would tolerate a specific sentinel value (not to mention having to decide whether it would be better to use 0 or ENUM__MAX as that sentinel). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-25-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:32 +02:00
if (err && visit_is_input(v)) {
qapi_free_%(c_name)s(*obj);
*obj = NULL;
}
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
out:
error_propagate(errp, err);
}
''',
c_name=c_name(name), c_elt_type=element_type.c_name())
def gen_visit_enum(name):
return mcgen('''
qapi: Swap visit_* arguments for consistent 'name' placement 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 <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-01-29 14:48:54 +01:00
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, errp);
*obj = value;
}
''',
c_name=c_name(name))
def gen_visit_alternate(name, variants):
ret = mcgen('''
qapi: Swap visit_* arguments for consistent 'name' placement 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 <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-01-29 14:48:54 +01:00
void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
{
Error *err = NULL;
visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
&err);
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
if (err) {
goto out;
}
qapi: Fix crash on missing alternate member of QAPI struct If a QAPI struct has a mandatory alternate member which is not present on input, the input visitor reports an error for the missing alternate without setting the discriminator, but the cleanup code for the struct still tries to use the dealloc visitor to clean up the alternate. Commit dbf11922 changed visit_start_alternate to set *obj to NULL when an error occurs, where it was previously left untouched. Thus, before the patch, the dealloc visitor is blindly trying to cleanup whatever branch corresponds to (*obj)->type == 0 (that is, QTYPE_NONE, because *obj still pointed to zeroed memory), which selects the default branch of the switch and sets an error, but this second error is ignored by the way the dealloc visitor is used; but after the patch, the attempt to switch dereferences NULL. When cleaning up after a partial object parse, we specifically check for !*obj after visit_start_struct() (see gen_visit_object()); doing the same for alternates fixes the crash. Enhance the testsuite to give coverage for both missing struct and missing alternate members. Also add an abort - we expect visit_start_alternate() to either set an error or to set (*obj)->type to a valid QType that corresponds to actual user input, and QTYPE_NONE should never be reachable from valid input. Had the abort() been in place earlier, we might have noticed the dealloc visitor dereferencing bogus zeroed memory prior to when commit dbf11922 forced our hand by setting *obj to NULL and causing a fault. Test case: {'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}} The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat struct, which has a mandatory 'file':'BlockdevRef' in QAPI. Since 'file' is missing as a sibling of 'driver', this should report a graceful error rather than fault. After this patch, we are back to: {"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}} Generated code in qapi-visit.c changes as: |@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v, | if (err) { | goto out; | } |+ if (!*obj) { |+ goto out_obj; |+ } | switch ((*obj)->type) { | case QTYPE_QDICT: | visit_start_struct(v, name, NULL, 0, &err); |@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v, | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); | break; |+ case QTYPE_NONE: |+ abort(); | default: | error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", | "BlockdevRef"); | } |+out_obj: | visit_end_alternate(v); Reported by Kashyap Chamarthy <kchamart@redhat.com> CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1466012271-5204-1-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Tested-by: Kashyap Chamarthy <kchamart@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-06-15 19:37:51 +02:00
if (!*obj) {
goto out_obj;
}
qapi-visit: Convert to new qapi union layout We have two issues with our qapi union layout: 1) Even though the QMP wire format spells the tag 'type', the C code spells it 'kind', requiring some hacks in the generator. 2) The C struct uses an anonymous union, which places all tag values in the same namespace as all non-variant members. This leads to spurious collisions if a tag value matches a non-variant member's name. Make the conversion to the new layout for qapi-visit.py. Generated code changes look like: |@@ -4912,16 +4912,16 @@ void visit_type_MemoryDeviceInfo(Visitor | if (!*obj) { | goto out_obj; | } |- visit_type_MemoryDeviceInfoKind(v, &(*obj)->kind, "type", &err); |+ visit_type_MemoryDeviceInfoKind(v, &(*obj)->type, "type", &err); | if (err) { | goto out_obj; | } |- if (!visit_start_union(v, !!(*obj)->data, &err) || err) { |+ if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { | goto out_obj; | } |- switch ((*obj)->kind) { |+ switch ((*obj)->type) { | case MEMORY_DEVICE_INFO_KIND_DIMM: |- visit_type_PCDIMMDeviceInfo(v, &(*obj)->dimm, "data", &err); |+ visit_type_PCDIMMDeviceInfo(v, &(*obj)->u.dimm, "data", &err); | break; | default: | abort(); |@@ -4930,7 +4930,7 @@ out_obj: | error_propagate(errp, err); | err = NULL; | if (*obj) { |- visit_end_union(v, !!(*obj)->data, &err); |+ visit_end_union(v, !!(*obj)->u.data, &err); | } | error_propagate(errp, err); | err = NULL; Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1445898903-12082-14-git-send-email-eblake@redhat.com> [Commit message tweaked slightly] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-10-26 23:34:52 +01:00
switch ((*obj)->type) {
''',
c_name=c_name(name))
for var in variants.variants:
ret += mcgen('''
case %(case)s:
qapi: Don't box struct branch of alternate There's no reason to do two malloc's for an alternate type visiting a QAPI struct; let's just inline the struct directly as the C union branch of the struct. Surprisingly, no clients were actually using the struct member prior to this patch outside of the testsuite; an earlier patch in the series added some testsuite coverage to make the effect of this patch more obvious. In qapi.py, c_type() gains a new is_unboxed flag to control when we are emitting a C struct unboxed within the context of an outer struct (different from our other two modes of usage with no flags for normal local variable declarations, and with is_param for adding 'const' in a parameter list). I don't know if there is any more pythonic way of collapsing the two flags into a single parameter, as we never have a caller setting both flags at once. Ultimately, we want to also unbox branches for QAPI unions, but as that touches a lot more client code, it is better as separate patches. But since unions and alternates share gen_variants(), I had to hack in a way to test if we are visiting an alternate type for setting the is_unboxed flag: look for a non-object branch. This works because alternates have at least two branches, with at most one object branch, while unions have only object branches. The hack will go away in a later patch. The generated code difference to qapi-types.h is relatively small: | struct BlockdevRef { | QType type; | union { /* union tag is @type */ | void *data; |- BlockdevOptions *definition; |+ BlockdevOptions definition; | char *reference; | } u; | }; The corresponding spot in qapi-visit.c calls visit_type_FOO(), which first calls visit_start_struct() to allocate or deallocate the member and handle a layer of {} from the JSON stream, then visits the members. To peel off the indirection and the memory management that comes with it, we inline this call, then suppress allocation / deallocation by passing NULL to visit_start_struct(), and adjust the member visit: | switch ((*obj)->type) { | case QTYPE_QDICT: |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); |+ visit_start_struct(v, name, NULL, 0, &err); |+ if (err) { |+ break; |+ } |+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err); |+ error_propagate(errp, err); |+ err = NULL; |+ visit_end_struct(v, &err); | break; | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); The visit of non-object fields is unchanged. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-13-git-send-email-eblake@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:26 +01:00
''',
case=var.type.alternate_qtype())
if isinstance(var.type, QAPISchemaObjectType):
ret += mcgen('''
visit_start_struct(v, name, NULL, 0, &err);
if (err) {
break;
}
visit_type_%(c_type)s_members(v, &(*obj)->u.%(c_name)s, &err);
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, &param, &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 <eblake@redhat.com> Message-Id: <1461879932-9020-20-git-send-email-eblake@redhat.com> [Conflict with a doc fixup resolved] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:27 +02:00
if (!err) {
visit_check_struct(v, &err);
}
2016-06-09 18:48:34 +02:00
visit_end_struct(v, NULL);
qapi: Don't box struct branch of alternate There's no reason to do two malloc's for an alternate type visiting a QAPI struct; let's just inline the struct directly as the C union branch of the struct. Surprisingly, no clients were actually using the struct member prior to this patch outside of the testsuite; an earlier patch in the series added some testsuite coverage to make the effect of this patch more obvious. In qapi.py, c_type() gains a new is_unboxed flag to control when we are emitting a C struct unboxed within the context of an outer struct (different from our other two modes of usage with no flags for normal local variable declarations, and with is_param for adding 'const' in a parameter list). I don't know if there is any more pythonic way of collapsing the two flags into a single parameter, as we never have a caller setting both flags at once. Ultimately, we want to also unbox branches for QAPI unions, but as that touches a lot more client code, it is better as separate patches. But since unions and alternates share gen_variants(), I had to hack in a way to test if we are visiting an alternate type for setting the is_unboxed flag: look for a non-object branch. This works because alternates have at least two branches, with at most one object branch, while unions have only object branches. The hack will go away in a later patch. The generated code difference to qapi-types.h is relatively small: | struct BlockdevRef { | QType type; | union { /* union tag is @type */ | void *data; |- BlockdevOptions *definition; |+ BlockdevOptions definition; | char *reference; | } u; | }; The corresponding spot in qapi-visit.c calls visit_type_FOO(), which first calls visit_start_struct() to allocate or deallocate the member and handle a layer of {} from the JSON stream, then visits the members. To peel off the indirection and the memory management that comes with it, we inline this call, then suppress allocation / deallocation by passing NULL to visit_start_struct(), and adjust the member visit: | switch ((*obj)->type) { | case QTYPE_QDICT: |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); |+ visit_start_struct(v, name, NULL, 0, &err); |+ if (err) { |+ break; |+ } |+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err); |+ error_propagate(errp, err); |+ err = NULL; |+ visit_end_struct(v, &err); | break; | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); The visit of non-object fields is unchanged. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-13-git-send-email-eblake@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:26 +01:00
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
else:
ret += mcgen('''
qapi: Swap visit_* arguments for consistent 'name' placement 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 <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-01-29 14:48:54 +01:00
visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
''',
qapi: Don't box struct branch of alternate There's no reason to do two malloc's for an alternate type visiting a QAPI struct; let's just inline the struct directly as the C union branch of the struct. Surprisingly, no clients were actually using the struct member prior to this patch outside of the testsuite; an earlier patch in the series added some testsuite coverage to make the effect of this patch more obvious. In qapi.py, c_type() gains a new is_unboxed flag to control when we are emitting a C struct unboxed within the context of an outer struct (different from our other two modes of usage with no flags for normal local variable declarations, and with is_param for adding 'const' in a parameter list). I don't know if there is any more pythonic way of collapsing the two flags into a single parameter, as we never have a caller setting both flags at once. Ultimately, we want to also unbox branches for QAPI unions, but as that touches a lot more client code, it is better as separate patches. But since unions and alternates share gen_variants(), I had to hack in a way to test if we are visiting an alternate type for setting the is_unboxed flag: look for a non-object branch. This works because alternates have at least two branches, with at most one object branch, while unions have only object branches. The hack will go away in a later patch. The generated code difference to qapi-types.h is relatively small: | struct BlockdevRef { | QType type; | union { /* union tag is @type */ | void *data; |- BlockdevOptions *definition; |+ BlockdevOptions definition; | char *reference; | } u; | }; The corresponding spot in qapi-visit.c calls visit_type_FOO(), which first calls visit_start_struct() to allocate or deallocate the member and handle a layer of {} from the JSON stream, then visits the members. To peel off the indirection and the memory management that comes with it, we inline this call, then suppress allocation / deallocation by passing NULL to visit_start_struct(), and adjust the member visit: | switch ((*obj)->type) { | case QTYPE_QDICT: |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); |+ visit_start_struct(v, name, NULL, 0, &err); |+ if (err) { |+ break; |+ } |+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err); |+ error_propagate(errp, err); |+ err = NULL; |+ visit_end_struct(v, &err); | break; | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); The visit of non-object fields is unchanged. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-13-git-send-email-eblake@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:26 +01:00
c_type=var.type.c_name(),
c_name=c_name(var.name))
ret += mcgen('''
break;
''')
ret += mcgen('''
qapi: Fix crash on missing alternate member of QAPI struct If a QAPI struct has a mandatory alternate member which is not present on input, the input visitor reports an error for the missing alternate without setting the discriminator, but the cleanup code for the struct still tries to use the dealloc visitor to clean up the alternate. Commit dbf11922 changed visit_start_alternate to set *obj to NULL when an error occurs, where it was previously left untouched. Thus, before the patch, the dealloc visitor is blindly trying to cleanup whatever branch corresponds to (*obj)->type == 0 (that is, QTYPE_NONE, because *obj still pointed to zeroed memory), which selects the default branch of the switch and sets an error, but this second error is ignored by the way the dealloc visitor is used; but after the patch, the attempt to switch dereferences NULL. When cleaning up after a partial object parse, we specifically check for !*obj after visit_start_struct() (see gen_visit_object()); doing the same for alternates fixes the crash. Enhance the testsuite to give coverage for both missing struct and missing alternate members. Also add an abort - we expect visit_start_alternate() to either set an error or to set (*obj)->type to a valid QType that corresponds to actual user input, and QTYPE_NONE should never be reachable from valid input. Had the abort() been in place earlier, we might have noticed the dealloc visitor dereferencing bogus zeroed memory prior to when commit dbf11922 forced our hand by setting *obj to NULL and causing a fault. Test case: {'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}} The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat struct, which has a mandatory 'file':'BlockdevRef' in QAPI. Since 'file' is missing as a sibling of 'driver', this should report a graceful error rather than fault. After this patch, we are back to: {"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}} Generated code in qapi-visit.c changes as: |@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v, | if (err) { | goto out; | } |+ if (!*obj) { |+ goto out_obj; |+ } | switch ((*obj)->type) { | case QTYPE_QDICT: | visit_start_struct(v, name, NULL, 0, &err); |@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v, | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); | break; |+ case QTYPE_NONE: |+ abort(); | default: | error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", | "BlockdevRef"); | } |+out_obj: | visit_end_alternate(v); Reported by Kashyap Chamarthy <kchamart@redhat.com> CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1466012271-5204-1-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Tested-by: Kashyap Chamarthy <kchamart@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-06-15 19:37:51 +02:00
case QTYPE_NONE:
abort();
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
default:
qapi: Simplify visiting of alternate types Previously, working with alternates required two lookup arrays and some indirection: for type Foo, we created Foo_qtypes[] which maps each qtype to a value of the generated FooKind enum, then look up that value in FooKind_lookup[] like we do for other union types. This has a couple of subtle bugs. First, the generator was creating a call with a parameter '(int *) &(*obj)->type' where type is an enum type; this is unsafe if the compiler chooses to store the enum type in a different size than int, where assigning through the wrong size pointer can corrupt data or cause a SIGBUS. Related bug, not not fixed in this patch: qapi-visit.py's gen_visit_enum() generates a cast of its enum * argument to int *. Marked FIXME. Second, since the values of the FooKind enum start at zero, all entries of the Foo_qtypes[] array that were not explicitly initialized will map to the same branch of the union as the first member of the alternate, rather than triggering a desired failure in visit_get_next_type(). Fortunately, the bug seldom bites; the very next thing the input visitor does is try to parse the incoming JSON with the wrong parser, which normally fails; the output visitor is not used with a C struct in that state, and the dealloc visitor has nothing to clean up (so there is no leak). However, the second bug IS observable in one case: parsing an integer causes unusual behavior in an alternate that contains at least a 'number' member but no 'int' member, because the 'number' parser accepts QTYPE_QINT in addition to the expected QTYPE_QFLOAT (that is, since 'int' is not a member, the type QTYPE_QINT accidentally maps to FooKind 0; if this enum value is the 'number' branch the integer parses successfully, but if the 'number' branch is not first, some other branch tries to parse the integer and rejects it). A later patch will worry about fixing alternates to always parse all inputs that a non-alternate 'number' would accept, for now this is still marked FIXME in the updated test-qmp-input-visitor.c, to merely point out that new undesired behavior of 'ans' matches the existing undesired behavior of 'asn'. This patch fixes the default-initialization bug by deleting the indirection, and modifying get_next_type() to directly assign a QTypeCode parameter. This in turn fixes the type-casting bug, as we are no longer casting a pointer to enum to a questionable size. There is no longer a need to generate an implicit FooKind enum associated with the alternate type (since the QMP wire format never uses the stringized counterparts of the C union member names). Since the updated visit_get_next_type() does not know which qtypes are expected, the generated visitor is modified to generate an error statement if an unexpected type is encountered. Callers now have to know the QTYPE_* mapping when looking at the discriminator; but so far, only the testsuite was even using the C struct of an alternate types. I considered the possibility of keeping the internal enum FooKind, but initialized differently than most generated arrays, as in: typedef enum FooKind { FOO_KIND_A = QTYPE_QDICT, FOO_KIND_B = QTYPE_QINT, } FooKind; to create nicer aliases for knowing when to use foo->a or foo->b when inspecting foo->type; but it turned out to add too much complexity, especially without a client. There is a user-visible side effect to this change, but I consider it to be an improvement. Previously, the invalid QMP command: {"execute":"blockdev-add", "arguments":{"options": {"driver":"raw", "id":"a", "file":true}}} failed with: {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'file', expected: QDict"}} (visit_get_next_type() succeeded, and the error comes from the visit_type_BlockdevOptions() expecting {}; there is no mention of the fact that a string would also work). Now it fails with: {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}} (the error when the next type doesn't match any expected types for the overall alternate). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-02 06:20:48 +01:00
error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"%(name)s");
}
qapi: Fix crash on missing alternate member of QAPI struct If a QAPI struct has a mandatory alternate member which is not present on input, the input visitor reports an error for the missing alternate without setting the discriminator, but the cleanup code for the struct still tries to use the dealloc visitor to clean up the alternate. Commit dbf11922 changed visit_start_alternate to set *obj to NULL when an error occurs, where it was previously left untouched. Thus, before the patch, the dealloc visitor is blindly trying to cleanup whatever branch corresponds to (*obj)->type == 0 (that is, QTYPE_NONE, because *obj still pointed to zeroed memory), which selects the default branch of the switch and sets an error, but this second error is ignored by the way the dealloc visitor is used; but after the patch, the attempt to switch dereferences NULL. When cleaning up after a partial object parse, we specifically check for !*obj after visit_start_struct() (see gen_visit_object()); doing the same for alternates fixes the crash. Enhance the testsuite to give coverage for both missing struct and missing alternate members. Also add an abort - we expect visit_start_alternate() to either set an error or to set (*obj)->type to a valid QType that corresponds to actual user input, and QTYPE_NONE should never be reachable from valid input. Had the abort() been in place earlier, we might have noticed the dealloc visitor dereferencing bogus zeroed memory prior to when commit dbf11922 forced our hand by setting *obj to NULL and causing a fault. Test case: {'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}} The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat struct, which has a mandatory 'file':'BlockdevRef' in QAPI. Since 'file' is missing as a sibling of 'driver', this should report a graceful error rather than fault. After this patch, we are back to: {"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}} Generated code in qapi-visit.c changes as: |@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v, | if (err) { | goto out; | } |+ if (!*obj) { |+ goto out_obj; |+ } | switch ((*obj)->type) { | case QTYPE_QDICT: | visit_start_struct(v, name, NULL, 0, &err); |@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v, | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); | break; |+ case QTYPE_NONE: |+ abort(); | default: | error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", | "BlockdevRef"); | } |+out_obj: | visit_end_alternate(v); Reported by Kashyap Chamarthy <kchamart@redhat.com> CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1466012271-5204-1-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Tested-by: Kashyap Chamarthy <kchamart@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-06-15 19:37:51 +02:00
out_obj:
2016-06-09 18:48:34 +02:00
visit_end_alternate(v, (void **)obj);
qapi: Change visit_type_FOO() to no longer return partial objects Returning a partial object on error is an invitation for a careless caller to leak memory. We already fixed things in an earlier patch to guarantee NULL if visit_start fails ("qapi: Guarantee NULL obj on input visitor callback error"), but that does not help the case where visit_start succeeds but some other failure happens before visit_end, such that we leak a partially constructed object outside visit_type_FOO(). As no one outside the testsuite was actually relying on these semantics, it is cleaner to just document and guarantee that ALL pointer-based visit_type_FOO() functions always leave a safe value in *obj during an input visitor (either the new object on success, or NULL if an error is encountered), so callers can now unconditionally use qapi_free_FOO() to clean up regardless of whether an error occurred. The decision is done by adding visit_is_input(), then updating the generated code to check if additional cleanup is needed based on the type of visitor in use. Note that we still leave *obj unchanged after a scalar-based visit_type_FOO(); I did not feel like auditing all uses of visit_type_Enum() to see if the callers would tolerate a specific sentinel value (not to mention having to decide whether it would be better to use 0 or ENUM__MAX as that sentinel). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-25-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:32 +02:00
if (err && visit_is_input(v)) {
qapi_free_%(c_name)s(*obj);
*obj = NULL;
}
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
out:
error_propagate(errp, err);
}
qapi: Simplify visiting of alternate types Previously, working with alternates required two lookup arrays and some indirection: for type Foo, we created Foo_qtypes[] which maps each qtype to a value of the generated FooKind enum, then look up that value in FooKind_lookup[] like we do for other union types. This has a couple of subtle bugs. First, the generator was creating a call with a parameter '(int *) &(*obj)->type' where type is an enum type; this is unsafe if the compiler chooses to store the enum type in a different size than int, where assigning through the wrong size pointer can corrupt data or cause a SIGBUS. Related bug, not not fixed in this patch: qapi-visit.py's gen_visit_enum() generates a cast of its enum * argument to int *. Marked FIXME. Second, since the values of the FooKind enum start at zero, all entries of the Foo_qtypes[] array that were not explicitly initialized will map to the same branch of the union as the first member of the alternate, rather than triggering a desired failure in visit_get_next_type(). Fortunately, the bug seldom bites; the very next thing the input visitor does is try to parse the incoming JSON with the wrong parser, which normally fails; the output visitor is not used with a C struct in that state, and the dealloc visitor has nothing to clean up (so there is no leak). However, the second bug IS observable in one case: parsing an integer causes unusual behavior in an alternate that contains at least a 'number' member but no 'int' member, because the 'number' parser accepts QTYPE_QINT in addition to the expected QTYPE_QFLOAT (that is, since 'int' is not a member, the type QTYPE_QINT accidentally maps to FooKind 0; if this enum value is the 'number' branch the integer parses successfully, but if the 'number' branch is not first, some other branch tries to parse the integer and rejects it). A later patch will worry about fixing alternates to always parse all inputs that a non-alternate 'number' would accept, for now this is still marked FIXME in the updated test-qmp-input-visitor.c, to merely point out that new undesired behavior of 'ans' matches the existing undesired behavior of 'asn'. This patch fixes the default-initialization bug by deleting the indirection, and modifying get_next_type() to directly assign a QTypeCode parameter. This in turn fixes the type-casting bug, as we are no longer casting a pointer to enum to a questionable size. There is no longer a need to generate an implicit FooKind enum associated with the alternate type (since the QMP wire format never uses the stringized counterparts of the C union member names). Since the updated visit_get_next_type() does not know which qtypes are expected, the generated visitor is modified to generate an error statement if an unexpected type is encountered. Callers now have to know the QTYPE_* mapping when looking at the discriminator; but so far, only the testsuite was even using the C struct of an alternate types. I considered the possibility of keeping the internal enum FooKind, but initialized differently than most generated arrays, as in: typedef enum FooKind { FOO_KIND_A = QTYPE_QDICT, FOO_KIND_B = QTYPE_QINT, } FooKind; to create nicer aliases for knowing when to use foo->a or foo->b when inspecting foo->type; but it turned out to add too much complexity, especially without a client. There is a user-visible side effect to this change, but I consider it to be an improvement. Previously, the invalid QMP command: {"execute":"blockdev-add", "arguments":{"options": {"driver":"raw", "id":"a", "file":true}}} failed with: {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'file', expected: QDict"}} (visit_get_next_type() succeeded, and the error comes from the visit_type_BlockdevOptions() expecting {}; there is no mention of the fact that a string would also work). Now it fails with: {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}} (the error when the next type doesn't match any expected types for the overall alternate). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-02 06:20:48 +01:00
''',
qapi: Change visit_type_FOO() to no longer return partial objects Returning a partial object on error is an invitation for a careless caller to leak memory. We already fixed things in an earlier patch to guarantee NULL if visit_start fails ("qapi: Guarantee NULL obj on input visitor callback error"), but that does not help the case where visit_start succeeds but some other failure happens before visit_end, such that we leak a partially constructed object outside visit_type_FOO(). As no one outside the testsuite was actually relying on these semantics, it is cleaner to just document and guarantee that ALL pointer-based visit_type_FOO() functions always leave a safe value in *obj during an input visitor (either the new object on success, or NULL if an error is encountered), so callers can now unconditionally use qapi_free_FOO() to clean up regardless of whether an error occurred. The decision is done by adding visit_is_input(), then updating the generated code to check if additional cleanup is needed based on the type of visitor in use. Note that we still leave *obj unchanged after a scalar-based visit_type_FOO(); I did not feel like auditing all uses of visit_type_Enum() to see if the callers would tolerate a specific sentinel value (not to mention having to decide whether it would be better to use 0 or ENUM__MAX as that sentinel). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-25-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:32 +02:00
name=name, c_name=c_name(name))
return ret
def gen_visit_object(name, base, members, variants):
qapi: Emit implicit structs in generated C We already have several places that want to visit all the members of an implicit object within a larger context (simple union variant, event with anonymous data, command with anonymous arguments struct); and will be adding another one soon (the ability to declare an anonymous base for a flat union). Having a C struct declared for these implicit types, along with a visit_type_FOO_members() helper function, will make for fewer special cases in our generator. We do not, however, need qapi_free_FOO() or visit_type_FOO() functions for implicit types, because they should not be used directly outside of the generated code. This is done by adding a conditional in visit_object_type() for both qapi-types.py and qapi-visit.py based on the object name. The comparison of "name.startswith('q_')" is a bit hacky (it's basically duplicating what .is_implicit() already uses), but beats changing the signature of the visit_object_type() callback to pass a new 'implicit' flag. The hack should be temporary: we are considering adding a future patch that consolidates the narrow visit_object_type(..., base, local_members, variants) and visit_object_type_flat(..., all_members, variants) [where different sets of information are already broken out, and the QAPISchemaObjectType is no longer available] into a broader visit_object_type(obj_type) [where the visitor can query the needed fields from obj_type directly]. Also, now that we WANT to output C code for implicits, we no longer need the visit_needed() filter, leaving 'q_empty' as the only object still needing a special case. Remember, 'q_empty' is the only built-in generated object, which means that without a special case it would be emitted in multiple files (the main qapi-types.h and in qga-qapi-types.h) causing compilation failure due to redefinition. But since it has no members, it's easier to just avoid an attempt to visit that particular type; since gen_object() is called recursively, we also prime the objects_seen set to cover any recursion into the empty type. The patch relies on the changed naming of implicit types in the previous patch. It is a bit unfortunate that the generated struct names and visit_type_FOO_members() don't match normal naming conventions, but it's not too bad, since they will only be used in generated code. The generated code grows substantially in size: the implicit '-wrapper' types must be emitted in qapi-types.h before any union can include an unboxed member of that type. Arguably, the '-args' types could be emitted in a private header for just qapi-visit.c and qmp-marshal.c, rather than polluting qapi-types.h; but adding complexity to the generator to split the output location according to role doesn't seem worth the maintenance costs. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1458254921-17042-6-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-03-17 23:48:30 +01:00
return mcgen('''
qapi: Swap visit_* arguments for consistent 'name' placement 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 <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-01-29 14:48:54 +01:00
void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
{
Error *err = NULL;
visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err);
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
if (err) {
goto out;
}
if (!*obj) {
goto out_obj;
}
visit_type_%(c_name)s_members(v, *obj, &err);
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, &param, &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 <eblake@redhat.com> Message-Id: <1461879932-9020-20-git-send-email-eblake@redhat.com> [Conflict with a doc fixup resolved] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:27 +02:00
if (err) {
goto out_obj;
}
visit_check_struct(v, &err);
qapi: Visit variants in visit_type_FOO_fields() We initially created the static visit_type_FOO_fields() helper function for reuse of code - we have cases where the initial setup for a visit has different allocation (depending on whether the fields represent a stand-alone type or are embedded as part of a larger type), but where the actual field visits are identical once a pointer is available. Up until the previous patch, visit_type_FOO_fields() was only used for structs (no variants), so it was covering every field for each type where it was emitted. Meanwhile, the code for visiting unions looks like: static visit_type_U_fields() { visit base; visit local_members; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit variants; visit_end_struct(); } which splits the fields of the union visit across two functions. Move the code to visit variants to live inside visit_type_U_fields(), while making it conditional on having variants so that all other instances of the helper function remain unchanged. This is also a step closer towards unifying struct and union visits, and towards allowing one union type to be the branch of another flat union. The resulting diff to the generated code is a bit hard to read, but it can be verified that it touches only union types, and that the end result is the following general structure: static visit_type_U_fields() { visit base; visit local_members; visit variants; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit_end_struct(); } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com> [gen_visit_struct_fields() parameter variants made mandatory] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:20 +01:00
out_obj:
2016-06-09 18:48:34 +02:00
visit_end_struct(v, (void **)obj);
qapi: Change visit_type_FOO() to no longer return partial objects Returning a partial object on error is an invitation for a careless caller to leak memory. We already fixed things in an earlier patch to guarantee NULL if visit_start fails ("qapi: Guarantee NULL obj on input visitor callback error"), but that does not help the case where visit_start succeeds but some other failure happens before visit_end, such that we leak a partially constructed object outside visit_type_FOO(). As no one outside the testsuite was actually relying on these semantics, it is cleaner to just document and guarantee that ALL pointer-based visit_type_FOO() functions always leave a safe value in *obj during an input visitor (either the new object on success, or NULL if an error is encountered), so callers can now unconditionally use qapi_free_FOO() to clean up regardless of whether an error occurred. The decision is done by adding visit_is_input(), then updating the generated code to check if additional cleanup is needed based on the type of visitor in use. Note that we still leave *obj unchanged after a scalar-based visit_type_FOO(); I did not feel like auditing all uses of visit_type_Enum() to see if the callers would tolerate a specific sentinel value (not to mention having to decide whether it would be better to use 0 or ENUM__MAX as that sentinel). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-25-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-04-28 23:45:32 +02:00
if (err && visit_is_input(v)) {
qapi_free_%(c_name)s(*obj);
*obj = NULL;
}
qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
2014-05-07 09:53:54 +02:00
out:
error_propagate(errp, err);
}
qapi: Visit variants in visit_type_FOO_fields() We initially created the static visit_type_FOO_fields() helper function for reuse of code - we have cases where the initial setup for a visit has different allocation (depending on whether the fields represent a stand-alone type or are embedded as part of a larger type), but where the actual field visits are identical once a pointer is available. Up until the previous patch, visit_type_FOO_fields() was only used for structs (no variants), so it was covering every field for each type where it was emitted. Meanwhile, the code for visiting unions looks like: static visit_type_U_fields() { visit base; visit local_members; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit variants; visit_end_struct(); } which splits the fields of the union visit across two functions. Move the code to visit variants to live inside visit_type_U_fields(), while making it conditional on having variants so that all other instances of the helper function remain unchanged. This is also a step closer towards unifying struct and union visits, and towards allowing one union type to be the branch of another flat union. The resulting diff to the generated code is a bit hard to read, but it can be verified that it touches only union types, and that the end result is the following general structure: static visit_type_U_fields() { visit base; visit local_members; visit variants; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit_end_struct(); } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com> [gen_visit_struct_fields() parameter variants made mandatory] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-18 07:48:20 +01:00
''',
c_name=c_name(name))
class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
def __init__(self, opt_builtins):
self._opt_builtins = opt_builtins
self.decl = None
self.defn = None
self._btin = None
def visit_begin(self, schema):
self.decl = ''
self.defn = ''
self._btin = '\n' + guardstart('QAPI_VISIT_BUILTIN')
def visit_end(self):
# To avoid header dependency hell, we always generate
# declarations for built-in types in our header files and
# simply guard them. See also opt_builtins (command line
# option -b).
self._btin += guardend('QAPI_VISIT_BUILTIN')
self.decl = self._btin + self.decl
self._btin = None
def visit_enum_type(self, name, info, values, prefix):
# Special case for our lone builtin enum type
# TODO use something cleaner than existence of info
if not info:
self._btin += gen_visit_decl(name, scalar=True)
if self._opt_builtins:
self.defn += gen_visit_enum(name)
else:
self.decl += gen_visit_decl(name, scalar=True)
self.defn += gen_visit_enum(name)
def visit_array_type(self, name, info, element_type):
decl = gen_visit_decl(name)
defn = gen_visit_list(name, element_type)
if isinstance(element_type, QAPISchemaBuiltinType):
self._btin += decl
if self._opt_builtins:
self.defn += defn
else:
self.decl += decl
self.defn += defn
def visit_object_type(self, name, info, base, members, variants):
qapi: Emit implicit structs in generated C We already have several places that want to visit all the members of an implicit object within a larger context (simple union variant, event with anonymous data, command with anonymous arguments struct); and will be adding another one soon (the ability to declare an anonymous base for a flat union). Having a C struct declared for these implicit types, along with a visit_type_FOO_members() helper function, will make for fewer special cases in our generator. We do not, however, need qapi_free_FOO() or visit_type_FOO() functions for implicit types, because they should not be used directly outside of the generated code. This is done by adding a conditional in visit_object_type() for both qapi-types.py and qapi-visit.py based on the object name. The comparison of "name.startswith('q_')" is a bit hacky (it's basically duplicating what .is_implicit() already uses), but beats changing the signature of the visit_object_type() callback to pass a new 'implicit' flag. The hack should be temporary: we are considering adding a future patch that consolidates the narrow visit_object_type(..., base, local_members, variants) and visit_object_type_flat(..., all_members, variants) [where different sets of information are already broken out, and the QAPISchemaObjectType is no longer available] into a broader visit_object_type(obj_type) [where the visitor can query the needed fields from obj_type directly]. Also, now that we WANT to output C code for implicits, we no longer need the visit_needed() filter, leaving 'q_empty' as the only object still needing a special case. Remember, 'q_empty' is the only built-in generated object, which means that without a special case it would be emitted in multiple files (the main qapi-types.h and in qga-qapi-types.h) causing compilation failure due to redefinition. But since it has no members, it's easier to just avoid an attempt to visit that particular type; since gen_object() is called recursively, we also prime the objects_seen set to cover any recursion into the empty type. The patch relies on the changed naming of implicit types in the previous patch. It is a bit unfortunate that the generated struct names and visit_type_FOO_members() don't match normal naming conventions, but it's not too bad, since they will only be used in generated code. The generated code grows substantially in size: the implicit '-wrapper' types must be emitted in qapi-types.h before any union can include an unboxed member of that type. Arguably, the '-args' types could be emitted in a private header for just qapi-visit.c and qmp-marshal.c, rather than polluting qapi-types.h; but adding complexity to the generator to split the output location according to role doesn't seem worth the maintenance costs. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1458254921-17042-6-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-03-17 23:48:30 +01:00
# Nothing to do for the special empty builtin
if name == 'q_empty':
return
self.decl += gen_visit_members_decl(name)
qapi: Emit implicit structs in generated C We already have several places that want to visit all the members of an implicit object within a larger context (simple union variant, event with anonymous data, command with anonymous arguments struct); and will be adding another one soon (the ability to declare an anonymous base for a flat union). Having a C struct declared for these implicit types, along with a visit_type_FOO_members() helper function, will make for fewer special cases in our generator. We do not, however, need qapi_free_FOO() or visit_type_FOO() functions for implicit types, because they should not be used directly outside of the generated code. This is done by adding a conditional in visit_object_type() for both qapi-types.py and qapi-visit.py based on the object name. The comparison of "name.startswith('q_')" is a bit hacky (it's basically duplicating what .is_implicit() already uses), but beats changing the signature of the visit_object_type() callback to pass a new 'implicit' flag. The hack should be temporary: we are considering adding a future patch that consolidates the narrow visit_object_type(..., base, local_members, variants) and visit_object_type_flat(..., all_members, variants) [where different sets of information are already broken out, and the QAPISchemaObjectType is no longer available] into a broader visit_object_type(obj_type) [where the visitor can query the needed fields from obj_type directly]. Also, now that we WANT to output C code for implicits, we no longer need the visit_needed() filter, leaving 'q_empty' as the only object still needing a special case. Remember, 'q_empty' is the only built-in generated object, which means that without a special case it would be emitted in multiple files (the main qapi-types.h and in qga-qapi-types.h) causing compilation failure due to redefinition. But since it has no members, it's easier to just avoid an attempt to visit that particular type; since gen_object() is called recursively, we also prime the objects_seen set to cover any recursion into the empty type. The patch relies on the changed naming of implicit types in the previous patch. It is a bit unfortunate that the generated struct names and visit_type_FOO_members() don't match normal naming conventions, but it's not too bad, since they will only be used in generated code. The generated code grows substantially in size: the implicit '-wrapper' types must be emitted in qapi-types.h before any union can include an unboxed member of that type. Arguably, the '-args' types could be emitted in a private header for just qapi-visit.c and qmp-marshal.c, rather than polluting qapi-types.h; but adding complexity to the generator to split the output location according to role doesn't seem worth the maintenance costs. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1458254921-17042-6-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-03-17 23:48:30 +01:00
self.defn += gen_visit_object_members(name, base, members, variants)
# TODO Worth changing the visitor signature, so we could
# directly use rather than repeat type.is_implicit()?
if not name.startswith('q_'):
# only explicit types need an allocating visit
self.decl += gen_visit_decl(name)
self.defn += gen_visit_object(name, base, members, variants)
def visit_alternate_type(self, name, info, variants):
self.decl += gen_visit_decl(name)
self.defn += gen_visit_alternate(name, variants)
def gen_visit(schema, output_dir, prefix, opt_builtins):
blurb = ' * Schema-defined QAPI visitors'
genc = QAPIGenC(blurb, __doc__)
genh = QAPIGenH(blurb, __doc__)
genc.add(mcgen('''
#include "qemu/osdep.h"
#include "qemu-common.h"
2016-03-14 09:01:28 +01:00
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
#include "%(prefix)sqapi-visit.h"
''',
prefix=prefix))
genh.add(mcgen('''
#include "qapi/visitor.h"
#include "%(prefix)sqapi-types.h"
''',
prefix=prefix))
vis = QAPISchemaGenVisitVisitor(opt_builtins)
schema.visit(vis)
genc.add(vis.defn)
genh.add(vis.decl)
genc.write(output_dir, prefix + 'qapi-visit.c')
genh.write(output_dir, prefix + 'qapi-visit.h')