From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35268) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGP30-00086q-FK for qemu-devel@nongnu.org; Tue, 05 Jan 2016 05:41:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGP2x-0000LL-3i for qemu-devel@nongnu.org; Tue, 05 Jan 2016 05:41:02 -0500 Received: from smtp.citrix.com ([66.165.176.89]:54603) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGP2w-0000Kz-VY for qemu-devel@nongnu.org; Tue, 05 Jan 2016 05:40:59 -0500 Date: Tue, 5 Jan 2016 10:40:12 +0000 From: Stefano Stabellini In-Reply-To: <568B6F6B.3020800@cn.fujitsu.com> Message-ID: References: <1451223640-2569-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1451223640-2569-2-git-send-email-caoj.fnst@cn.fujitsu.com> <568B6F6B.3020800@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" 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: Cao jin Cc: qemu-devel@nongnu.org, Stefano Stabellini On Tue, 5 Jan 2016, Cao jin wrote: > 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. This is true, however I think that relying on it is error prone: in a couple of years from now somebody might change the call sequence without updating the error handling (easy to forget), causing QEMU to crash on error. I think it is safer not to rely on errp != 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? I think we only need to use a local Error variable when we want to check for the returned error, in cases such as: if (*errp) { In other cases, when we are not interested in *errp, we can simply propagate the error, like you have done in your patches.