Skip to content

Commit

Permalink
qapi: Replace uncommon use of the error API by the common one
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
Markus Armbruster authored and Luiz Capitulino committed May 15, 2014
1 parent cdaec38 commit 297a364
Show file tree
Hide file tree
Showing 10 changed files with 354 additions and 286 deletions.
87 changes: 50 additions & 37 deletions docs/qapi-code-gen.txt
View file
Expand Up @@ -275,7 +275,6 @@ Example:
qapi_dealloc_visitor_cleanup(md);
}


void qapi_free_UserDefOne(UserDefOne * obj)
{
QapiDeallocVisitor *md;
Expand Down Expand Up @@ -352,49 +351,54 @@ Example:
{
Error *err = NULL;
visit_type_int(m, &(*obj)->integer, "integer", &err);
if (err) {
goto out;
}
visit_type_str(m, &(*obj)->string, "string", &err);
if (err) {
goto out;
}

out:
error_propagate(errp, err);
}

void visit_type_UserDefOne(Visitor *m, UserDefOne ** obj, const char *name, Error **errp)
{
if (!error_is_set(errp)) {
Error *err = NULL;
visit_start_struct(m, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), &err);
if (!err) {
if (*obj) {
visit_type_UserDefOne_fields(m, obj, &err);
error_propagate(errp, err);
err = NULL;
}
/* Always call end_struct if start_struct succeeded. */
visit_end_struct(m, &err);
Error *err = NULL;

visit_start_struct(m, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), &err);
if (!err) {
if (*obj) {
visit_type_UserDefOne_fields(m, obj, errp);
}
error_propagate(errp, err);
visit_end_struct(m, &err);
}
error_propagate(errp, err);
}

void visit_type_UserDefOneList(Visitor *m, UserDefOneList ** obj, const char *name, Error **errp)
{
GenericList *i, **prev = (GenericList **)obj;
Error *err = NULL;
GenericList *i, **prev;

if (!error_is_set(errp)) {
visit_start_list(m, name, &err);
if (!err) {
for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
UserDefOneList *native_i = (UserDefOneList *)i;
visit_type_UserDefOne(m, &native_i->value, NULL, &err);
}
error_propagate(errp, err);
err = NULL;

/* Always call end_list if start_list succeeded. */
visit_end_list(m, &err);
}
error_propagate(errp, err);
visit_start_list(m, name, &err);
if (err) {
goto out;
}

for (prev = (GenericList **)obj;
!err && (i = visit_next_list(m, prev, &err)) != NULL;
prev = &i) {
UserDefOneList *native_i = (UserDefOneList *)i;
visit_type_UserDefOne(m, &native_i->value, NULL, &err);
}

error_propagate(errp, err);
err = NULL;
visit_end_list(m, &err);
out:
error_propagate(errp, err);
}
mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qapi-visit.h
[Uninteresting stuff omitted...]
Expand Down Expand Up @@ -434,15 +438,20 @@ Example:

static void qmp_marshal_output_my_command(UserDefOne * ret_in, QObject **ret_out, Error **errp)
{
Error *local_err = NULL;
QmpOutputVisitor *mo = qmp_output_visitor_new();
QapiDeallocVisitor *md;
Visitor *v;

v = qmp_output_get_visitor(mo);
visit_type_UserDefOne(v, &ret_in, "unused", errp);
if (!error_is_set(errp)) {
*ret_out = qmp_output_get_qobject(mo);
visit_type_UserDefOne(v, &ret_in, "unused", &local_err);
if (local_err) {
goto out;
}
*ret_out = qmp_output_get_qobject(mo);

out:
error_propagate(errp, local_err);
qmp_output_visitor_cleanup(mo);
md = qapi_dealloc_visitor_new();
v = qapi_dealloc_get_visitor(md);
Expand All @@ -452,24 +461,28 @@ Example:

static void qmp_marshal_input_my_command(QDict *args, QObject **ret, Error **errp)
{
Error *local_err = NULL;
UserDefOne * retval = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
QapiDeallocVisitor *md;
Visitor *v;
UserDefOne * arg1 = NULL;

v = qmp_input_get_visitor(mi);
visit_type_UserDefOne(v, &arg1, "arg1", errp);

if (error_is_set(errp)) {
visit_type_UserDefOne(v, &arg1, "arg1", &local_err);
if (local_err) {
goto out;
}
retval = qmp_my_command(arg1, errp);
if (!error_is_set(errp)) {
qmp_marshal_output_my_command(retval, ret, errp);

retval = qmp_my_command(arg1, &local_err);
if (local_err) {
goto out;
}

qmp_marshal_output_my_command(retval, ret, &local_err);

out:
error_propagate(errp, local_err);
qmp_input_visitor_cleanup(mi);
md = qapi_dealloc_visitor_new();
v = qapi_dealloc_get_visitor(md);
Expand Down
24 changes: 22 additions & 2 deletions hw/timer/mc146818rtc.c
View file
Expand Up @@ -804,13 +804,33 @@ static void rtc_get_date(Object *obj, Visitor *v, void *opaque,
goto out;
}
visit_type_int32(v, &current_tm.tm_year, "tm_year", &err);
if (err) {
goto out_end;
}
visit_type_int32(v, &current_tm.tm_mon, "tm_mon", &err);
if (err) {
goto out_end;
}
visit_type_int32(v, &current_tm.tm_mday, "tm_mday", &err);
if (err) {
goto out_end;
}
visit_type_int32(v, &current_tm.tm_hour, "tm_hour", &err);
if (err) {
goto out_end;
}
visit_type_int32(v, &current_tm.tm_min, "tm_min", &err);
if (err) {
goto out_end;
}
visit_type_int32(v, &current_tm.tm_sec, "tm_sec", &err);
visit_end_struct(v, &err);

if (err) {
goto out_end;
}
out_end:
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, errp);
out:
error_propagate(errp, err);
}
Expand Down
12 changes: 8 additions & 4 deletions hw/virtio/virtio-balloon.c
View file
Expand Up @@ -121,23 +121,27 @@ static void balloon_stats_get_all(Object *obj, struct Visitor *v,
if (err) {
goto out;
}

visit_type_int(v, &s->stats_last_update, "last-update", &err);
if (err) {
goto out_end;
}

visit_start_struct(v, NULL, NULL, "stats", 0, &err);
if (err) {
goto out_end;
}

for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
for (i = 0; !err && i < VIRTIO_BALLOON_S_NR; i++) {
visit_type_int64(v, (int64_t *) &s->stats[i], balloon_stat_names[i],
&err);
}
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);

out_end:
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);

out:
error_propagate(errp, err);
}
Expand Down

0 comments on commit 297a364

Please sign in to comment.