[PATCH][xend] Fix/cleanup destoryDevice code path

[PATCH][xend] Fix/cleanup destoryDevice code path

[Jim Fehlig]

[-- Attachment #1: Type: text/plain, Size: 93 bytes --]

More subtle problems with destroyDevice code path in xend.  See attached
patch

Regards,
Jim

[-- Attachment #2: xend_dev_destroy_cleanup.patch --]
[-- Type: text/x-patch, Size: 5447 bytes --]

# HG changeset patch
# User Jim Fehlig <jfehlig@novell.com>
# Date 1186081049 21600
# Node ID 430ae0d3a333ff4d212df7c2313caa03e8f4dd51
# Parent  88bb0d305308a2cab31fd8559a6a2719db1ea55a
Fix/cleanup destroyDevice code path in xend.

When calling destroyDevice code path (e.g. xm block-detach dom devid),
allow specifying an integer device id or a device name such as xvdN or
/dev/xvdN.  Allowing the /dev/xvdN form is useful when detaching devices
from dom0.  Bootloaders may do this to unmount a disk previously
mounted in dom0.

Move examination of device ID format into the DevController, permitting
device controllers to determine a valid device ID instead of higher
level code.

Signed-off-by: Jim Fehlig <jfehlig@novell.com>

diff -r 88bb0d305308 -r 430ae0d3a333 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Wed Aug 01 15:47:54 2007 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Thu Aug 02 12:57:29 2007 -0600
@@ -559,18 +559,8 @@ class XendDomainInfo:
             self.getDeviceController(devclass).waitForDevices()
 
     def destroyDevice(self, deviceClass, devid, force = False):
-        try:
-            dev = int(devid)
-        except ValueError:
-            # devid is not a number but a string containing either device
-            # name (e.g. xvda) or device_type/device_id (e.g. vbd/51728)
-            dev = type(devid) is str and devid.split('/')[-1] or None
-            if dev == None:
-                log.debug("Could not find the device %s", devid)
-                return None
-
-        log.debug("dev = %s", dev)
-        return self.getDeviceController(deviceClass).destroyDevice(dev, force)
+        log.debug("dev = %s", devid)
+        return self.getDeviceController(deviceClass).destroyDevice(devid, force)
 
     def getDeviceSxprs(self, deviceClass):
         if self._stateGet() in (DOM_STATE_RUNNING, DOM_STATE_PAUSED):
diff -r 88bb0d305308 -r 430ae0d3a333 tools/python/xen/xend/server/DevController.py
--- a/tools/python/xen/xend/server/DevController.py	Wed Aug 01 15:47:54 2007 +0100
+++ b/tools/python/xen/xend/server/DevController.py	Thu Aug 02 12:57:29 2007 -0600
@@ -203,27 +203,32 @@ class DevController:
 
         The implementation here simply deletes the appropriate paths from the
         store.  This may be overridden by subclasses who need to perform other
-        tasks on destruction.  Further, the implementation here can only
-        accept integer device IDs, or values that can be converted to
-        integers.  Subclasses may accept other values and convert them to
-        integers before passing them here.
-        """
-
-        devid = int(devid)
+        tasks on destruction. The implementation here accepts integer device
+        IDs or paths containg integer deviceIDs, e.g. vfb/0.  Subclasses may
+        accept other values and convert them to integers before passing them
+        here.
+        """
+
+        try:
+            dev = int(devid)
+        except ValueError:
+            # Does devid contain devicetype/deviceid?
+            # Propogate exception if unable to find an integer devid
+            dev = int(type(devid) is str and devid.split('/')[-1] or None)
 
         # Modify online status /before/ updating state (latter is watched by
         # drivers, so this ordering avoids a race).
-        self.writeBackend(devid, 'online', "0")
-        self.writeBackend(devid, 'state', str(xenbusState['Closing']))
+        self.writeBackend(dev, 'online', "0")
+        self.writeBackend(dev, 'state', str(xenbusState['Closing']))
 
         if force:
-            frontpath = self.frontendPath(devid)
+            frontpath = self.frontendPath(dev)
             backpath = xstransact.Read(frontpath, "backend")
             if backpath:
                 xstransact.Remove(backpath)
             xstransact.Remove(frontpath)
 
-        self.vm._removeVm("device/%s/%d" % (self.deviceClass, devid))
+        self.vm._removeVm("device/%s/%d" % (self.deviceClass, dev))
 
     def configurations(self):
         return map(self.configuration, self.deviceIDs())
diff -r 88bb0d305308 -r 430ae0d3a333 tools/python/xen/xend/server/blkif.py
--- a/tools/python/xen/xend/server/blkif.py	Wed Aug 01 15:47:54 2007 +0100
+++ b/tools/python/xen/xend/server/blkif.py	Thu Aug 02 12:57:29 2007 -0600
@@ -149,13 +149,16 @@ class BlkifController(DevController):
     def destroyDevice(self, devid, force):
         """@see DevController.destroyDevice"""
 
-        # If we are given a device name, then look up the device ID from it,
-        # and destroy that ID instead.  If what we are given is an integer,
-        # then assume it's a device ID and pass it straight through to our
-        # superclass's method.
-
+        # vbd device IDs can be either string or integer.  Further, the
+        # following string values are possible:
+        #    - devicetype/deviceid (vbd/51728)
+        #    - devicetype/devicename (/dev/xvdb)
+        #    - devicename (xvdb)
+        # Let our superclass handle integer or devicetype/deviceid forms.
+        # If we are given a device name form, then look up the device ID
+        # from it, and destroy that ID instead.
         try:
-            DevController.destroyDevice(self, int(devid), force)
+            DevController.destroyDevice(self, devid, force)
         except ValueError:
             devid_end = type(devid) is str and devid.split('/')[-1] or None
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH][xend] Fix/cleanup destoryDevice code path

[Mats Petersson]

Jim

Can you check if this removes the device nodes within xenstore when 
doing xm save [1] - as I modified this section of code to fix up a 
problem with just that not so long ago. Unfortunately, I'm not able 
to test it myself, as I'm "without a xen-capable machine at the moment".

I have no particular reason to believe it DOESN'T do that, I'm just 
asking to make sure that it's destroyed properly, because I found 
that leaving lots of device info in xenstore bloats the database in 
xenstore, and eventually Dom0 runs out of physical memory.... :-(

The easiest way to do this is to repeatedly save and restore a 
domain, whilst whatching the size of xenstore, e.g. "xenstore-ls | wc"

--
Mats

At 20:01 02/08/2007, Jim Fehlig wrote:
>More subtle problems with destroyDevice code path in xend.  See attached
>patch
>
>Regards,
>Jim
>
># HG changeset patch
># User Jim Fehlig <jfehlig@novell.com>
># Date 1186081049 21600
># Node ID 430ae0d3a333ff4d212df7c2313caa03e8f4dd51
># Parent  88bb0d305308a2cab31fd8559a6a2719db1ea55a
>Fix/cleanup destroyDevice code path in xend.
>
>When calling destroyDevice code path (e.g. xm block-detach dom devid),
>allow specifying an integer device id or a device name such as xvdN or
>/dev/xvdN.  Allowing the /dev/xvdN form is useful when detaching devices
>from dom0.  Bootloaders may do this to unmount a disk previously
>mounted in dom0.
>
>Move examination of device ID format into the DevController, permitting
>device controllers to determine a valid device ID instead of higher
>level code.
>
>Signed-off-by: Jim Fehlig <jfehlig@novell.com>
>
>diff -r 88bb0d305308 -r 430ae0d3a333 tools/python/xen/xend/XendDomainInfo.py
>--- a/tools/python/xen/xend/XendDomainInfo.py   Wed Aug 01 15:47:54 2007 +0100
>+++ b/tools/python/xen/xend/XendDomainInfo.py   Thu Aug 02 12:57:29 2007 -0600
>@@ -559,18 +559,8 @@ class XendDomainInfo:
>              self.getDeviceController(devclass).waitForDevices()
>
>      def destroyDevice(self, deviceClass, devid, force = False):
>-        try:
>-            dev = int(devid)
>-        except ValueError:
>-            # devid is not a number but a string containing either device
>-            # name (e.g. xvda) or device_type/device_id (e.g. vbd/51728)
>-            dev = type(devid) is str and devid.split('/')[-1] or None
>-            if dev == None:
>-                log.debug("Could not find the device %s", devid)
>-                return None
>-
>-        log.debug("dev = %s", dev)
>-        return 
>self.getDeviceController(deviceClass).destroyDevice(dev, force)
>+        log.debug("dev = %s", devid)
>+        return 
>self.getDeviceController(deviceClass).destroyDevice(devid, force)
>
>      def getDeviceSxprs(self, deviceClass):
>          if self._stateGet() in (DOM_STATE_RUNNING, DOM_STATE_PAUSED):
>diff -r 88bb0d305308 -r 430ae0d3a333 
>tools/python/xen/xend/server/DevController.py
>--- a/tools/python/xen/xend/server/DevController.py     Wed Aug 01 
>15:47:54 2007 +0100
>+++ b/tools/python/xen/xend/server/DevController.py     Thu Aug 02 
>12:57:29 2007 -0600
>@@ -203,27 +203,32 @@ class DevController:
>
>          The implementation here simply deletes the appropriate 
> paths from the
>          store.  This may be overridden by subclasses who need to 
> perform other
>-        tasks on destruction.  Further, the implementation here can only
>-        accept integer device IDs, or values that can be converted to
>-        integers.  Subclasses may accept other values and convert them to
>-        integers before passing them here.
>-        """
>-
>-        devid = int(devid)
>+        tasks on destruction. The implementation here accepts integer device
>+        IDs or paths containg integer deviceIDs, e.g. vfb/0.  Subclasses may
>+        accept other values and convert them to integers before passing them
>+        here.
>+        """
>+
>+        try:
>+            dev = int(devid)
>+        except ValueError:
>+            # Does devid contain devicetype/deviceid?
>+            # Propogate exception if unable to find an integer devid
>+            dev = int(type(devid) is str and devid.split('/')[-1] or None)
>
>          # Modify online status /before/ updating state (latter is watched by
>          # drivers, so this ordering avoids a race).
>-        self.writeBackend(devid, 'online', "0")
>-        self.writeBackend(devid, 'state', str(xenbusState['Closing']))
>+        self.writeBackend(dev, 'online', "0")
>+        self.writeBackend(dev, 'state', str(xenbusState['Closing']))
>
>          if force:
>-            frontpath = self.frontendPath(devid)
>+            frontpath = self.frontendPath(dev)
>              backpath = xstransact.Read(frontpath, "backend")
>              if backpath:
>                  xstransact.Remove(backpath)
>              xstransact.Remove(frontpath)
>
>-        self.vm._removeVm("device/%s/%d" % (self.deviceClass, devid))
>+        self.vm._removeVm("device/%s/%d" % (self.deviceClass, dev))
>
>      def configurations(self):
>          return map(self.configuration, self.deviceIDs())
>diff -r 88bb0d305308 -r 430ae0d3a333 tools/python/xen/xend/server/blkif.py
>--- a/tools/python/xen/xend/server/blkif.py     Wed Aug 01 15:47:54 2007 +0100
>+++ b/tools/python/xen/xend/server/blkif.py     Thu Aug 02 12:57:29 2007 -0600
>@@ -149,13 +149,16 @@ class BlkifController(DevController):
>      def destroyDevice(self, devid, force):
>          """@see DevController.destroyDevice"""
>
>-        # If we are given a device name, then look up the device ID from it,
>-        # and destroy that ID instead.  If what we are given is an integer,
>-        # then assume it's a device ID and pass it straight through to our
>-        # superclass's method.
>-
>+        # vbd device IDs can be either string or integer.  Further, the
>+        # following string values are possible:
>+        #    - devicetype/deviceid (vbd/51728)
>+        #    - devicetype/devicename (/dev/xvdb)
>+        #    - devicename (xvdb)
>+        # Let our superclass handle integer or devicetype/deviceid forms.
>+        # If we are given a device name form, then look up the device ID
>+        # from it, and destroy that ID instead.
>          try:
>-            DevController.destroyDevice(self, int(devid), force)
>+            DevController.destroyDevice(self, devid, force)
>          except ValueError:
>              devid_end = type(devid) is str and devid.split('/')[-1] or None
>
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel

Re: [PATCH][xend] Fix/cleanup destoryDevice code path

[Jim Fehlig]

Mats Petersson wrote:
> Jim
>
> Can you check if this removes the device nodes within xenstore when
> doing xm save [1] - as I modified this section of code to fix up a
> problem with just that not so long ago. Unfortunately, I'm not able to
> test it myself, as I'm "without a xen-capable machine at the moment".

Yes, everything gets nuked from xenstore as expected on save.

> I have no particular reason to believe it DOESN'T do that, I'm just
> asking to make sure that it's destroyed properly, because I found that
> leaving lots of device info in xenstore bloats the database in
> xenstore, and eventually Dom0 runs out of physical memory.... :-(

Yes, I recall this behavior.  I recently received a large database file
(~34M) from a 3.0.4-based system that includes your patches.  This
system had well over 25000 vms started/[stopped|destroyed|failed] and
the database had *many* /local/domain/<vmid>/device/console/0 entries. 
So some types of devices (console in particular) are still orphaned
under some yet undetermined vm lifeclycle event.  Trying to reproduce ...

BTW, any ideas on how to read a xenstore tdb file provided via a bug
report?  :-)

Regards,
Jim