Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get()

[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 <address@hidden>
> > 
> > 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.