Fix xend xenstore handling.

xend can get into a situation where two processes are attempting to
interact with the xenstore socket, with disastrous results. Fix the
two bad users of xstransact, add a big warning, and fix the destructor
so future mistakes will be detected earlier.

Signed-off-by: John Levon <john.levon@sun.com>

3 files changed, 33 insertions, 23 deletions

diff --git a/tools/python/xen/xend/XendDomainInfo.py b/tools/python/xen/xend/XendDomainInfo.py
index 8cc809a135..7a8081c030 100644
--- a/tools/python/xen/xend/XendDomainInfo.py
+++ b/tools/python/xen/xend/XendDomainInfo.py
@@ -1525,18 +1525,19 @@ class XendDomainInfo:
 
         log.debug("Releasing devices")
         t = xstransact("%s/device" % self.dompath)
-        for devclass in XendDevices.valid_devices():
-            for dev in t.list(devclass):
-                try:
-                    log.debug("Removing %s", dev);
-                    self.destroyDevice(devclass, dev, False);
-                except:
-                    # Log and swallow any exceptions in removal --
-                    # there's nothing more we can do.
+        try:
+            for devclass in XendDevices.valid_devices():
+                for dev in t.list(devclass):
+                    try:
+                        log.debug("Removing %s", dev);
+                        self.destroyDevice(devclass, dev, False);
+                    except:
+                        # Log and swallow any exceptions in removal --
+                        # there's nothing more we can do.
                         log.exception("Device release failed: %s; %s; %s",
                                       self.info['name_label'], devclass, dev)
-
-            
+        finally:
+            t.abort()
 
     def getDeviceController(self, name):
         """Get the device controller for this domain, and if it
@@ -1848,16 +1849,18 @@ class XendDomainInfo:
         # build list of phantom devices to be removed after normal devices
         plist = []
         if self.domid is not None:
-            from xen.xend.xenstore.xstransact import xstransact
             t = xstransact("%s/device/vbd" % GetDomainPath(self.domid))
-            for dev in t.list():
-                backend_phantom_vbd = xstransact.Read("%s/device/vbd/%s/phantom_vbd" \
-                                      % (self.dompath, dev))
-                if backend_phantom_vbd is not None:
-                    frontend_phantom_vbd =  xstransact.Read("%s/frontend" \
-                                      % backend_phantom_vbd)
-                    plist.append(backend_phantom_vbd)
-                    plist.append(frontend_phantom_vbd)
+            try:
+                for dev in t.list():
+                    backend_phantom_vbd = xstransact.Read("%s/device/vbd/%s/phantom_vbd" \
+                                          % (self.dompath, dev))
+                    if backend_phantom_vbd is not None:
+                        frontend_phantom_vbd =  xstransact.Read("%s/frontend" \
+                                          % backend_phantom_vbd)
+                        plist.append(backend_phantom_vbd)
+                        plist.append(frontend_phantom_vbd)
+            finally:
+                t.abort()
         return plist
 
     def _cleanup_phantom_devs(self, plist):
diff --git a/tools/python/xen/xend/server/pciif.py b/tools/python/xen/xend/server/pciif.py
index 91f83f20dd..ec875950ad 100644
--- a/tools/python/xen/xend/server/pciif.py
+++ b/tools/python/xen/xend/server/pciif.py
@@ -23,8 +23,6 @@ from xen.xend import sxp
 from xen.xend.XendError import VmError
 from xen.xend.XendLogging import log
 
-from xen.xend.xenstore.xstransact import xstransact
-
 from xen.xend.server.DevController import DevController
 
 import xen.lowlevel.xc
diff --git a/tools/python/xen/xend/xenstore/xstransact.py b/tools/python/xen/xend/xenstore/xstransact.py
index dd9aa98544..af07298067 100644
--- a/tools/python/xen/xend/xenstore/xstransact.py
+++ b/tools/python/xen/xend/xenstore/xstransact.py
@@ -7,8 +7,16 @@
 
 from xen.xend.xenstore.xsutil import xshandle
 
-
 class xstransact:
+    """WARNING: Be very careful if you're instantiating an xstransact object
+       yourself (i.e. not using the capitalized static helpers like .Read().
+       It is essential that you clean up the object in place via
+       t.commit/abort(): GC can happen at any time, including contexts where
+       it's not safe to to use the shared xenstore socket fd. In particular,
+       if xend forks, and GC occurs, we can have two processes trying to
+       use the same xenstore fd, and all hell breaks loose.
+       """
+
 
     def __init__(self, path = ""):
         
@@ -22,8 +30,9 @@ class xstransact:
         self.in_transaction = True
 
     def __del__(self):
+        # see above.
         if self.in_transaction:
-            xshandle().transaction_end(self.transaction, True)
+            raise RuntimeError("ERROR: GC of live transaction")
 
     def commit(self):
         if not self.in_transaction: