[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBu
From: |
Paul Durrant |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler |
Date: |
Thu, 12 Sep 2019 15:15:30 +0000 |
> -----Original Message-----
> From: Anthony PERARD <address@hidden>
> Sent: 12 September 2019 16:04
> To: Paul Durrant <address@hidden>
> Cc: address@hidden; address@hidden; Stefano Stabellini <address@hidden>
> 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 <address@hidden>
> > ---
> >
> > 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.
> > + XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> >
> > - if (!xen_backend_try_device_destroy(xendev, &local_err)) {
> > - object_unparent(OBJECT(xendev));
> > - }
> > + QLIST_INSERT_HEAD(&xenbus->offline_devices, xendev, list);
> >
> > - if (local_err) {
> > - error_report_err(local_err);
> > - }
> > + /*
> > + * Re-write the state to cause a XenBus backend_watch notification,
> > + * resulting in a call to xen_bus_cleanup().
> > + */
> > + xen_device_backend_printf(xendev, "state", "%u", state);
>
> It kind of feels wrong to rely on xenstore to notify QEMU's xenbus
> driver that a device needs cleanup. But that does the job.
>
I had originally considered setting up an event notifier but that seemed like
more fds than were strictly necessary ;-)
> With a note about the use of QLIST_NEXT and the few typo fixed, the
> patch will be good to go.
>
Cool. I'll tidy up and send a v3.
Paul
> Thanks,
>
> --
> Anthony PERARD