Skip to content

Commit

Permalink
edd: Fix the error case in the "absurd_virt" test.
Browse files Browse the repository at this point in the history 
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@redhat.com>
  • Loading branch information
@vathpela
vathpela committed Nov 6, 2015
1 parent 779f18b commit 8cbd219
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 52 deletions.
137 changes: 88 additions & 49 deletions blivet/devicelibs/edd.py
View file
Expand Up @@ -358,47 +358,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'))
Expand All @@ -407,13 +369,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,)]
Expand All @@ -440,15 +402,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",
Expand All @@ -459,7 +498,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)

Expand Down
7 changes: 4 additions & 3 deletions tests/devicelibs_test/edd_test.py
View file
Expand Up @@ -197,6 +197,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.check_logs(debugs=debugs, infos=infos, warnings=warnings)

Expand Down Expand Up @@ -358,7 +359,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.check_logs(debugs=debugs, infos=infos, errors=errors)
self.check_logs(debugs=debugs, infos=infos, warnings=warnings)

0 comments on commit 8cbd219

Please sign in to comment.