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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: fix cleanup of tap devices 
in libxl__device_destroy"):
> On Thu, 2012-08-02 at 15:42 +0100, Ian Jackson wrote:
> > In general in this destroy path we should consider whether, on
> > failure, we should abandon the cleanup or note the error and carry on.
> 
> Since this is a destroy operation note it and carry on I think, so as to
> clean up as much as we are able.

Right.  Good then I guess it is OK.  (We do risk leaking some stuff in
xenstore which you'd have to use low-level tools to remove but that's
probably acceptable if things are so bad you can't remove stuff from
xenstore.)

> BTW, is there a libxl__xs_transaction_abort missing in this function
> too?

Yes.  I have edited my patch to fix this.  (See below; I will repost
it with v5 of my series, later today I think.)

Ian.

From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Subject: [PATCH] libxl: unify libxl__device_destroy and device_hotplug_done

device_hotplug_done contains an open-coded but improved version of
libxl__device_destroy.  So move the contents of device_hotplug_done
into libxl__device_destroy, deleting the old code, and replace it at
its old location with a function call.

Add the missing call to libxl__xs_transaction_abort (which was present
in neither version and technically speaking is always a no-op with
this code as it stands at the moment because no-one does "goto out"
other than after libxl__xs_transaction_start or _commit).

Also fix the error handling: the rc from the destroy should be
propagated into the aodev.

Reported-by: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

-
Changes in v5 of series:
 * Also add missing xs abort.

---
 tools/libxl/libxl_device.c |   36 +++++++++++++-----------------------
 1 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index da0c3ea..95b169e 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -513,22 +513,24 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device 
*dev)
     char *be_path = libxl__device_backend_path(gc, dev);
     char *fe_path = libxl__device_frontend_path(gc, dev);
     xs_transaction_t t = 0;
-    int rc = 0;
+    int rc;
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
 
-    do {
-        t = xs_transaction_start(CTX->xsh);
         libxl__xs_path_cleanup(gc, t, fe_path);
         libxl__xs_path_cleanup(gc, t, be_path);
-        rc = !xs_transaction_end(CTX->xsh, t, 0);
-    } while (rc && errno == EAGAIN);
-    if (rc) {
-        LOGE(ERROR, "unable to finish transaction");
-        goto out;
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) goto out;
     }
 
     libxl__device_destroy_tapdisk(gc, be_path);
 
 out:
+    libxl__xs_transaction_abort(gc, &t);
     return rc;
 }
 
@@ -993,29 +995,17 @@ error:
 static void device_hotplug_done(libxl__egc *egc, libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
-    char *be_path = libxl__device_backend_path(gc, aodev->dev);
-    char *fe_path = libxl__device_frontend_path(gc, aodev->dev);
-    xs_transaction_t t = 0;
     int rc;
 
     device_hotplug_clean(gc, aodev);
 
     /* Clean xenstore if it's a disconnection */
     if (aodev->action == DEVICE_DISCONNECT) {
-        for (;;) {
-            rc = libxl__xs_transaction_start(gc, &t);
-            if (rc) goto out;
-
-            libxl__xs_path_cleanup(gc, t, fe_path);
-            libxl__xs_path_cleanup(gc, t, be_path);
-
-            rc = libxl__xs_transaction_commit(gc, &t);
-            if (!rc) break;
-            if (rc < 0) goto out;
-        }
+        rc = libxl__device_destroy(gc, aodev->dev);
+        if (!aodev->rc)
+            aodev->rc = rc;
     }
 
-out:
     aodev->callback(egc, aodev);
     return;
 }
-- 
tg: (7fc019d..) t/xen/xl.device-destroy-unify (depends on: t/xen/gitignore)

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