From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48538) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzvSU-0000pS-0P for qemu-devel@nongnu.org; Wed, 25 Jun 2014 18:14:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WzvSK-0005Hj-Vv for qemu-devel@nongnu.org; Wed, 25 Jun 2014 18:14:25 -0400 Message-ID: <53AB49A4.9030700@gmail.com> Date: Thu, 26 Jun 2014 06:13:56 +0800 From: Chen Gang MIME-Version: 1.0 References: <53A84797.9040304@gmail.com> <20140624104614.GD3458@noname.redhat.com> <87d2dywoyn.fsf@blackfin.pond.sub.org> <53A9A0A2.9010904@msgid.tls.msk.ru> In-Reply-To: <53A9A0A2.9010904@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-trivial] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: Kevin Wolf , qemu-devel@nongnu.org, famz@redhat.com, qemu-trivial@nongnu.org, Markus Armbruster , mreitz@redhat.com, stefanha@redhat.com Thank you for all of your work, if necessary to send patch v3 for it (may change the comments), please let me know. Thanks. On 06/25/2014 12:00 AM, Michael Tokarev wrote: > 24.06.2014 15:01, Markus Armbruster wrote: >> Kevin Wolf writes: >> >>> Am 23.06.2014 um 17:28 hat Chen Gang geschrieben: >>>> When failure occurs, 'ret' need be set, or may return 0 to indicate success. >>>> And error_propagate() also need be called only one time within a function. >>>> >>>> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still >>>> set errp when error occurs -- although it contents return value internally. >>>> >>>> So let bdrv_append_temp_snapshot() internal return value outside, and let >>>> all things normal, then fix the issue too. >>>> >>>> Signed-off-by: Chen Gang >>> >>> What does this fix? >> >> It fixes the return value of bdrv_open() when >> bdrv_append_temp_snapshot() fails. Before this patch, it returns a >> positive value, which is wrong. After the patch, it returns the >> negative error code bdrv_append_temp_snapshot() now returns. > > So, what should be done there? Kevin, maybe you should pick this up > instead of going -trivial route? > >>> Having both a return value and an Error* object is duplication and >>> only a sign that a function hasn't been fully converted to the Error >>> framework yet. We shouldn't introduce new instances of this without a >>> very good reason. >> >> Maybe. But I very much prefer >> >> ret = foo(arg, errp); >> if (ret < 0) { >> return ret; >> } >> >> over >> >> Error *local_err = NULL; >> >> foo(arg, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> return; >> } > > Yes, this new error propagation is a bit ugly, I dislike it too. > > Thanks, > > /mjt > -- Chen Gang Open share and attitude like air water and life which God blessed