From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqLPi-0003nn-Iz for qemu-devel@nongnu.org; Wed, 13 Apr 2016 10:05:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqLPe-0008QD-Gc for qemu-devel@nongnu.org; Wed, 13 Apr 2016 10:05:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35475) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqLPe-0008Py-8w for qemu-devel@nongnu.org; Wed, 13 Apr 2016 10:04:58 -0400 From: Markus Armbruster References: <1460131992-32278-1-git-send-email-eblake@redhat.com> <1460131992-32278-4-git-send-email-eblake@redhat.com> Date: Wed, 13 Apr 2016 16:04:55 +0200 In-Reply-To: <1460131992-32278-4-git-send-email-eblake@redhat.com> (Eric Blake's message of "Fri, 8 Apr 2016 10:12:56 -0600") Message-ID: <87inzlli48.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v14 03/19] qapi: Guarantee NULL obj on input visitor callback error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > Our existing input visitors were not very consistent on errors > in a function taking 'TYPE **obj'. While all of them set '*obj' Suggest to list the methods. I guess it's start_struct(), start_alternate(), type_str(), type_any(). > to allocated storage on success, it was not obvious whether > '*obj' was guaranteed safe on failure, or whether it was left > uninitialized. But a future patch wants to guarantee that > visit_type_FOO() does not leak a partially-constructed obj back > to the caller; it is easier to implement this if we can reliably > state that '*obj' is assigned on exit, even on failures. There are two sane behaviors on failure: (1) don't touch *obj, (2) set it to null. I generally like "do nothing on failure", i.e. (1), but I'm fine with (2) when it's more convenient. We'll see. > The opts-visitor start_struct() doesn't set an error, but it > also was doing a weird check for 0 size; all callers pass in > non-zero size if obj is non-NULL. > > Signed-off-by: Eric Blake > > --- > v14: no change > v13: no change > v12: new patch > --- > qapi/opts-visitor.c | 3 ++- > qapi/qmp-input-visitor.c | 4 ++++ > qapi/string-input-visitor.c | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index f98cf2e..cdb6e42 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -133,7 +133,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, > const QemuOpt *opt; > > if (obj) { > - *obj = g_malloc0(size > 0 ? size : 1); > + *obj = g_malloc0(size); > } > if (ov->depth++ > 0) { > return; > @@ -314,6 +314,7 @@ opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) > > opt = lookup_scalar(ov, name, errp); > if (!opt) { > + *obj = NULL; > return; > } > *obj = g_strdup(opt->str ? opt->str : ""); > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 02d4233..77cce8b 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -120,6 +120,9 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, > QObject *qobj = qmp_input_get_object(qiv, name, true); > Error *err = NULL; > > + if (obj) { > + *obj = NULL; > + } > if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "QDict"); > @@ -267,6 +270,7 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, > QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true)); > > if (!qstr) { > + *obj = NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "string"); > return; > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index d604575..797973a 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -293,6 +293,7 @@ static void parse_type_str(Visitor *v, const char *name, char **obj, > if (siv->string) { > *obj = g_strdup(siv->string); > } else { > + *obj = NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "string"); > } If you want to make setting *obj on failure part of the method contract, you get to write it into the contract. Looks like you do that later in this series, when you retrofit the missing contracts. Okay.