From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SplGv-0001s3-G8 for qemu-devel@nongnu.org; Fri, 13 Jul 2012 15:11:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SplGu-0004Ap-Dt for qemu-devel@nongnu.org; Fri, 13 Jul 2012 15:11:25 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:59792) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SplGu-0004Ah-7n for qemu-devel@nongnu.org; Fri, 13 Jul 2012 15:11:24 -0400 Received: by pbbro12 with SMTP id ro12so6186844pbb.4 for ; Fri, 13 Jul 2012 12:11:22 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <500072D3.2060901@redhat.com> Date: Fri, 13 Jul 2012 21:11:15 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1339575768-2557-1-git-send-email-lersek@redhat.com> <1339575768-2557-2-git-send-email-lersek@redhat.com> <20120713133852.3a446672@doriath.home> <50005B3D.2070009@redhat.com> In-Reply-To: <50005B3D.2070009@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: qemu-devel@nongnu.org, Luiz Capitulino Il 13/07/2012 19:30, Laszlo Ersek ha scritto: >>> >> - if (errp == NULL) { >>> >> + if (errp == NULL || *errp != NULL) { >> > >> > I think we should use assert() here. >> > >> > If the error is already set, that most probably indicates a bug in the caller, as >> > it's the caller's responsibility to decide which error to return. > I believe we had a good argument against this, but I can't precisely > recall (or find) it now. Paolo, do you remember? Can you please both > search your respective mailboxen for Message-ID > <4FB21B71.7030804@redhat.com>? That's where we started to discuss this. > > I believe I saw some paths in the code that tripped on this leak, and > generally keeping the first error seemed like a good idea. > opts_end_struct() originally checked for any pre-existent error > explicitly, but then the check was moved to the common code. The reason to do this for error_propagate was to allow this idiom: /* Always call end_struct if start_struct succeeded. */ error_propagate(errp, err); err = NULL; visit_end_struct(v, &err); error_propagate(errp, err); I think doing it for error_set was just for symmetry and to avoid introducing excessive complexity. Paolo