From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54956) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYa7k-0006ab-Nd for qemu-devel@nongnu.org; Fri, 11 Apr 2014 08:00:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WYa7e-00058x-Hs for qemu-devel@nongnu.org; Fri, 11 Apr 2014 08:00:00 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:48548) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYa7e-00058s-Bq for qemu-devel@nongnu.org; Fri, 11 Apr 2014 07:59:54 -0400 Received: by mail-wi0-f178.google.com with SMTP id bs8so869141wib.5 for ; Fri, 11 Apr 2014 04:59:53 -0700 (PDT) MIME-Version: 1.0 Sender: peter.crosthwaite@petalogix.com In-Reply-To: <871tx6zeoz.fsf@blackfin.pond.sub.org> References: <871tx6zeoz.fsf@blackfin.pond.sub.org> Date: Fri, 11 Apr 2014 21:59:53 +1000 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=ISO-8859-1 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 Developers" , Michael Roth , Luiz Capitulino , Anthony Liguori , Cole Robinson , Paolo Bonzini On Thu, Apr 10, 2014 at 1:48 AM, Markus Armbruster wrote: > 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. That should be the callers problem. If you pass a NULL errp then the intended semantic is to ignore errors. > > The function could be improved like this: > > void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp) > { > assert(errp); And this is irregular in that you are now mandating the Error ** and thus removing the capability to ignore errors. > 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 think this code as-is is a good example of what we should do elsewhere. The code base has bloated with the if (error) { bail; } on every Error ** accepting API call. I proposed a while back a semantic that Error ** Accepting APIs perform no action when the error is already set to allow for long sequences of calls to run without the constant checks. You then report the first error in a catchall at the end of the run. I think this particular code is probably good, provided your case of NULL errp is enforced against by the caller. Regards, Peter