[rhel6-branch 1/2] edd: use pci_dev as an additional clue for disk boot priority (#621175)

[Ales Kozumplik]

In general there is not always mbrsig on a disk (new device), so we can't
always succeed in creating the disk -> bios id mapping. On the other hand,
the pci_dev symlink in edd does not give us complete information about the
device. So we have to consider both in compareDisks().
---
 kickstart.py              |    4 +-
 storage/__init__.py       |   18 ++++--
 storage/devicelibs/edd.py |  167 ++++++++++++++++++++++++++++++---------------
 3 files changed, 126 insertions(+), 63 deletions(-)

diff --git a/kickstart.py b/kickstart.py
index b606ab8..67b1480 100644
--- a/kickstart.py
+++ b/kickstart.py
@@ -665,8 +665,8 @@ class PartitionData(commands.partition.F12_PartData):
         storage.doAutoPart = False

if self.onbiosdisk != "":
-            for (disk, biosdisk) in storage.eddDict.iteritems():
-                if str(biosdisk) == self.onbiosdisk:
+            for (disk, eddinfo) in storage.eddDict.iteritems():
+                if str(eddinfo.biosdisk) == self.onbiosdisk:
                     self.disk = disk
                     break

diff --git a/storage/__init__.py b/storage/__init__.py
index 464eebf..09dab1f 100644
--- a/storage/__init__.py
+++ b/storage/__init__.py
@@ -1180,18 +1180,26 @@ class Storage(object):
         return self.fsset.rootDevice

def compareDisks(self, first, second):
-        if self.eddDict.has_key(first) and self.eddDict.has_key(second):
-            one = self.eddDict[first]
-            two = self.eddDict[second]
+        # if exactly one of the devices has a pcidev, prefer it
+        if bool(self.eddDict[first].pcidev) ^ bool(self.eddDict[second].pcidev):
+            if self.eddDict[first].pcidev:
+                return -1
+            if self.eddDict[second].pcidev:
+                return 1
+
+        # next, compare the biosdev numbers
+        if self.eddDict[first].biosdev and self.eddDict[second].biosdev:
+            one = self.eddDict[first].biosdev
+            two = self.eddDict[second].biosdev
             if (one < two):
                 return -1
             elif (one > two):
                 return 1

# if one is in the BIOS and the other not prefer the one in the BIOS
-        if self.eddDict.has_key(first):
+        if self.eddDict[first].biosdev:
             return -1
-        if self.eddDict.has_key(second):
+        if self.eddDict[second].biosdev:
             return 1

if first.startswith("hd"):
diff --git a/storage/devicelibs/edd.py b/storage/devicelibs/edd.py
index da03914..db2ba57 100644
--- a/storage/devicelibs/edd.py
+++ b/storage/devicelibs/edd.py
@@ -21,77 +21,132 @@
 #

import os
+import os.path
 import struct

import logging
 log = logging.getLogger("storage")

+class EddInfo:
+    def __init__(self, biosdev):
+        self.biosdev = biosdev
+        self.pcidev = None
+
 def get_edd_dict(devices):
-    """Given an array of devices return a dict with the BIOS ID for them."""
+    """
+    Given an array of devices return a mapping from their devices names to their
+    EddInfo.
+
+    CAVEATS: It is important to note that the current implementation of the edd
+    kernel module is incomplete and does not give us a reliable method about
+    discovering the complete device <-> BIOS device mapping. Checking the MBR
+    signature works in most cases but will always fail e.g. on new disks that do
+    not contain an MBR yet. On the other hand, the pcidev entry which is
+    guaranteed to exist is shared across all disks connected through the same
+    PCI device and so does not provide a one-to-one relation.
+    """
+
+    # initialize the dict with blank info
     edd_dict = {}
+    for dev in devices:
+        edd_dict[dev.name] = EddInfo(None)

for biosdev in range(80, 80 + 15):
+
         sysfspath = "/sys/firmware/edd/int13_dev%d" % biosdev
         if not os.path.exists(sysfspath):
             break # We are done

-        sysfspath = "/sys/firmware/edd/int13_dev%d/mbr_signature" % biosdev
-        if not os.path.exists(sysfspath):
-            log.warning("No mbrsig for biosdev: %d" % biosdev)
-            continue
+        dev = _find_device_from_mbrsig(devices, biosdev)
+        if dev:
+            edd_dict[dev].biosdev = biosdev
+
+        (pcidev, devs_for_pcidev) = _devices_for_pcidev(devices, biosdev)
+        for d in devs_for_pcidev:
+            edd_dict[d.name].pcidev = pcidev
+
+    _dump_dict(edd_dict)
+    return edd_dict
+
+def _dump_dict(edd_dict):
+    log.debug("edd:generated dict:")
+    for key in edd_dict:
+        log.debug("%s: biosdev: %s, pcidev: %s" %
+                  (key, edd_dict[key].biosdev, edd_dict[key].pcidev))
+    log.debug("edd:end of generated dict dump")
+
+def _find_device_from_mbrsig(devices, biosdev):
+    sysfspath = "/sys/firmware/edd/int13_dev%d/mbr_signature" % biosdev
+    if not os.path.exists(sysfspath):
+        log.info("edd: no mbrsig for biosdev: %d" % biosdev)
+        return None
+
+    try:
+        file = open(sysfspath, "r")
+        eddsig = file.read()
+        file.close()
+    except (IOError, OSError) as e:
+        log.info("edd: error reading EDD mbrsig for %d: %s" %
+                    (biosdev, str(e)))
+        return None

+    sysfspath = "/sys/firmware/edd/int13_dev%d/sectors" % biosdev
+    try:
+        file = open(sysfspath, "r")
+        eddsize = file.read()
+        file.close()
+    except (IOError, OSError) as e:
+        eddsize = None
+
+    found = []
+    for dev in devices:
         try:
-            file = open(sysfspath, "r")
-            eddsig = file.read()
-            file.close()
-        except (IOError, OSError) as e:
-            log.warning("Error reading EDD mbrsig for %d: %s" %
-                        (biosdev, str(e)))
+            fd = os.open(dev.path, os.O_RDONLY)
+            os.lseek(fd, 440, 0)
+            mbrsig = struct.unpack('I', os.read(fd, 4))
+            os.close(fd)
+        except OSError as e:
+            log.info("edd: error reading mbrsig from disk %s: %s" %
+                        (dev.name, str(e)))
             continue

-        sysfspath = "/sys/firmware/edd/int13_dev%d/sectors" % biosdev
-        try:
-            file = open(sysfspath, "r")
-            eddsize = file.read()
-            file.close()
-        except (IOError, OSError) as e:
-            eddsize = None
-
-        found = []
-        for dev in devices:
-            try:
-                fd = os.open(dev.path, os.O_RDONLY)
-                os.lseek(fd, 440, 0) 
-                mbrsig = struct.unpack('I', os.read(fd, 4))
-                os.close(fd)
-            except OSError as e:
-                log.warning("Error reading mbrsig from disk %s: %s" %
-                            (dev.name, str(e)))
-                continue
-
-            mbrsigStr = "0x%08x\n" % mbrsig
-            if mbrsigStr == eddsig:
-                if eddsize:
-                    sysfspath = "/sys%s/size" % dev.sysfsPath
-                    try:
-                        file = open(sysfspath, "r")
-                        size = file.read()
-                        file.close()
-                    except (IOError, OSError) as e:
-                        log.warning("Error getting size for: %s" % dev.name)
-                        continue
-                    if eddsize != size:
-                        continue
-                found.append(dev.name)
-
-        if not found:
-            log.error("No matching mbr signature found for biosdev %d" %
-                      biosdev)
-        elif len(found) > 1:
-            log.error("Multiple signature matches found for biosdev %d: %s" %
-                      (biosdev, str(found)))
-        else:
-            log.info("Found %s for biosdev %d" %(found[0], biosdev))
-            edd_dict[found[0]] = biosdev
+        mbrsigStr = "0x%08x\n" % mbrsig
+        if mbrsigStr == eddsig:
+            if eddsize:
+                sysfspath = "/sys%s/size" % dev.sysfsPath
+                try:
+                    file = open(sysfspath, "r")
+                    size = file.read()
+                    file.close()
+                except (IOError, OSError) as e:
+                    log.warning("Error getting size for: %s" % dev.name)
+                    continue
+                if eddsize != size:
+                    continue
+            found.append(dev.name)

-    return edd_dict
+    if not found:
+        log.error("edd: no matching mbr signature found for biosdev %d" %
+                  biosdev)
+        return None
+    elif len(found) > 1:
+        log.error("edd: multiple signature matches found for biosdev %d: %s" %
+                  (biosdev, str(found)))
+        return None
+    else:
+        return found[0]
+
+def _devices_for_pcidev(devices, biosdev):
+    edd_dir = "/sys/firmware/edd/int13_dev%d" % biosdev
+    try:
+        dereferenced = os.readlink("%s/pci_dev" % edd_dir)
+    except OSError as e:
+        log.info ("edd: no pci_dev for biosdev %d" % biosdev)
+        return (None, [])
+    # get absolute and normalized path
+    sysfs_path = os.path.normpath(os.path.join(edd_dir, dereferenced))
+    if sysfs_path.startswith("/sys/"):
+        sysfs_path = sysfs_path[4:]
+    return (sysfs_path,
+            [dev for dev in devices if dev.sysfsPath.startswith(sysfs_path)]
+            )

[rhel6-branch 2/2] sda is less then vda in compareDisks()

[Ales Kozumplik]

Related: rhbz#621175
---
 storage/__init__.py |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/storage/__init__.py b/storage/__init__.py
index 09dab1f..03c8f07 100644
--- a/storage/__init__.py
+++ b/storage/__init__.py
@@ -1180,6 +1180,17 @@ class Storage(object):
         return self.fsset.rootDevice

def compareDisks(self, first, second):
+        def name_to_prio(name):
+            # the order is: hd < sd < vd or xvd < something else
+            if name.startswith("hd"):
+                return -1
+            elif name.startswith("sd"):
+                return 0
+            elif (name.startswith("vd") or name.startswith("xvd")):
+                return 1
+            else:
+                return 2
+
         # if exactly one of the devices has a pcidev, prefer it
         if bool(self.eddDict[first].pcidev) ^ bool(self.eddDict[second].pcidev):
             if self.eddDict[first].pcidev:
@@ -1202,24 +1213,8 @@ class Storage(object):
         if self.eddDict[second].biosdev:
             return 1

-        if first.startswith("hd"):
-            type1 = 0
-        elif first.startswith("sd"):
-            type1 = 1
-        elif (first.startswith("vd") or first.startswith("xvd")):
-            type1 = -1
-        else:
-            type1 = 2
-
-        if second.startswith("hd"):
-            type2 = 0
-        elif second.startswith("sd"):
-            type2 = 1
-        elif (second.startswith("vd") or second.startswith("xvd")):
-            type2 = -1
-        else:
-            type2 = 2
-
+        type1 = name_to_prio(first)
+        type2 = name_to_prio(second)
         if (type1 < type2):
             return -1
         elif (type1 > type2):

[David Lehman]

Both of these look okay to me, but I'd feel better having all of this
new code validated by the reporter of the bug.

Dave

On Thu, 2010-12-16 at 09:59 +0100, Ales Kozumplik wrote:
> In general there is not always mbrsig on a disk (new device), so we can't
> always succeed in creating the disk -> bios id mapping. On the other hand,
> the pci_dev symlink in edd does not give us complete information about the
> device. So we have to consider both in compareDisks().
>
> kickstart.py              |    4 +-
>  storage/__init__.py       |   18 ++++--
>  storage/devicelibs/edd.py |  167 ++++++++++++++++++++++++++++++---------------
>  3 files changed, 126 insertions(+), 63 deletions(-)
>
> diff --git a/kickstart.py b/kickstart.py
> index b606ab8..67b1480 100644
> --- a/kickstart.py
> +++ b/kickstart.py
> @@ -665,8 +665,8 @@ class PartitionData(commands.partition.F12_PartData):
>          storage.doAutoPart = False
>      if self.onbiosdisk != "":
>        for (disk, biosdisk) in storage.eddDict.iteritems():
>            if str(biosdisk) == self.onbiosdisk:
>        for (disk, eddinfo) in storage.eddDict.iteritems():
>            if str(eddinfo.biosdisk) == self.onbiosdisk:
>                self.disk = disk
>                break
>
> diff --git a/storage/__init__.py b/storage/__init__.py
> index 464eebf..09dab1f 100644
> --- a/storage/__init__.py
> +++ b/storage/__init__.py
> @@ -1180,18 +1180,26 @@ class Storage(object):
>          return self.fsset.rootDevice
>  def compareDisks(self, first, second):
>    if self.eddDict.has_key(first) and self.eddDict.has_key(second):
>        one = self.eddDict[first]
>        two = self.eddDict[second]
>    # if exactly one of the devices has a pcidev, prefer it
>    if bool(self.eddDict[first].pcidev) ^ bool(self.eddDict[second].pcidev):
>        if self.eddDict[first].pcidev:
>            return -1
>        if self.eddDict[second].pcidev:
>            return 1
>    # next, compare the biosdev numbers
>    if self.eddDict[first].biosdev and self.eddDict[second].biosdev:
>        one = self.eddDict[first].biosdev
>        two = self.eddDict[second].biosdev
>        if (one < two):
>            return -1
>        elif (one > two):
>            return 1
>
>    # if one is in the BIOS and the other not prefer the one in the BIOS
>    if self.eddDict.has_key(first):
>    if self.eddDict[first].biosdev:
>        return -1
>    if self.eddDict.has_key(second):
>    if self.eddDict[second].biosdev:
>        return 1
>
>    if first.startswith("hd"):
>
> diff --git a/storage/devicelibs/edd.py b/storage/devicelibs/edd.py
> index da03914..db2ba57 100644
> --- a/storage/devicelibs/edd.py
> +++ b/storage/devicelibs/edd.py
> @@ -21,77 +21,132 @@
>  #
>
> import os
> +import os.path
>  import struct
>
> import logging
>  log = logging.getLogger("storage")
>
> +class EddInfo:
> def __init__(self, biosdev):
>    self.biosdev = biosdev
>    self.pcidev = None
>
> def get_edd_dict(devices):
> """Given an array of devices return a dict with the BIOS ID for them."""
>
> """
>
> Given an array of devices return a mapping from their devices names to their
>
> EddInfo.
>
> CAVEATS: It is important to note that the current implementation of the edd
>
> kernel module is incomplete and does not give us a reliable method about
>
> discovering the complete device <-> BIOS device mapping. Checking the MBR
>
> signature works in most cases but will always fail e.g. on new disks that do
>
> not contain an MBR yet. On the other hand, the pcidev entry which is
>
> guaranteed to exist is shared across all disks connected through the same
>
> PCI device and so does not provide a one-to-one relation.
>
> """
>
> # initialize the dict with blank info
> edd_dict = {}
>
> for dev in devices:
>    edd_dict[dev.name] = EddInfo(None)
>
> for biosdev in range(80, 80 + 15):
>    sysfspath = "/sys/firmware/edd/int13_dev%d" % biosdev
>    if not os.path.exists(sysfspath):
>        break # We are done
>
>    sysfspath = "/sys/firmware/edd/int13_dev%d/mbr_signature" % biosdev
>    if not os.path.exists(sysfspath):
>        log.warning("No mbrsig for biosdev: %d" % biosdev)
>        continue
>    dev = _find_device_from_mbrsig(devices, biosdev)
>    if dev:
>        edd_dict[dev].biosdev = biosdev
>    (pcidev, devs_for_pcidev) = _devices_for_pcidev(devices, biosdev)
>    for d in devs_for_pcidev:
>        edd_dict[d.name].pcidev = pcidev
> _dump_dict(edd_dict)
> return edd_dict
>
> +def _dump_dict(edd_dict):
> log.debug("edd:generated dict:")
> for key in edd_dict:
>    log.debug("%s: biosdev: %s, pcidev: %s" %
>              (key, edd_dict[key].biosdev, edd_dict[key].pcidev))
> log.debug("edd:end of generated dict dump")
>
> +def _find_device_from_mbrsig(devices, biosdev):
>
> sysfspath = "/sys/firmware/edd/int13_dev%d/mbr_signature" % biosdev
>
> if not os.path.exists(sysfspath):
>    log.info("edd: no mbrsig for biosdev: %d" % biosdev)
>    return None
>
> try:
>    file = open(sysfspath, "r")
>    eddsig = file.read()
>    file.close()
>
> except (IOError, OSError) as e:
>    log.info("edd: error reading EDD mbrsig for %d: %s" %
>                (biosdev, str(e)))
>    return None
>
> sysfspath = "/sys/firmware/edd/int13_dev%d/sectors" % biosdev
>
> try:
>    file = open(sysfspath, "r")
>    eddsize = file.read()
>    file.close()
>
> except (IOError, OSError) as e:
>    eddsize = None
>
> found = []
>
> for dev in devices:
>     try:
>        file = open(sysfspath, "r")
>        eddsig = file.read()
>        file.close()
>    except (IOError, OSError) as e:
>        log.warning("Error reading EDD mbrsig for %d: %s" %
>                    (biosdev, str(e)))
>        fd = os.open(dev.path, os.O_RDONLY)
>        os.lseek(fd, 440, 0)
>        mbrsig = struct.unpack('I', os.read(fd, 4))
>        os.close(fd)
>    except OSError as e:
>        log.info("edd: error reading mbrsig from disk %s: %s" %
>                    (dev.name, str(e)))
>        continue
>
>    sysfspath = "/sys/firmware/edd/int13_dev%d/sectors" % biosdev
>    try:
>        file = open(sysfspath, "r")
>        eddsize = file.read()
>        file.close()
>    except (IOError, OSError) as e:
>        eddsize = None
>    found = []
>    for dev in devices:
>        try:
>            fd = os.open(dev.path, os.O_RDONLY)
>            os.lseek(fd, 440, 0) 
>            mbrsig = struct.unpack('I', os.read(fd, 4))
>            os.close(fd)
>        except OSError as e:
>            log.warning("Error reading mbrsig from disk %s: %s" %
>                        (dev.name, str(e)))
>            continue
>        mbrsigStr = "0x%08x\n" % mbrsig
>        if mbrsigStr == eddsig:
>            if eddsize:
>                sysfspath = "/sys%s/size" % dev.sysfsPath
>                try:
>                    file = open(sysfspath, "r")
>                    size = file.read()
>                    file.close()
>                except (IOError, OSError) as e:
>                    log.warning("Error getting size for: %s" % dev.name)
>                    continue
>                if eddsize != size:
>                    continue
>            found.append(dev.name)
>    if not found:
>        log.error("No matching mbr signature found for biosdev %d" %
>                  biosdev)
>    elif len(found) > 1:
>        log.error("Multiple signature matches found for biosdev %d: %s" %
>                  (biosdev, str(found)))
>    else:
>        log.info("Found %s for biosdev %d" %(found[0], biosdev))
>        edd_dict[found[0]] = biosdev
>    mbrsigStr = "0x%08x\n" % mbrsig
>    if mbrsigStr == eddsig:
>        if eddsize:
>            sysfspath = "/sys%s/size" % dev.sysfsPath
>            try:
>                file = open(sysfspath, "r")
>                size = file.read()
>                file.close()
>            except (IOError, OSError) as e:
>                log.warning("Error getting size for: %s" % dev.name)
>                continue
>            if eddsize != size:
>                continue
>        found.append(dev.name)
>
> return edd_dict
> if not found:
>    log.error("edd: no matching mbr signature found for biosdev %d" %
>              biosdev)
>    return None
> elif len(found) > 1:
>    log.error("edd: multiple signature matches found for biosdev %d: %s" %
>              (biosdev, str(found)))
>    return None
> else:
>    return found[0]
>
> +def _devices_for_pcidev(devices, biosdev):
> edd_dir = "/sys/firmware/edd/int13_dev%d" % biosdev
> try:
>    dereferenced = os.readlink("%s/pci_dev" % edd_dir)
> except OSError as e:
>    log.info ("edd: no pci_dev for biosdev %d" % biosdev)
>    return (None, [])
> # get absolute and normalized path
> sysfs_path = os.path.normpath(os.path.join(edd_dir, dereferenced))
> if sysfs_path.startswith("/sys/"):
>    sysfs_path = sysfs_path[4:]
> return (sysfs_path,
>        [dev for dev in devices if dev.sysfsPath.startswith(sysfs_path)]
>        )