From patchwork Tue Jul 17 14:17:04 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laszlo Ersek X-Patchwork-Id: 171452 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id AAADA2C0093 for ; Wed, 18 Jul 2012 01:09:04 +1000 (EST) Received: from localhost ([::1]:38568 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sr8aG-00055e-2S for incoming@patchwork.ozlabs.org; Tue, 17 Jul 2012 10:17:04 -0400 Received: from eggs.gnu.org ([208.118.235.92]:43051) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sr8ZZ-0003Q3-Em for qemu-devel@nongnu.org; Tue, 17 Jul 2012 10:16:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sr8ZR-00083m-FU for qemu-devel@nongnu.org; Tue, 17 Jul 2012 10:16:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12465) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sr8ZR-00083P-5X for qemu-devel@nongnu.org; Tue, 17 Jul 2012 10:16:13 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q6HEGCV0031845 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 17 Jul 2012 10:16:12 -0400 Received: from lacos-laptop.usersys.redhat.com (vpn1-5-251.ams2.redhat.com [10.36.5.251]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q6HEG9K9022379; Tue, 17 Jul 2012 10:16:11 -0400 From: Laszlo Ersek To: qemu-devel@nongnu.org, lersek@redhat.com Date: Tue, 17 Jul 2012 16:17:04 +0200 Message-Id: <1342534641-22450-2-git-send-email-lersek@redhat.com> In-Reply-To: <1342534641-22450-1-git-send-email-lersek@redhat.com> References: <1342534641-22450-1-git-send-email-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH v3 01/18] qapi: fix error propagation X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org From: Paolo Bonzini Don't overwrite / leak previously set errors. Make traversal cope with missing mandatory sub-structs. Don't try to end a container that could not be started. v1->v2: - unchanged v2->v3: - instead of examining, assert that we never overwrite errors with error_set() - allow visitors to set a NULL struct pointer successfully, so traversal of incomplete objects can continue - check for a NULL "obj" before accessing "(*obj)->has_XXX" (this is not a typo, "obj != NULL" implies "*obj != NULL" here) - fix start_struct / end_struct balance for unions as well Signed-off-by: Paolo Bonzini Signed-off-by: Laszlo Ersek --- error.h | 2 +- error.c | 3 +- qapi/qapi-visit-core.c | 10 +-- tests/test-qmp-input-visitor.c | 24 ++++-- docs/qapi-code-gen.txt | 2 + scripts/qapi-visit.py | 150 +++++++++++++++++++++++++-------------- 6 files changed, 121 insertions(+), 70 deletions(-) diff --git a/error.h b/error.h index 45ff6c1..3d9d96d 100644 --- a/error.h +++ b/error.h @@ -57,7 +57,7 @@ void error_set_field(Error *err, const char *field, const char *value); /** * Propagate an error to an indirect pointer to an error. This function will * always transfer ownership of the error reference and handles the case where - * dst_err is NULL correctly. + * dst_err is NULL correctly. Errors after the first are discarded. */ void error_propagate(Error **dst_err, Error *local_err); diff --git a/error.c b/error.c index a52b771..58f55a0 100644 --- a/error.c +++ b/error.c @@ -32,6 +32,7 @@ void error_set(Error **errp, const char *fmt, ...) if (errp == NULL) { return; } + assert(*errp == NULL); err = g_malloc0(sizeof(*err)); @@ -132,7 +133,7 @@ bool error_is_type(Error *err, const char *fmt) void error_propagate(Error **dst_err, Error *local_err) { - if (dst_err) { + if (dst_err && !*dst_err) { *dst_err = local_err; } else if (local_err) { error_free(local_err); diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index ffffbf7..0a513d2 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -39,9 +39,8 @@ void visit_start_struct(Visitor *v, void **obj, const char *kind, void visit_end_struct(Visitor *v, Error **errp) { - if (!error_is_set(errp)) { - v->end_struct(v, errp); - } + assert(!error_is_set(errp)); + v->end_struct(v, errp); } void visit_start_list(Visitor *v, const char *name, Error **errp) @@ -62,9 +61,8 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp) void visit_end_list(Visitor *v, Error **errp) { - if (!error_is_set(errp)) { - v->end_list(v, errp); - } + assert(!error_is_set(errp)); + v->end_list(v, errp); } void visit_start_optional(Visitor *v, bool *present, const char *name, diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index c30fdc4..8f5a509 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -151,14 +151,22 @@ typedef struct TestStruct static void visit_type_TestStruct(Visitor *v, TestStruct **obj, const char *name, Error **errp) { - visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct), - errp); - - visit_type_int(v, &(*obj)->integer, "integer", errp); - visit_type_bool(v, &(*obj)->boolean, "boolean", errp); - visit_type_str(v, &(*obj)->string, "string", errp); - - visit_end_struct(v, errp); + Error *err = NULL; + if (!error_is_set(errp)) { + visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct), + &err); + if (!err) { + visit_type_int(v, &(*obj)->integer, "integer", &err); + visit_type_bool(v, &(*obj)->boolean, "boolean", &err); + visit_type_str(v, &(*obj)->string, "string", &err); + + /* Always call end_struct if start_struct succeeded. */ + error_propagate(errp, err); + err = NULL; + visit_end_struct(v, &err); + } + error_propagate(errp, err); + } } static void test_visitor_in_struct(TestInputVisitorData *data, diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index ad11767..cccb11e 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -220,6 +220,8 @@ Example: #endif mdroth@illuin:~/w/qemu2.git$ +(The actual structure of the visit_type_* functions is a bit more complex +in order to propagate errors correctly and avoid leaking memory). === scripts/qapi-commands.py === diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 8d4e94a..04ef7c4 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -17,32 +17,49 @@ import os import getopt import errno -def generate_visit_struct_body(field_prefix, members): - ret = "" +def generate_visit_struct_body(field_prefix, name, members): + ret = mcgen(''' +if (!error_is_set(errp)) { +''') + push_indent() + if len(field_prefix): field_prefix = field_prefix + "." + ret += mcgen(''' +Error **errp = &err; /* from outer scope */ +Error *err = NULL; +visit_start_struct(m, NULL, "", "%(name)s", 0, &err); +''', + name=name) + else: + ret += mcgen(''' +Error *err = NULL; +visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); +''', + name=name) + + ret += mcgen(''' +if (!err) { + if (!obj || *obj) { +''') + + push_indent() + push_indent() for argname, argentry, optional, structured in parse_args(members): if optional: ret += mcgen(''' -visit_start_optional(m, (obj && *obj) ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", errp); -if ((*obj)->%(prefix)shas_%(c_name)s) { +visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err); +if (obj && (*obj)->%(prefix)shas_%(c_name)s) { ''', c_prefix=c_var(field_prefix), prefix=field_prefix, c_name=c_var(argname), name=argname) push_indent() if structured: - ret += mcgen(''' -visit_start_struct(m, NULL, "", "%(name)s", 0, errp); -''', - name=argname) - ret += generate_visit_struct_body(field_prefix + argname, argentry) - ret += mcgen(''' -visit_end_struct(m, errp); -''') + ret += generate_visit_struct_body(field_prefix + argname, argname, argentry) else: ret += mcgen(''' -visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", errp); +visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err); ''', c_prefix=c_var(field_prefix), prefix=field_prefix, type=type_name(argentry), c_name=c_var(argname), @@ -52,7 +69,25 @@ visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, " pop_indent() ret += mcgen(''' } -visit_end_optional(m, errp); +visit_end_optional(m, &err); +''') + + pop_indent() + ret += mcgen(''' + + error_propagate(errp, err); + err = NULL; +} +''') + + pop_indent() + pop_indent() + ret += mcgen(''' + /* Always call end_struct if start_struct succeeded. */ + visit_end_struct(m, &err); + } + error_propagate(errp, err); +} ''') return ret @@ -61,22 +96,14 @@ def generate_visit_struct(name, members): void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) { - if (error_is_set(errp)) { - return; - } - visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp); - if (obj && !*obj) { - goto end; - } ''', name=name) + push_indent() - ret += generate_visit_struct_body("", members) + ret += generate_visit_struct_body("", name, members) pop_indent() ret += mcgen(''' -end: - visit_end_struct(m, errp); } ''') return ret @@ -87,18 +114,23 @@ def generate_visit_list(name, members): void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp) { GenericList *i, **prev = (GenericList **)obj; + Error *err = NULL; - if (error_is_set(errp)) { - return; - } - visit_start_list(m, name, errp); - - for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) { - %(name)sList *native_i = (%(name)sList *)i; - visit_type_%(name)s(m, &native_i->value, NULL, errp); + if (!error_is_set(errp)) { + visit_start_list(m, name, &err); + if (!err) { + for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) { + %(name)sList *native_i = (%(name)sList *)i; + visit_type_%(name)s(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_end_list(m, errp); } ''', name=name) @@ -122,27 +154,23 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** { Error *err = NULL; - if (error_is_set(errp)) { - return; - } - visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); - if (obj && !*obj) { - goto end; - } - visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err); - if (err) { - error_propagate(errp, err); - goto end; - } - switch ((*obj)->kind) { + if (!error_is_set(errp)) { + visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); + if (!err) { + if (!obj || *obj) { + visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err); + if (!err) { + switch ((*obj)->kind) { ''', name=name) + push_indent() + push_indent() for key in members: ret += mcgen(''' - case %(abbrev)s_KIND_%(enum)s: - visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", errp); - break; + case %(abbrev)s_KIND_%(enum)s: + visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err); + break; ''', abbrev = de_camel_case(name).upper(), enum = c_fun(de_camel_case(key)).upper(), @@ -150,11 +178,25 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** c_name=c_fun(key)) ret += mcgen(''' - default: - abort(); + default: + abort(); + } + } + error_propagate(errp, err); + err = NULL; + } +''') + pop_indent() + ret += mcgen(''' + /* Always call end_struct if start_struct succeeded. */ + visit_end_struct(m, &err); } -end: - visit_end_struct(m, errp); + error_propagate(errp, err); +} +''') + + pop_indent(); + ret += mcgen(''' } ''')