From: Anthony PERARD <anthony.perard@citrix.com>
To: Paul Durrant <paul.durrant@citrix.com>
Cc: xen-devel@lists.xenproject.org,
Stefano Stabellini <sstabellini@kernel.org>,
qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler
Date: Thu, 12 Sep 2019 16:04:17 +0100 [thread overview]
Message-ID: <20190912150417.GD1308@perard.uk.xensource.com> (raw)
In-Reply-To: <20190912121646.29148-4-paul.durrant@citrix.com>
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@citrix.com>
> ---
>
> 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.)
> + 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.
With a note about the use of QLIST_NEXT and the few typo fixed, the
patch will be good to go.
Thanks,
--
Anthony PERARD
next prev parent reply other threads:[~2019-09-12 15:05 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-12 12:16 [Qemu-devel] [PATCH v2 0/3] xen: fix a potential crash in xen-bus Paul Durrant
2019-09-12 12:16 ` [Qemu-devel] [PATCH v2 1/3] xen / notify: introduce a new XenWatchList abstraction Paul Durrant
2019-09-12 12:16 ` [Qemu-devel] [PATCH v2 2/3] xen: introduce separate XenWatchList for XenDevice objects Paul Durrant
2019-09-12 12:16 ` [Qemu-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler Paul Durrant
2019-09-12 15:04 ` Anthony PERARD [this message]
2019-09-12 15:15 ` Paul Durrant
2019-09-13 8:03 ` Paul Durrant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190912150417.GD1308@perard.uk.xensource.com \
--to=anthony.perard@citrix.com \
--cc=paul.durrant@citrix.com \
--cc=qemu-devel@nongnu.org \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).