From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35094) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W9C5k-0007sq-DJ for qemu-devel@nongnu.org; Fri, 31 Jan 2014 06:17:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W9C5e-0001o9-QU for qemu-devel@nongnu.org; Fri, 31 Jan 2014 06:17:00 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:52453 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W9C5e-0001ld-G2 for qemu-devel@nongnu.org; Fri, 31 Jan 2014 06:16:54 -0500 Message-ID: <52EB8616.4020005@kamp.de> Date: Fri, 31 Jan 2014 12:16:38 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1390985425-13165-1-git-send-email-pl@kamp.de> <1390985425-13165-2-git-send-email-pl@kamp.de> <20140129161959.GG3079@irqsave.net> <20140130142224.GA15494@stefanha-thinkpad.redhat.com> <9B01B588-64FC-45B9-9212-909781C0EA31@kamp.de> <52EB68D5.3050207@kamp.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , Kevin Wolf , Fam Zheng , ronnie sahlberg , Jeff Cody , qemu-devel , Max Reitz , Orit Wasserman , Federico Simoncelli , Stefan Hajnoczi , Wenchao Xia On 31.01.2014 11:46, Stefan Hajnoczi wrote: > On Fri, Jan 31, 2014 at 10:11 AM, Peter Lieven wrote: >> On 31.01.2014 09:57, Stefan Hajnoczi wrote: >>> On Thu, Jan 30, 2014 at 10:35 PM, Peter Lieven wrote: >>>> Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi : >>>> >>>>> On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoīt Canet wrote: >>>>>> Le Wednesday 29 Jan 2014 ą 09:50:21 (+0100), Peter Lieven a écrit : >>>>>>> +static int nfs_file_open(BlockDriverState *bs, QDict *options, int >>>>>>> flags, >>>>>>> + Error **errp) { >>>>>>> + NFSClient *client = bs->opaque; >>>>>>> + int64_t ret; >>>>>>> + QemuOpts *opts; >>>>>>> + Error *local_err = NULL; >>>>>>> + >>>>>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); >>>>>>> + qemu_opts_absorb_qdict(opts, options, &local_err); >>>>>>> + if (error_is_set(&local_err)) { >>>>>>> + qerror_report_err(local_err); >>>>>> I have seen more usage of error_propagate(errp, local_err); in QEMU >>>>>> code. >>>>>> Maybe I am missing the point. >>>>> Yes, I think you are right. The Error should be propagated to the >>>>> caller. It's not clear to me whether we can ever get an error from >>>>> qemu_opts_absorb_qdict() in this call site though. >>>> Is there any action I should take here? If yes, can you advise what >>>> to do please. >>> The issue is that nfs_file_open() takes an Error **errp argument. >>> This means the function should report detailed errors using the Error >>> object. >>> >>> The patch prints and then discards the local_error instead of >>> propagating it to the caller's errp. >>> >>> We should just propagate the error instead of printing it: >>> if (error_is_set(&local_err)) { >>> error_propagate(errp, local_err); >>> goto ...; >> >> Ok, you are just referring to this part in nfs_file_open: >> >> if (error_is_set(&local_err)) { >> qerror_report_err(local_err); >> error_free(local_err); >> return -EINVAL; >> } >> >> which I would change to: >> >> if (error_is_set(&local_err)) { >> error_propagate(errp, local_err); >> return -EINVAL; >> } > Yes. > >> The use of error_setg in nfs_client_open is ok? > Yes, it's fine. > > The Error API is not 100% obvious when you first see it, but once you > learn the conventions it's pretty usable: > > Functions take an Error **errp argument. This argument is optional, > so a caller that doesn't care about detailed error messages may pass > NULL. This has implications... > > error_setg(errp, fmt, ...) handles errp == NULL internally so you can > call it unconditionally. > > The tricky thing is that error_is_set() only works if errp is non-NULL > (otherwise error_setg() skips creating an Error object). So it means > you cannot rely on your errp argument and often functions will declare > Error *local_err = NULL, so they can pass &local_err to child > functions. Finally, error_propagate(errp, local_err) is used to pass > out the Error object to our caller. > > Hope this summary makes the Error API clear. > > Stefan Thanks for the explanation. I will sent you an updated series shortly. Peter -- Mit freundlichen Grüßen Peter Lieven ........................................................... KAMP Netzwerkdienste GmbH Vestische Str. 89-91 | 46117 Oberhausen Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40 pl@kamp.de | http://www.kamp.de Geschäftsführer: Heiner Lante | Michael Lante Amtsgericht Duisburg | HRB Nr. 12154 USt-Id-Nr.: DE 120607556 ...........................................................