From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49482) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXSFE-0005z5-2n for qemu-devel@nongnu.org; Sat, 19 Oct 2013 04:50:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VXSF5-0002TD-E3 for qemu-devel@nongnu.org; Sat, 19 Oct 2013 04:50:48 -0400 Received: from mail-ea0-x22f.google.com ([2a00:1450:4013:c01::22f]:44944) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXSF5-0002Q8-4N for qemu-devel@nongnu.org; Sat, 19 Oct 2013 04:50:39 -0400 Received: by mail-ea0-f175.google.com with SMTP id m14so2534248eaj.34 for ; Sat, 19 Oct 2013 01:50:37 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <526247D7.8050404@redhat.com> Date: Sat, 19 Oct 2013 10:50:31 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1381803804-4551-1-git-send-email-famz@redhat.com> <525EE171.20208@redhat.com> <20131017124928.GI10774@stefanha-thinkpad.redhat.com> <20131017130023.GD2942@dhcp-200-207.str.redhat.com> <20131018085150.GA27034@T430s.nay.redhat.com> <52617702.1080906@redhat.com> In-Reply-To: <52617702.1080906@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] mirror: drop local_err in mirror_complete List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , Stefan Hajnoczi , famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Il 18/10/2013 19:59, Max Reitz ha scritto: > Someone obviously intended some purpose for it, so even if it doesn't > make a difference now (and my RFC is unneeded), I'd still use it to > propagate errors (instead of passing the error pointer). My point being > that there *is* a function for propagating errors and I think we should > therefore use it. The current qemu code seems to alternate between the > two alternatives, although using error_propagate seems more common to me > (at least, that was the result when I looked through the code trying to > decide whether to use it or not). > > Generally, we need a proper discussion about whether error_propagate is > obsolete or not. Afterwards, we can adapt the current codebase to the > result of that discussion; but until then, I oppose changing existing > code without actual need to do so but personal preference. error_propagate is not obsolete. It is particularly pervasive in generated code. You can and should skip error_propagate if you are tail-calling another function. In this case, the extra if/error_propagate pair is useless, makes the code less clear and adds 3-4 useless lines of code. If you have an alternative way to see whether an error occurred (typically based on the return value: <0 if it is int, NULL if it is a pointer), it is fine to use it instead of error_propagate, because error_propagate adds some complexity to the logic. It is also fine to use error_propagate; whatever makes the code simpler. However, the converse is not true. If you have a function that returns void and takes an Error*, it is not okay to make it return int or bool for the sake of avoiding error_propagate. There are also cases where you have a return value, but you cannot use it to ascertain whether an error occurred. For example, NULL may be a valid return value even when no error occurs. In such case, you have to use error_propagate. In the end, I agree with Kevin: "If in doubt, use local_err". Tail calls should be the only case where local_err is clearly unnecessary, any other case needs to be considered specifically. Paolo