From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52482) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYD6L-0004l4-Cj for qemu-devel@nongnu.org; Thu, 10 Apr 2014 07:25:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WYD6C-00032p-Gk for qemu-devel@nongnu.org; Thu, 10 Apr 2014 07:25:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYD6C-00032l-8f for qemu-devel@nongnu.org; Thu, 10 Apr 2014 07:24:52 -0400 Date: Thu, 10 Apr 2014 13:24:46 +0200 From: Kevin Wolf Message-ID: <20140410112446.GC4038@noname.redhat.com> References: <871tx6zeoz.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <871tx6zeoz.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] Error propagation in generated visitors and command marshallers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth , Luiz Capitulino , Anthony Liguori , Cole Robinson , Paolo Bonzini Am 09.04.2014 um 17:48 hat Markus Armbruster geschrieben: > I stumbled over this while trying to purge error_is_set() from the code. > > > Here's how we commonly use the Error API: > > Error *err = NULL; > > foo(arg, &err) > if (err) { > goto out; > } > bar(arg, &err) > if (err) { > goto out; > } > > This ensures that err is null on entry, both for foo() and for bar(). > Many functions rely on that, like this: > > void foo(ArgType arg, Error **errp) > { > if (frobnicate(arg) < 0) { > error_setg(errp, "Can't frobnicate"); > // This asserts errp != NULL > } > } > > > Here's how some of our visitor code uses the Error API (for real code, > check out generated qmp-marshal.c): > > Error *err = NULL; > QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args)); > Visitor *v = qmp_input_get_visitor(mi); > char *foo = NULL; > char *bar = NULL; > > visit_type_str(v, &foo, "foo", &err); > visit_type_str(v, &bar, "bar", &err); > if (err) { > goto out; > } > > Unlike above, this may pass a non-null errp to the second > visit_type_str(), namely when the first one fails. > > The visitor functions guard against that, like this: > > void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp) > { > if (!error_is_set(errp)) { > v->type_str(v, obj, name, errp); > } > } > > As discussed before, error_is_set() is almost almost wrong, fragile or > unclean. What if errp is null? Then we fail to stop visiting after an > error. > > The function could be improved like this: > > void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp) > { > assert(errp); > if (!*errp) { > v->type_str(v, obj, name, errp); > } > } > > > But: is it a good idea to have both patterns in the code? Should we > perhaps use the common pattern for visiting, too? Like this: > > visit_type_str(v, &foo, "foo", &err); > if (err) { > goto out; > } > visit_type_str(v, &bar, "bar", &err); > if (err) { > goto out; > } > > Then we can assume *errp is clear on function entry, like this: > > void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp) > { > v->type_str(v, obj, name, errp); > } > > Should execute roughly the same number of conditional branches. > > Tedious repetition of "if (err) goto out" in the caller, but that's what > we do elsewhere, and unlike elsewhere, these one's are generated. > > Opinions? I agree, use the same style as everywhere else. The pattern in the generated visitor that I find more annoying, though, is that it has a lot of code like: if (!error_is_set(errp)) { /* long block of code here */ } And I believe there are even cases where this nests. There are also error_propagate() calls that can (and do in the common case) propagate NULL, this way selecting the first error, if any, but not stopping on the first error. I always found it confusing to read that code. Kevin