Re: [Qemu-devel] [PATCH v2 05/18] xen: add xenstore watcher infrastructure

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 07 December 2018 15:58
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-
> devel@lists.xenproject.org; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v2 05/18] xen: add xenstore watcher infrastructure
> 
> On Thu, Dec 06, 2018 at 03:08:31PM +0000, Paul Durrant wrote:
> > @@ -36,6 +54,12 @@ static void xen_block_unrealize(XenDevice *xendev,
> Error **errp)
> >
> >      trace_xen_block_unrealize(type, vdev->disk, vdev->partition);
> >
> > +    /* Disconnect from the frontend in case this has not already
> happened */
> > +    xen_block_disconnect(xendev, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> 
> If xen_block_disconnect fails, local_err is going to be reuse below. If
> it's fine to try unrealize, then `local_err=NULL` is probably enough.
> 

Actually, on this path there is nothing useful to be done with an error... disconnect really really should not fail, so I'll pass NULL instead to ignore any error.

> > +    }
> > +
> >      if (blockdev_class->unrealize) {
> >          blockdev_class->unrealize(blockdev, &local_err);
> >          if (local_err) {
> 
> [...]
> 
> > +static void xen_bus_remove_watch(XenBus *xenbus, XenWatch *watch,
> > +                                 Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    trace_xen_bus_remove_watch(watch->node, watch->key, watch->token);
> > +
> > +    xs_node_unwatch(xenbus->xsh, watch->node, watch->key, watch->token,
> > +                    &local_err);
> 
> You could simply pass `errp' directly instead of having `local_err'.
> 

Indeed.

  Paul

> > +
> > +    notifier_remove(&watch->notifier);
> > +    free_watch(watch);
> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +    }
> > +}
> > +
> 
> --
> Anthony PERARD