From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W9BcI-0000YX-Qv for qemu-devel@nongnu.org; Fri, 31 Jan 2014 05:46:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W9BcH-0008UL-7V for qemu-devel@nongnu.org; Fri, 31 Jan 2014 05:46:34 -0500 Received: from mail-oa0-x233.google.com ([2607:f8b0:4003:c02::233]:47540) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W9BcH-0008U8-22 for qemu-devel@nongnu.org; Fri, 31 Jan 2014 05:46:33 -0500 Received: by mail-oa0-f51.google.com with SMTP id h16so5019871oag.10 for ; Fri, 31 Jan 2014 02:46:32 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <52EB68D5.3050207@kamp.de> 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> Date: Fri, 31 Jan 2014 11:46:31 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: Peter Lieven Cc: =?ISO-8859-1?Q?Beno=EEt_Canet?= , Kevin Wolf , Fam Zheng , ronnie sahlberg , Jeff Cody , qemu-devel , Max Reitz , Orit Wasserman , Federico Simoncelli , Stefan Hajnoczi , Wenchao Xia 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=C4=ABt Canet wrote: >>>>> >>>>> Le Wednesday 29 Jan 2014 =C4=85 09:50:21 (+0100), Peter Lieven a =C3= =A9crit : >>>>>> >>>>>> +static int nfs_file_open(BlockDriverState *bs, QDict *options, int >>>>>> flags, >>>>>> + Error **errp) { >>>>>> + NFSClient *client =3D bs->opaque; >>>>>> + int64_t ret; >>>>>> + QemuOpts *opts; >>>>>> + Error *local_err =3D NULL; >>>>>> + >>>>>> + opts =3D 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 =3D=3D 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 =3D 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