Re: [Xen-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Paul 
> Durrant
> Sent: 12 September 2019 16:16
> To: Anthony Perard <anthony.perard@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; qemu-devel@xxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in 
> XenBus watch handler
> 
> > -----Original Message-----
> > From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > Sent: 12 September 2019 16:04
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano 
> > Stabellini
> <sstabellini@xxxxxxxxxx>
> > Subject: Re: [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch 
> > handler
> >
> > On Thu, Sep 12, 2019 at 01:16:46PM +0100, Paul Durrant wrote:
> > > Cleaning up offine XenDevice objects directly in
> >               ^ offline
> >
> > > xen_device_backend_changed() is dangerous as xen_device_unrealize() will
> > > modify the watch list that is being walked. Even the QLIST_FOREACH_SAFE()
> > > used in notifier_list_notify() is insufficient as *two* notifiers (for
> > > the frontend and backend watches) are removed, thus potentially rendering
> > > the 'next' pointer unsafe.
> > >
> > > The solution is to use the XenBus backend_watch handler to do the clean-up
> > > instead, as it is invoked whilst walking a separate watch list.
> > >
> > > This patch therefore adds a new 'offline_devices' list to XenBus, to which
> > > offline devices are added by xen_device_backend_changed(). The XenBus
> > > backend_watch registration is also changed to not only invoke
> > > xen_bus_enumerate() but also a new xen_bus_cleanup() function, which will
> > > walk 'offline_devices' and perform the necessary actions.
> > > For safety a an extra 'online' check is also added to
> >              ^ one 'a' too many?
> >
> > > xen_bus_type_enumerate() to make sure that no attempt is made to create a
> > > new XenDevice object for a backend that is offline.
> > >
> > > NOTE: This patch also include some cosmetic changes:
> > >       - substitute the local variable name 'backend_state'
> > >         in xen_bus_type_enumerate() with 'state', since there
> > >         is no ambiguity with any other state in that context.
> > >       - change xen_device_state_is_active() to
> > >         xen_device_frontend_is_active() (and pass a XenDevice directly)
> > >         since the state tests contained therein only apply to a frontend.
> > >       - use 'state' rather then 'xendev->backend_state' in
> > >         xen_device_backend_changed() to shorten the code.
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > ---
> > >
> > > v2:
> > >  - Make sure we don't try to add a XenDevice to 'offline_devices' more 
> > > than
> > >    once
> > > ---
> > >
> > >      /*
> > >       * If a backend is still 'online' then we should leave it alone but,
> > > -     * if a backend is not 'online', then the device should be destroyed
> > > -     * once the state is Closed.
> > > +     * if a backend is not 'online', then the device is a candidate
> > > +     * for destruction. Hence add it to the 'offline' list to be cleaned
> > > +     * by xen_bus_cleanup().
> > >       */
> > > -    if (!xendev->backend_online &&
> > > -        (xendev->backend_state == XenbusStateClosed ||
> > > -         xendev->backend_state == XenbusStateInitialising ||
> > > -         xendev->backend_state == XenbusStateInitWait ||
> > > -         xendev->backend_state == XenbusStateUnknown)) {
> > > -        Error *local_err = NULL;
> > > +    if (!online &&
> > > +        (state == XenbusStateClosed ||  state == XenbusStateInitialising 
> > > ||
> > > +         state == XenbusStateInitWait || state == XenbusStateUnknown) &&
> > > +        !QLIST_NEXT(xendev, list)) {
> >
> > Could you add a note about this QLIST_NEXT? I don't think it's going to
> > be obvious enough why we check that there are no next item. I might only
> > understand it just because of your reply to the v1 of the patch.
> > (Well the changelog of the patch also point out what it's for.)
> >
> 
> Sure, it's worth a comment.

Actually, on closer inspection !QLIST_NEXT() is an insufficient checked. I had 
assumed that QLISTs were full doubly-linked lists but they are not; the last 
element on a list will still have a NULL next pointer. It will be sufficient, 
and also clearer to the reader, if I add a boolean to XenDevice which is set 
when it is added to the list.

  Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel