From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57234) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGLvk-0005pa-MR for qemu-devel@nongnu.org; Tue, 05 Jan 2016 02:21:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGLvh-0005E2-ES for qemu-devel@nongnu.org; Tue, 05 Jan 2016 02:21:20 -0500 Received: from [59.151.112.132] (port=25543 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGLvh-0005Db-1s for qemu-devel@nongnu.org; Tue, 05 Jan 2016 02:21:17 -0500 References: <1451223640-2569-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1451223640-2569-2-git-send-email-caoj.fnst@cn.fujitsu.com> From: Cao jin Message-ID: <568B6F6B.3020800@cn.fujitsu.com> Date: Tue, 5 Jan 2016 15:23:23 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: qemu-devel@nongnu.org On 01/04/2016 11:15 PM, Stefano Stabellini wrote: > On Sun, 27 Dec 2015, Cao jin wrote: >> To catch the error msg. Also modify the caller >> >> Signed-off-by: Cao jin > > This looks much better, thanks. > > [...] >> >> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain, >> - uint8_t bus, uint8_t dev, uint8_t func) >> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain, >> + uint8_t bus, uint8_t dev, uint8_t func, >> + Error **errp) >> { >> unsigned int v; >> - int rc = 0; >> >> d->config_fd = -1; >> d->domain = domain; >> @@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain, >> d->dev = dev; >> d->func = func; >> >> - rc = xen_host_pci_config_open(d); >> - if (rc) { >> + xen_host_pci_config_open(d, errp); >> + if (*errp) { > > I think that errp could be NULL, therefore the right way to do this is: > > Error *err = NULL; > foo(arg, &err); > if (err) { > handle the error... > error_propagate(errp, err); > } > > see the comment at the beginning of include/qapi/error.h. > Hi stefano, I read that comment, and find something maybe new: "errp could be NULL", I think it is saying, if we are in a .realize() function, yes, *errp* maybe NULL, but reality is, here is the callee of .realize(), and we defined a local variable: Error *local_err = NULL in .realize() and passed it to all the callee, so, theoretically *errp* won`t be NULL. so the way you said above is suitable in .realize() IMHO, and I also did it in that way. comment also says: * Receive an error and pass it on to the caller: * Error *err = NULL; * foo(arg, &err); * if (err) { * handle the error... * error_propagate(errp, err); * } * where Error **errp is a parameter, by convention the last one. If I understand the last sentence well, the Error **errp in .realize() prototype is *the last one*, so we could call error_propagate(errp, err) only in .realize() The comment also says: * But when all you do with the error is pass it on, please use * foo(arg, errp); * for readability." We just pass error on in all the callees, so I guess I also did as comment suggest? How do you think? [...] -- Yours Sincerely, Cao Jin