[Xen-devel] [PATCH v2 for-4.6 1/2] libxl: fix devd removal path

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

The current flow of the devd helper (in charge of launching hotplug scripts
inside of driver domains) is to wait for the device backend to switch to
state 6  (XenbusStateClosed) and then remove it. This is not correct, since
a domain can reconnect it's PV devices as many times as it wants.

In order to fix this, introduce the following logic: the control domain will
set the "online" backend node to 0 when it wants the driver domain to
disconnect the device, so now the condition applied in devd is that "state"
must be 6 and "online" 0 in order to proceed with the disconnection.

Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
Reported-by: Alex Velazquez <alex.j.velazquez@xxxxxxxxx>
Cc: Alex Velazquez <alex.j.velazquez@xxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
---
Changes since v1:
 - Use strrchr and strcmp in order to check the event path.
 - Remove superfluous LOG in case of failed xenstore write.
 - Remove uneeded strlen checks.
---
 tools/libxl/libxl.c        | 32 ++++++++++++++++++--------------
 tools/libxl/libxl_device.c |  9 ++++-----
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 10d1909..6c4ef6d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4410,32 +4410,36 @@ static void backend_watch_callback(libxl__egc *egc, 
libxl__ev_xswatch *watch,
     libxl__ao *nested_ao = libxl__nested_ao_create(ddomain->ao);
     STATE_AO_GC(nested_ao);
     char *p, *path;
-    const char *sstate;
-    int state, rc, num_devs;
+    const char *sstate, *sonline;
+    int state, online, rc, num_devs;
     libxl__device *dev = NULL;
     libxl__ddomain_device *ddev = NULL;
     libxl__ddomain_guest *dguest = NULL;
     bool free_ao = false;
 
-    /* Check if event_path ends with "state" and truncate it */
-    if (strlen(event_path) < strlen("state"))
-        goto skip;
-
+    /* Check if event_path ends with "state" or "online" and truncate it. */
     path = libxl__strdup(gc, event_path);
-    p = path + strlen(path) - strlen("state") - 1;
-    if (*p != '/')
+    p = strrchr(path, '/');
+    if (p == NULL)
         goto skip;
-    *p = '\0';
-    p++;
-    if (strcmp(p, "state") != 0)
+    if (strcmp(p, "/state") != 0 && strcmp(p, "/online") != 0)
         goto skip;
+    /* Truncate the string so it points to the backend directory. */
+    *p = '\0';
 
-    /* Check if the state is 1 (XenbusStateInitialising) or greater */
-    rc = libxl__xs_read_checked(gc, XBT_NULL, event_path, &sstate);
+    /* Fetch the value of the state and online nodes. */
+    rc = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/state", path),
+                                &sstate);
     if (rc || !sstate)
         goto skip;
     state = atoi(sstate);
 
+    rc = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/online", path),
+                                &sonline);
+    if (rc || !sonline)
+        goto skip;
+    online = atoi(sonline);
+
     dev = libxl__zalloc(NOGC, sizeof(*dev));
     rc = libxl__parse_backend_path(gc, path, dev);
     if (rc)
@@ -4477,7 +4481,7 @@ static void backend_watch_callback(libxl__egc *egc, 
libxl__ev_xswatch *watch,
         rc = add_device(egc, nested_ao, dguest, ddev);
         if (rc > 0)
             free_ao = true;
-    } else if (state == XenbusStateClosed) {
+    } else if (state == XenbusStateClosed && online == 0) {
         /*
          * Removal of an active device, remove it from the list and
          * free it's data structures if they are no longer needed.
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index bee5ed5..85a3662 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -837,16 +837,15 @@ void libxl__initiate_device_remove(libxl__egc *egc,
             goto out;
         }
 
+        rc = libxl__xs_write_checked(gc, t, online_path, "0");
+        if (rc)
+            goto out;
+
         /*
          * Check if device is already in "closed" state, in which case
          * it should not be changed.
          */
          if (state && atoi(state) != XenbusStateClosed) {
-            rc = libxl__xs_write_checked(gc, t, online_path, "0");
-            if (rc) {
-                LOG(ERROR, "unable to write to xenstore path %s", online_path);
-                goto out;
-            }
             rc = libxl__xs_write_checked(gc, t, state_path, GCSPRINTF("%d", 
XenbusStateClosing));
             if (rc) {
                 LOG(ERROR, "unable to write to xenstore path %s", state_path);
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel