From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35613) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDHQz-00009w-Hj for qemu-devel@nongnu.org; Wed, 06 Mar 2013 11:43:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDHQx-0002Kd-Pl for qemu-devel@nongnu.org; Wed, 06 Mar 2013 11:43:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:21512) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDHQx-0002KU-Gh for qemu-devel@nongnu.org; Wed, 06 Mar 2013 11:43:15 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r26GhEG8007066 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 6 Mar 2013 11:43:14 -0500 Message-ID: <5137721F.7060503@redhat.com> Date: Wed, 06 Mar 2013 17:43:11 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1362566886-14073-1-git-send-email-kwolf@redhat.com> <513722BD.6010503@redhat.com> <20130306111126.GA2285@dhcp-200-207.str.redhat.com> <513756D5.1020506@redhat.com> <51375B04.9020402@redhat.com> <20130306151905.GB2285@dhcp-200-207.str.redhat.com> <87a9qg5xjf.fsf@blackfin.pond.sub.org> In-Reply-To: <87a9qg5xjf.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , Laszlo Ersek , qemu-devel@nongnu.org, Luiz Capitulino Il 06/03/2013 16:59, Markus Armbruster ha scritto: >> > >> > I don't really understand the difference. As long as the function >> > doesn't depend on the Error object to be present (which it doesn't), >> > isn't it semantically exactly the same? > I guess it is in this case (I didn't call your patch wrong). However, I > figure that as soon as we go beyond utterly trivial with errors, it's > advisable to put them in local variables, and error_propagate() to the > parameter only at the end. Messing around with errp parameters directly > feels error-prone, and I'm afraid even correct messing could easily lead > to incorrect messing, when folks imitate it without really understanding > what they're doing. Yes, that's my thought more or less. However, the possibilities are: 1) asserting that there is no error. We hardly ever do this, and it's not clear to me that we should start. It would introduce one more error-propagation idiom, and more confusion. 2) do nothing if there is an error. This is what for example the QAPI visitors do. 3) what Laszlo suggested. However, it is almost certainly a bug to call this function with a pre-existing error, because this function has quite "strong" side effects. Hence this might mask a bug and leave pending connections. 4) just your patch. This is just as wrong as (3) if there was a pre-existing error, and it also overwrites the pre-existing error; harder to debug, and inconsistent with functions using the local variables. None is optimal, but (2) seems the best. Paolo > Yes, extra local variables make the error propagation code even bulkier. > No, I don't like it any more than you do.