[master 22/26] edd: Fix the error case in the "absurd_virt" test.

vathpela installerbot-noreply at redhat.com
Fri Nov 6 18:31:25 UTC 2015


From: Peter Jones <pjones at redhat.com>

The problem is that SeaBIOS (and thus our qemu VMs) doesn't enumerate ata
devices separately from PCI devices.  So on a real system,
EddEntry.ata_device should reset to 0 for each new PCI device, but on a
virt running SeaBIOS, they just count up monotonically.

So do the /right/ thing when we can, and if we don't find a correct
device, try relaxing the requirement on the scsi target id in sysfs, and
see if we find the right port number anyway.

Signed-off-by: Peter Jones <pjones at redhat.com>
---
 blivet/devicelibs/edd.py          | 137 ++++++++++++++++++++++++--------------
 tests/devicelibs_test/edd_test.py |   7 +-
 2 files changed, 92 insertions(+), 52 deletions(-)

diff --git a/blivet/devicelibs/edd.py b/blivet/devicelibs/edd.py
index 07d1694..49be9cd 100644
--- a/blivet/devicelibs/edd.py
+++ b/blivet/devicelibs/edd.py
@@ -357,47 +357,9 @@ def __init__(self, edd_entry, root=None):
 
     def devname_from_ata_pci_dev(self):
         pattern = util.Path('/sys/block/*', root=self.root)
-        answers = []
-        for path in pattern.glob():
-            emptyslash = util.Path("/", root=self.root)
-            path = util.Path(path, root=self.root)
-            link = util.sysfs_readlink(path=emptyslash, link=path)
-            testdata_log.debug("sysfs link: \"%s\" -> \"%s\"", path, link)
-            # just add /sys/block/ at the beginning so it's always valid
-            # paths in the filesystem...
-            components = ['/sys/block'] + link.split('/')
-            if len(components) != 11:
-                continue
-            # ATA and SATA paths look like:
-            # ../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda
-            # where literally the only pieces of data here are
-            # "pci0000:00:1f.2", "ata1", and "sda".
-            #
-            # EDD 3's "channel" doesn't really mean anything at all on SATA,
-            # and 255 means "not in use".  Any other value should be an ATA
-            # device (but might be a SATA device in compat mode), and that
-            # matches N in devM.N .  So basically "channel" means master/slave
-            # for ATA (non-SATA) devices.  Also in EDD 3, SATA port multipliers
-            # aren't represented in any way.
-            #
-            # In EDD 4, which unfortunately says to leave 0x30 as the version
-            # number, the port multiplier id is an additional field on the
-            # interface.  So basically we should try to use the value the
-            # kernel gives us*, but we can't trust it.  Thankfully there
-            # won't be a devX.Y.Z (i.e. a port multiplier device) in sysfs
-            # that collides with devX.Z (a non-port-multiplied device),
-            # so if we get a value from the kernel, we can try with and
-            # without it.
-            #
-            # * When the kernel finally learns of these facts...
-            #
-            if components[4] != '0000:%s' % (self.edd.pci_dev,):
-                continue
-            if not components[5].startswith('ata'):
-                continue
-            ata_port = components[5]
-            ata_port_idx = int(components[5][3:])
+        retries = []
 
+        def match_port(components, ata_port, ata_port_idx, path, link):
             fn = util.Path(util.join_paths(components[0:6] \
                                            + ['ata_port', ata_port]), root=self.root)
             port_no = int(util.get_sysfs_attr(fn, 'port_no'))
@@ -406,13 +368,13 @@ def devname_from_ata_pci_dev(self):
                 # On ATA, port_no is kernel's ata_port->local_port_no, which
                 # should be the same as the ata device number.
                 if port_no != self.edd.ata_device:
-                    continue
+                    return
             else:
                 # On SATA, "port_no" is the kernel's ata_port->print_id, which
                 # is awesomely ata_port->id + 1, where ata_port->id is edd's
                 # ata_device
                 if port_no != self.edd.ata_device + 1:
-                    continue
+                    return
 
             fn = components[0:6] + ['link%d' % (ata_port_idx,),]
             exp = [r'.*']+fn+[r'dev%d\.(\d+)(\.(\d+)){0,1}$' % (ata_port_idx,)]
@@ -439,15 +401,92 @@ def devname_from_ata_pci_dev(self):
                         channel = int(match.group(1))
                         if (self.edd.channel == 255 and channel == 0) or \
                                 (self.edd.channel == channel):
-                            answers.append({
-                                'link': util.Path(link, root=self.root),
-                                'path': path.split('/')[-1]})
+                            yield ({'link': util.Path(link, root=self.root),
+                                    'path': path.split('/')[-1]})
                     else:
                         pmp = int(match.group(1))
                         if self.edd.ata_pmp == pmp:
-                            answers.append({
-                                'link': util.Path(link, root=self.root),
-                                'path': path.split('/')[-1]})
+                            yield ({'link': util.Path(link, root=self.root),
+                                    'path': path.split('/')[-1]})
+
+        answers = []
+        for path in pattern.glob():
+            emptyslash = util.Path("/", self.root)
+            path = util.Path(path, self.root)
+            link = util.sysfs_readlink(path=emptyslash, link=path)
+            testdata_log.debug("sysfs link: \"%s\" -> \"%s\"", path, link)
+            # just add /sys/block/ at the beginning so it's always valid
+            # paths in the filesystem...
+            components = ['/sys/block'] + link.split('/')
+            if len(components) != 11:
+                continue
+            # ATA and SATA paths look like:
+            # ../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda
+            # where literally the only pieces of data here are
+            # "pci0000:00:1f.2", "ata1", and "sda".
+            #
+            # EDD 3's "channel" doesn't really mean anything at all on SATA,
+            # and 255 means "not in use".  Any other value should be an ATA
+            # device (but might be a SATA device in compat mode), and that
+            # matches N in devM.N .  So basically "channel" means master/slave
+            # for ATA (non-SATA) devices.  Also in EDD 3, SATA port multipliers
+            # aren't represented in any way.
+            #
+            # In EDD 4, which unfortunately says to leave 0x30 as the version
+            # number, the port multiplier id is an additional field on the
+            # interface.  So basically we should try to use the value the
+            # kernel gives us*, but we can't trust it.  Thankfully there
+            # won't be a devX.Y.Z (i.e. a port multiplier device) in sysfs
+            # that collides with devX.Z (a non-port-multiplied device),
+            # so if we get a value from the kernel, we can try with and
+            # without it.
+            #
+            # * When the kernel finally learns of these facts...
+            #
+            if components[4] != '0000:%s' % (self.edd.pci_dev,):
+                continue
+            if not components[5].startswith('ata'):
+                continue
+            ata_port = components[5]
+            ata_port_idx = int(components[5][3:])
+
+            # strictly this should always be required, but #!@#!@#!@ seabios
+            # iterates the sata device number /independently/ of the host
+            # bridge it claims things are attached to.  In that case this
+            # the scsi target will always have "0" as the ID component.
+            args = { 'device': self.edd.ata_device }
+            exp = r"target\d+:0:%(device)s/\d+:0:%(device)s:0/block/.*" % args
+            matcher = re.compile(exp)
+            match = matcher.match("/".join(components[7:]))
+            if not match:
+                retries.append({
+                    'components': components,
+                    'ata_port': ata_port,
+                    'ata_port_idx': ata_port_idx,
+                    'path': path,
+                    'link': link,
+                })
+                continue
+
+            for port in match_port(components, ata_port, ata_port_idx, path,
+                                   link):
+                answers.append(port)
+
+        # now handle the ones we discarded because libata's scsi id doesn't
+        # match the ata_device.
+        for retry in retries:
+            for port in match_port(**retry):
+                if answers:
+                    log.warning("edd: ignoring possible extra match for ATA device %s channel %s ata %d pmp %s: %s",
+                                self.edd.pci_dev, self.edd.channel,
+                                self.edd.ata_device, self.edd.ata_pmp,
+                                retry['path'])
+                else:
+                    log.warning("edd: using possible extra match for ATA device %s channel %s ata %d pmp %s: %s",
+                                self.edd.pci_dev, self.edd.channel,
+                                self.edd.ata_device, self.edd.ata_pmp,
+                                retry['path'])
+                    answers.append(port)
 
         if len(answers) > 1:
             log.error("edd: Found too many ATA devices for EDD device 0x%x: %s",
@@ -458,7 +497,7 @@ def devname_from_ata_pci_dev(self):
             return answers[0]['path']
         else:
             log.warning(
-                "edd: Could not find ATA device for pci dev %s channel %s ata %d pmp %d",
+                "edd: Could not find ATA device for pci dev %s channel %s ata %d pmp %s",
                 self.edd.pci_dev, self.edd.channel,
                 self.edd.ata_device, self.edd.ata_pmp)
 
diff --git a/tests/devicelibs_test/edd_test.py b/tests/devicelibs_test/edd_test.py
index 1f147c2..0a170a1 100644
--- a/tests/devicelibs_test/edd_test.py
+++ b/tests/devicelibs_test/edd_test.py
@@ -190,6 +190,7 @@ def test_get_edd_dict_sata_usb(self):
             ("edd: interface type %s is not implemented (%s)", "USB",
                 "/sys/firmware/edd/int13_dev81/"),
             ("edd: interface details: %s", "USB     \tserial_number: 30302e31"),
+            ('edd: using possible extra match for ATA device %s channel %s ata %d pmp %s: %s', '00:1f.2', 255, 1, None, '/sys/block/sda')
         ]
         self.checkLogs(debugs=debugs, infos=infos, warnings=warnings)
 
@@ -355,7 +356,7 @@ def test_get_edd_dict_absurd_virt(self):
             ('edd: Could not find Virtio device for pci dev %s channel %s', '00:01.1', 0),
             ('edd: Could not find Virtio device for pci dev %s channel %s', '00:0b.0', 0),
         ]
-        errors = [
-            ('edd: Found too many ATA devices for EDD device 0x%x: %s', 129, ['../devices/pci0000:00/0000:00:01.1/ata7/host6/target6:0:1/6:0:1:0/block/sdb', '../devices/pci0000:00/0000:00:01.1/ata7/host6/target6:0:0/6:0:0:0/block/sr0']),
+        warnings = [
+            ('edd: ignoring possible extra match for ATA device %s channel %s ata %d pmp %s: %s', '00:01.1', 0, 1, None, '/sys/block/sr0')
         ]
-        self.checkLogs(debugs=debugs, infos=infos, errors=errors)
+        self.checkLogs(debugs=debugs, infos=infos, warnings=warnings)


-- 
To view this commit on github, visit https://github.com/rhinstaller/blivet/commit/e9fc440e842a46fbeb004c74ae25bb3e6f79b234


More information about the anaconda-patches mailing list