[Xen-devel] [PATCH] Fix bug #331

[Dan Smith <danms@xxxxxxxxxx>]

This patch adds the following checks to xend:

- Make sure a block device is not attached to more than one DomU at a
  time
- Make sure a block device is not attached to a DomU if it is mounted
  in Dom0
- Make sure a block device exists before even trying to attach it
- Make sure files specified with "file:" actually exist and have the
  necessary permissions

This makes tests 11_block_attach and 12_block_attach pass.

In order to check if a device is attached to another DomU, I search
the /local/domain/0/device/vbd/*/*/physical-device nodes to see if any
DomU had the device already.  Although this is ugly, I think it's the
best way, so that we don't depend on any xend internal state.

Comments?

Signed-off-by: Dan Smith <danms@xxxxxxxxxx>

# HG changeset patch
# User dan@xxxxxxxxxxxxxxxxxxxxxxxxxxx
# Node ID 643382df7dc8a2562cf1034250244dc6b2bdebd3
# Parent  14d733e5e1d014e302d72fb78df1428ee08e3ce3
Fix bug #331.  Perform some checks before creating block device to make
sure that the device is not in use by another DomU, or mounted in Dom0.
Also, make a small tweak to the 12_block_attach test so that we don't leave
a DomU running with a block device attached.

diff -r 14d733e5e1d0 -r 643382df7dc8 
tools/python/xen/xend/server/DevController.py
--- a/tools/python/xen/xend/server/DevController.py     Wed Nov 23 13:15:35 2005
+++ b/tools/python/xen/xend/server/DevController.py     Wed Nov 23 20:13:07 2005
@@ -75,6 +75,10 @@
 
         @return The ID for the newly created device.
         """
+        (avail, reason) = self.checkAvailability(config)
+        if not avail:
+            raise VmError("Device is unavailable: %s" % reason)
+
         (devid, back, front) = self.getDeviceDetails(config)
         if devid is None:
             return 0
@@ -202,6 +206,18 @@
 
         raise NotImplementedError()
 
+    def checkAvailability(self, config):
+
+        """Determine if the device requested is available for use.
+        Quick checks that can be made immediately.  For example, this
+        may be an existence, permission, or sharing check.  We default
+        to 'available' because this may not make sense for all device
+        types.
+
+        @return (isAvailable, reason)
+        """
+
+        return (True, "OK")
 
     def getDomid(self):
         """Stub to {@link XendDomainInfo.getDomid}, for use by our
diff -r 14d733e5e1d0 -r 643382df7dc8 tools/python/xen/xend/server/blkif.py
--- a/tools/python/xen/xend/server/blkif.py     Wed Nov 23 13:15:35 2005
+++ b/tools/python/xen/xend/server/blkif.py     Wed Nov 23 20:13:07 2005
@@ -19,13 +19,16 @@
 
 import re
 import string
+import os
 
 from xen.util import blkif
 from xen.xend import sxp
 from xen.xend.XendError import VmError
 
 from xen.xend.server.DevController import DevController
+from xen.xend.xenstore.xstransact import xstransact
 
+import xen.xend.XendDomain
 
 class BlkifController(DevController):
     """Block device interface controller. Handles all block devices
@@ -37,6 +40,94 @@
         """
         DevController.__init__(self, vm)
 
+    def __phyCheckAvailability(self, device, mode):
+
+        device = blkif.expand_dev_name(device)
+
+        if not os.access(device, os.F_OK):
+            return (False, "Device %s does not exist" % device)
+
+        if blkif.mount_mode(device) == 'w':
+            # It's write-mounted, so this domain can't have
+            # it read or write
+            return (False, "Device mounted in driver domain")
+
+        return (True, "OK")
+
+    def __fileCheckAvailability(self, file, mode):
+
+        if mode == "r":
+            checkMode = os.R_OK
+        else:
+            checkMode = os.W_OK
+
+        if not os.access(file, os.F_OK):
+            return (False, "File %s does not exist" % file)
+        if not os.access(file, checkMode):
+            return (False, "%s: Permission denied" % file)
+
+        return (True, "OK")
+
+    def __isDeviceAttached(self, backdom, type, device):
+
+        # Utility function
+        # For physical devices, we compare the physical-device node to
+        # the major/minor device number we've been given.  For files
+        # we compare the params node to the filename
+        def isSameDevice(type, value, expected):
+            if type == "phy":
+                return int(value, 16) == int(expected)
+            elif type == "file":
+                return value == expected
+
+        dom0Devs = xstransact.ListRecursive("%s/backend/%s"
+                                            % (backdom.getDomainPath(),
+                                               self.deviceClass))
+
+        if type == "phy":
+            valToCheck = "physical-device"
+            device = blkif.blkdev_name_to_number(device)
+            if not device:
+                return False
+        else:
+            valToCheck = "params"
+
+        # This is ugly
+        for dom, devs in dom0Devs:
+            for devid, vals in devs:
+                for name, value in vals:
+                    if name == valToCheck:
+                        if isSameDevice(type, value, device):
+                            return True
+                        else:
+                            break
+
+        return False
+
+    def checkAvailability(self, config):
+        """@see DevController.checkAvailability"""
+
+        xd = xen.xend.XendDomain.instance()
+        backdom = xd.privilegedDomain()
+        devid = blkif.blkdev_name_to_number(sxp.child_value(config, 'dev'))
+        backpath = self.backendPath(backdom, devid)
+
+        (type, params) = string.split(sxp.child_value(config, 'uname'), ":", 1)
+        mode = sxp.child_value(config, 'mode', 'r')
+
+        # Check to see if the device is in use by any other domains
+        if self.__isDeviceAttached(backdom, type, params):
+            return (False,
+                    "Device %s is already in use by a running domain" % params)
+
+        # Check to see if the device actually exists and that we
+        # can access it
+        if 'phy' == type:
+            return self.__phyCheckAvailability(params, mode)
+        elif 'file' == type:
+            return self.__fileCheckAvailability(params, mode)
+        else:
+            return (True, "OK")
 
     def getDeviceDetails(self, config):
         """@see DevController.getDeviceDetails"""
diff -r 14d733e5e1d0 -r 643382df7dc8 
tools/xm-test/tests/block-create/12_block_attach_shared_domU.py
--- a/tools/xm-test/tests/block-create/12_block_attach_shared_domU.py   Wed Nov 
23 13:15:35 2005
+++ b/tools/xm-test/tests/block-create/12_block_attach_shared_domU.py   Wed Nov 
23 20:13:07 2005
@@ -18,7 +18,7 @@
 
 try:
     dom2.start()
+    dom1.destroy()
     FAIL("Bug #331: Started a DomU with write access to an in-use block 
device")
 except DomainError, e:
-    pass
-    
+    dom1.destroy()

-- 
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms@xxxxxxxxxx

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