| Message ID | 1453902888-20457-3-git-send-email-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/27/2016 06:54 AM, Markus Armbruster wrote: > The generated code can call visit_end_union() without having called > visit_start_union(). Example: > > if (!*obj) { > goto out_obj; > } > visit_type_BlockdevOptions_fields(v, obj, &err); > if (err) { > goto out_obj; // if we go from here... > } > if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { > goto out_obj; > } > switch ((*obj)->driver) { > [...] > } > out_obj: > // ... then *obj is true, and ... > error_propagate(errp, err); > err = NULL; > if (*obj) { > // we end up here > visit_end_union(v, !!(*obj)->u.data, &err); > } > error_propagate(errp, err); > > Harmless only because no visitor implements end_union(). Clean it up > anyway. I plan on deleting visit_end_union() anyways (and visit_start_union); see 32/37, plus the FIXME comments added in 21/37. Maybe it's easier to just delete this incorrect and unused callback earlier in the series, using your commit message as additional rationale why it is worthless, and leaving only visit_start_union() cleanups for 32/37. > > Messed up since we have visit_end_union (commit cee2ded). Indeed.
Eric Blake <eblake@redhat.com> writes: > On 01/27/2016 06:54 AM, Markus Armbruster wrote: >> The generated code can call visit_end_union() without having called >> visit_start_union(). Example: >> >> if (!*obj) { >> goto out_obj; >> } >> visit_type_BlockdevOptions_fields(v, obj, &err); >> if (err) { >> goto out_obj; // if we go from here... >> } >> if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { >> goto out_obj; >> } >> switch ((*obj)->driver) { >> [...] >> } >> out_obj: >> // ... then *obj is true, and ... >> error_propagate(errp, err); >> err = NULL; >> if (*obj) { >> // we end up here >> visit_end_union(v, !!(*obj)->u.data, &err); >> } >> error_propagate(errp, err); Tabs crept into my commit message, oops. >> >> Harmless only because no visitor implements end_union(). Clean it up >> anyway. > > I plan on deleting visit_end_union() anyways (and visit_start_union); > see 32/37, plus the FIXME comments added in 21/37. Maybe it's easier to > just delete this incorrect and unused callback earlier in the series, > using your commit message as additional rationale why it is worthless, > and leaving only visit_start_union() cleanups for 32/37. I could've tried to move PATCH 32 before the unification, but that would've been work, so I went with this straightforward fix instead. Sure, it fixes something that'll go away soon, but I think the churn is quite tolerable: 1 file changed, 2 insertions(+), 4 deletions(-). >> Messed up since we have visit_end_union (commit cee2ded). > > Indeed.
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 5c0d4d2..8dcc6dc 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -299,12 +299,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error default: abort(); } -out_obj: error_propagate(errp, err); err = NULL; - if (*obj) { - visit_end_union(v, !!(*obj)->u.data, &err); - } + visit_end_union(v, !!(*obj)->u.data, &err); +out_obj: error_propagate(errp, err); err = NULL; visit_end_struct(v, &err);
The generated code can call visit_end_union() without having called visit_start_union(). Example: if (!*obj) { goto out_obj; } visit_type_BlockdevOptions_fields(v, obj, &err); if (err) { goto out_obj; // if we go from here... } if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { goto out_obj; } switch ((*obj)->driver) { [...] } out_obj: // ... then *obj is true, and ... error_propagate(errp, err); err = NULL; if (*obj) { // we end up here visit_end_union(v, !!(*obj)->u.data, &err); } error_propagate(errp, err); Harmless only because no visitor implements end_union(). Clean it up anyway. Messed up since we have visit_end_union (commit cee2ded). Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi-visit.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)