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

2019-09-11 Thread Paul Durrant 
> -Original Message-
> From: Paul Durrant 
> Sent: 11 September 2019 15:36
> To: qemu-devel@nongnu.org; xen-de...@lists.xenproject.org
> Cc: Paul Durrant ; Stefano Stabellini 
> ; Anthony
> Perard 
> Subject: [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler
> 
> Cleaning up offine XenDevice objects directly in
> 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
> 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 
> ---
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> ---
>  hw/xen/trace-events  |  2 +
>  hw/xen/xen-bus.c | 91 +---
>  include/hw/xen/xen-bus.h |  2 +
>  3 files changed, 70 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/xen/trace-events b/hw/xen/trace-events
> index 80ce3dafad..e6885bc751 100644
> --- a/hw/xen/trace-events
> +++ b/hw/xen/trace-events
> @@ -17,8 +17,10 @@ xen_domid_restrict(int err) "err: %u"
>  xen_bus_realize(void) ""
>  xen_bus_unrealize(void) ""
>  xen_bus_enumerate(void) ""
> +xen_bus_cleanup(void) ""
>  xen_bus_type_enumerate(const char *type) "type: %s"
>  xen_bus_backend_create(const char *type, const char *path) "type: %s path: 
> %s"
> +xen_bus_device_cleanup(const char *type, char *name) "type: %s name: %s"
>  xen_bus_add_watch(const char *node, const char *key) "node: %s key: %s"
>  xen_bus_remove_watch(const char *node, const char *key) "node: %s key: %s"
>  xen_device_realize(const char *type, char *name) "type: %s name: %s"
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index 810a4e2df3..055beb7260 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -340,13 +340,18 @@ static void xen_bus_type_enumerate(XenBus *xenbus, 
> const char *type)
>  for (i = 0; i < n; i++) {
>  char *backend_path = g_strdup_printf("%s/%s", domain_path,
>   backend[i]);
> -enum xenbus_state backend_state;
> +enum xenbus_state state;
> +unsigned int online;
> 
>  if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "state",
> -  NULL, "%u", &backend_state) != 1)
> -backend_state = XenbusStateUnknown;
> +  NULL, "%u", &state) != 1)
> +state = XenbusStateUnknown;
> 
> -if (backend_state == XenbusStateInitialising) {
> +if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "online",
> +  NULL, "%u", &online) != 1)
> +online = 0;
> +
> +if (online && state == XenbusStateInitialising) {
>  Error *local_err = NULL;
> 
>  xen_bus_backend_create(xenbus, type, backend[i], backend_path,
> @@ -365,9 +370,8 @@ out:
>  g_free(domain_path);
>  }
> 
> -static void xen_bus_enumerate(void *opaque)
> +static void xen_bus_enumerate(XenBus *xenbus)
>  {
> -XenBus *xenbus = opaque;
>  char **type;
>  unsigned int i, n;
> 
> @@ -385,6 +389,44 @@ static void xen_bus_enumerate(void *opaque)
>  free(type);
>  }
> 
> +static void xen_bus_device_cleanup(XenDevice *xendev)
> +{
> +const char *type = object_get_typename(OBJECT(xendev));
> +Error *local_err = NULL;
> +
> +trace_xen_bus_device_cleanup(type, xendev->name);
> +
> +g_assert(!xendev->backend_online);
> +
> +if (!xen_backend_try_device_destroy(xendev, &local_err)) {
> +object_unparent(OBJECT(xendev));
> +}
> +
> +if (local_err) {
> +error_report_err(local_err);
> +}
> +}
>

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

2019-09-11 Thread Paul Durrant 
Cleaning up offine XenDevice objects directly in
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
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 
---
Cc: Stefano Stabellini 
Cc: Anthony Perard 
---
 hw/xen/trace-events  |  2 +
 hw/xen/xen-bus.c | 91 +---
 include/hw/xen/xen-bus.h |  2 +
 3 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 80ce3dafad..e6885bc751 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -17,8 +17,10 @@ xen_domid_restrict(int err) "err: %u"
 xen_bus_realize(void) ""
 xen_bus_unrealize(void) ""
 xen_bus_enumerate(void) ""
+xen_bus_cleanup(void) ""
 xen_bus_type_enumerate(const char *type) "type: %s"
 xen_bus_backend_create(const char *type, const char *path) "type: %s path: %s"
+xen_bus_device_cleanup(const char *type, char *name) "type: %s name: %s"
 xen_bus_add_watch(const char *node, const char *key) "node: %s key: %s"
 xen_bus_remove_watch(const char *node, const char *key) "node: %s key: %s"
 xen_device_realize(const char *type, char *name) "type: %s name: %s"
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 810a4e2df3..055beb7260 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -340,13 +340,18 @@ static void xen_bus_type_enumerate(XenBus *xenbus, const 
char *type)
 for (i = 0; i < n; i++) {
 char *backend_path = g_strdup_printf("%s/%s", domain_path,
  backend[i]);
-enum xenbus_state backend_state;
+enum xenbus_state state;
+unsigned int online;
 
 if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "state",
-  NULL, "%u", &backend_state) != 1)
-backend_state = XenbusStateUnknown;
+  NULL, "%u", &state) != 1)
+state = XenbusStateUnknown;
 
-if (backend_state == XenbusStateInitialising) {
+if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "online",
+  NULL, "%u", &online) != 1)
+online = 0;
+
+if (online && state == XenbusStateInitialising) {
 Error *local_err = NULL;
 
 xen_bus_backend_create(xenbus, type, backend[i], backend_path,
@@ -365,9 +370,8 @@ out:
 g_free(domain_path);
 }
 
-static void xen_bus_enumerate(void *opaque)
+static void xen_bus_enumerate(XenBus *xenbus)
 {
-XenBus *xenbus = opaque;
 char **type;
 unsigned int i, n;
 
@@ -385,6 +389,44 @@ static void xen_bus_enumerate(void *opaque)
 free(type);
 }
 
+static void xen_bus_device_cleanup(XenDevice *xendev)
+{
+const char *type = object_get_typename(OBJECT(xendev));
+Error *local_err = NULL;
+
+trace_xen_bus_device_cleanup(type, xendev->name);
+
+g_assert(!xendev->backend_online);
+
+if (!xen_backend_try_device_destroy(xendev, &local_err)) {
+object_unparent(OBJECT(xendev));
+}
+
+if (local_err) {
+error_report_err(local_err);
+}
+}
+
+static void xen_bus_cleanup(XenBus *xenbus)
+{
+XenDevice *xendev, *next;
+
+trace_xen_bus_cleanup();
+
+QLIST_FOREACH_SAFE(xendev, &xenbus->offline_devices, list, next) {
+QLIST_REMOVE(xendev, list);
+xen_bus_device_cleanup(xendev);
+}
+}
+
+static void xen_bus_backend_changed(void *opaque)
+{
+XenBus *xenbus = opaque;
+
+xen_bus_enumerate(xenbus);
+xen_bus_cleanup(xenbus);
+}
+
 static void xen_bus_unrealize(BusState *bus, Error **errp)
 {
 XenBus *xenbus =