[Xen-devel] [PATCH V2] libxl: fix cleanup of tap devices in libxl__device_destroy

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

# HG changeset patch
# User Ian Campbell <ian.campbell@xxxxxxxxxx>
# Date 1343985174 -3600
# Node ID 2c21d5c75dcbdf52987fbc47e4c8181b9236bca3
# Parent  f9d3acc755bd1b05f5d2c6a592a760953f0e83bd
libxl: fix cleanup of tap devices in libxl__device_destroy

We pass be_path to tapdisk_destroy but we've already deleted it so it
fails to read tapdisk-params. However it appears that we need to
destroy the tap device after tearing down xenstore, to avoid the leak
reported by Greg Wettstein in
<201207312141.q6VLfJje012656@xxxxxxxxxxxxxxxxx>.

So read the tapdisk-params in the cleanup transaction, before the
remove, and pass that down to destroy_tapdisk instead. tapdisk-params
may of course be NULL if the device isn't a tap device.

There is no need to tear down the tap device from
libxl__initiate_device_remove since this ultimately calls
libxl__device_destroy.

Propagate and log errors from libxl__device_destroy_tapdisk.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
---
v2:
  - use libxl__xs_read_checked. libxl__device_destroy_tapdisk
    therefore takes a const char * params and dups itself a writeable
    copy.
  - prerequisites are now in tree.

diff -r f9d3acc755bd -r 2c21d5c75dcb tools/libxl/libxl_blktap2.c
--- a/tools/libxl/libxl_blktap2.c       Fri Aug 03 10:12:33 2012 +0100
+++ b/tools/libxl/libxl_blktap2.c       Fri Aug 03 10:12:54 2012 +0100
@@ -51,28 +51,37 @@ char *libxl__blktap_devpath(libxl__gc *g
 }
 
 
-void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path)
+int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
 {
-    char *path, *params, *type, *disk;
+    char *type, *disk;
     int err;
     tap_list_t tap;
 
-    path = libxl__sprintf(gc, "%s/tapdisk-params", be_path);
-    if (!path) return;
+    type = libxl__strdup(gc, params);
 
-    params = libxl__xs_read(gc, XBT_NULL, path);
-    if (!params) return;
-
-    type = params;
-    disk = strchr(params, ':');
-    if (!disk) return;
+    disk = strchr(type, ':');
+    if (!disk) {
+        LOG(ERROR, "Unable to parse params %s", params);
+        return ERROR_INVAL;
+    }
 
     *disk++ = '\0';
 
     err = tap_ctl_find(type, disk, &tap);
-    if (err < 0) return;
+    if (err < 0) {
+        /* returns -errno */
+        LOGEV(ERROR, -err, "Unable to find type %s disk %s", type, disk);
+        return ERROR_FAIL;
+    }
 
-    tap_ctl_destroy(tap.id, tap.minor);
+    err = tap_ctl_destroy(tap.id, tap.minor);
+    if (err < 0) {
+        LOGEV(ERROR, -err, "Failed to destroy tap device id %d minor %d",
+              tap.id, tap.minor);
+        return ERROR_FAIL;
+    }
+
+    return 0;
 }
 
 /*
diff -r f9d3acc755bd -r 2c21d5c75dcb tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c        Fri Aug 03 10:12:33 2012 +0100
+++ b/tools/libxl/libxl_device.c        Fri Aug 03 10:12:54 2012 +0100
@@ -522,8 +522,10 @@ DEFINE_DEVICES_ADD(nic)
 
 int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
 {
-    char *be_path = libxl__device_backend_path(gc, dev);
+    const char *be_path = libxl__device_backend_path(gc, dev);
     const char *fe_path = libxl__device_frontend_path(gc, dev);
+    const char *tapdisk_path = GCSPRINTF("%s/%s", be_path, "tapdisk-params");
+    const char *tapdisk_params;
     xs_transaction_t t = 0;
     int rc;
 
@@ -531,6 +533,10 @@ int libxl__device_destroy(libxl__gc *gc,
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
 
+        /* May not exist if this is not a tap device */
+        rc = libxl__xs_read_checked(gc, t, tapdisk_path, &tapdisk_params);
+        if (rc) goto out;
+
         libxl__xs_path_cleanup(gc, t, fe_path);
         libxl__xs_path_cleanup(gc, t, be_path);
 
@@ -539,7 +545,8 @@ int libxl__device_destroy(libxl__gc *gc,
         if (rc < 0) goto out;
     }
 
-    libxl__device_destroy_tapdisk(gc, be_path);
+    if (tapdisk_params)
+        rc = libxl__device_destroy_tapdisk(gc, tapdisk_params);
 
 out:
     libxl__xs_transaction_abort(gc, &t);
@@ -790,8 +797,6 @@ void libxl__initiate_device_remove(libxl
         if (rc < 0) goto out;
     }
 
-    libxl__device_destroy_tapdisk(gc, be_path);
-
     rc = libxl__ev_devstate_wait(gc, &aodev->backend_ds,
                                  device_backend_callback,
                                  state_path, XenbusStateClosed,
diff -r f9d3acc755bd -r 2c21d5c75dcb tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h      Fri Aug 03 10:12:33 2012 +0100
+++ b/tools/libxl/libxl_internal.h      Fri Aug 03 10:12:54 2012 +0100
@@ -1344,8 +1344,9 @@ _hidden char *libxl__blktap_devpath(libx
 /* libxl__device_destroy_tapdisk:
  *   Destroys any tapdisk process associated with the backend represented
  *   by be_path.
+ *   Always logs on failure.
  */
-_hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
+_hidden int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params);
 
 _hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
                                    libxl_device_disk *disk,
diff -r f9d3acc755bd -r 2c21d5c75dcb tools/libxl/libxl_noblktap2.c
--- a/tools/libxl/libxl_noblktap2.c     Fri Aug 03 10:12:33 2012 +0100
+++ b/tools/libxl/libxl_noblktap2.c     Fri Aug 03 10:12:54 2012 +0100
@@ -28,8 +28,9 @@ char *libxl__blktap_devpath(libxl__gc *g
     return NULL;
 }
 
-void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path)
+int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
 {
+    return 0;
 }
 
 /*

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