diff --git a/alias.go b/alias.go index 965d80be..66b4f2b9 100644 --- a/alias.go +++ b/alias.go @@ -17,6 +17,7 @@ import ( "github.com/jaypipes/ghw/pkg/net" "github.com/jaypipes/ghw/pkg/option" "github.com/jaypipes/ghw/pkg/pci" + pciaddress "github.com/jaypipes/ghw/pkg/pci/address" "github.com/jaypipes/ghw/pkg/product" "github.com/jaypipes/ghw/pkg/topology" ) @@ -125,12 +126,12 @@ const ( ) type PCIInfo = pci.Info -type PCIAddress = pci.Address +type PCIAddress = pciaddress.Address type PCIDevice = pci.Device var ( PCI = pci.New - PCIAddressFromString = pci.AddressFromString + PCIAddressFromString = pciaddress.FromString ) type ProductInfo = product.Info diff --git a/pkg/pci/address/address.go b/pkg/pci/address/address.go new file mode 100644 index 00000000..7b136053 --- /dev/null +++ b/pkg/pci/address/address.go @@ -0,0 +1,55 @@ +// +// Use and distribution licensed under the Apache license version 2. +// +// See the COPYING file in the root project directory for full text. +// + +package address + +import ( + "regexp" + "strings" +) + +var ( + regexAddress *regexp.Regexp = regexp.MustCompile( + `^(([0-9a-f]{0,4}):)?([0-9a-f]{2}):([0-9a-f]{2})\.([0-9a-f]{1})$`, + ) +) + +type Address struct { + Domain string + Bus string + Slot string + Function string +} + +// String() returns the canonical [D]BSF representation of this Address +func (addr *Address) String() string { + return addr.Domain + ":" + addr.Bus + ":" + addr.Slot + "." + addr.Function +} + +// Given a string address, returns a complete Address struct, filled in with +// domain, bus, slot and function components. The address string may either +// be in $BUS:$SLOT.$FUNCTION (BSF) format or it can be a full PCI address +// that includes the 4-digit $DOMAIN information as well: +// $DOMAIN:$BUS:$SLOT.$FUNCTION. +// +// Returns "" if the address string wasn't a valid PCI address. +func FromString(address string) *Address { + addrLowered := strings.ToLower(address) + matches := regexAddress.FindStringSubmatch(addrLowered) + if len(matches) == 6 { + dom := "0000" + if matches[1] != "" { + dom = matches[2] + } + return &Address{ + Domain: dom, + Bus: matches[3], + Slot: matches[4], + Function: matches[5], + } + } + return nil +} diff --git a/pkg/pci/address/address_test.go b/pkg/pci/address/address_test.go new file mode 100644 index 00000000..4edf3d7a --- /dev/null +++ b/pkg/pci/address/address_test.go @@ -0,0 +1,82 @@ +// +// Use and distribution licensed under the Apache license version 2. +// +// See the COPYING file in the root project directory for full text. +// + +package address_test + +import ( + "reflect" + "strings" + "testing" + + pciaddr "github.com/jaypipes/ghw/pkg/pci/address" +) + +func TestPCIAddressFromString(t *testing.T) { + + tests := []struct { + addrStr string + expected *pciaddr.Address + // AddressFromString is more flexible than String() and wants + // to accept addresses not in full canonical form, as long as + // it can do the right thing - e.g. a sane default Domain exists. + // Thus we need to sometimes skip the Address -> string check. + skipStringTest bool + }{ + { + addrStr: "00:00.0", + expected: &pciaddr.Address{ + Domain: "0000", + Bus: "00", + Slot: "00", + Function: "0", + }, + skipStringTest: true, + }, + { + addrStr: "0000:00:00.0", + expected: &pciaddr.Address{ + Domain: "0000", + Bus: "00", + Slot: "00", + Function: "0", + }, + }, + { + addrStr: "0000:03:00.0", + expected: &pciaddr.Address{ + Domain: "0000", + Bus: "03", + Slot: "00", + Function: "0", + }, + }, + { + addrStr: "0000:03:00.A", + expected: &pciaddr.Address{ + Domain: "0000", + Bus: "03", + Slot: "00", + Function: "a", + }, + }, + } + for x, test := range tests { + got := pciaddr.FromString(test.addrStr) + if !reflect.DeepEqual(got, test.expected) { + t.Fatalf("Test #%d failed. Expected %v but got %v", x, test.expected, got) + } + + if test.skipStringTest { + continue + } + + addrStr := got.String() + // addresses are case insensitive + if !strings.EqualFold(addrStr, test.addrStr) { + t.Fatalf("Test #%d failed. Expected %q but got %q (case insensitive match)", x, test.addrStr, addrStr) + } + } +} diff --git a/pkg/pci/pci.go b/pkg/pci/pci.go index c2c347bd..50f3b9f3 100644 --- a/pkg/pci/pci.go +++ b/pkg/pci/pci.go @@ -10,17 +10,23 @@ import ( "encoding/json" "fmt" "regexp" - "strings" "github.com/jaypipes/pcidb" "github.com/jaypipes/ghw/pkg/context" "github.com/jaypipes/ghw/pkg/marshal" "github.com/jaypipes/ghw/pkg/option" + pciaddr "github.com/jaypipes/ghw/pkg/pci/address" "github.com/jaypipes/ghw/pkg/topology" "github.com/jaypipes/ghw/pkg/util" ) +// backward compatibility, to be removed in 1.0.0 +type Address pciaddr.Address + +// backward compatibility, to be removed in 1.0.0 +var AddressFromString = pciaddr.FromString + var ( regexAddress *regexp.Regexp = regexp.MustCompile( `^(([0-9a-f]{0,4}):)?([0-9a-f]{2}):([0-9a-f]{2})\.([0-9a-f]{1})$`, @@ -142,43 +148,6 @@ func (i *Info) String() string { return fmt.Sprintf("PCI (%d devices)", len(i.Devices)) } -type Address struct { - Domain string - Bus string - Slot string - Function string -} - -// String() returns the canonical [D]BSF representation of this Address -func (addr *Address) String() string { - return addr.Domain + ":" + addr.Bus + ":" + addr.Slot + "." + addr.Function -} - -// Given a string address, returns a complete Address struct, filled in with -// domain, bus, slot and function components. The address string may either -// be in $BUS:$SLOT.$FUNCTION (BSF) format or it can be a full PCI address -// that includes the 4-digit $DOMAIN information as well: -// $DOMAIN:$BUS:$SLOT.$FUNCTION. -// -// Returns "" if the address string wasn't a valid PCI address. -func AddressFromString(address string) *Address { - addrLowered := strings.ToLower(address) - matches := regexAddress.FindStringSubmatch(addrLowered) - if len(matches) == 6 { - dom := "0000" - if matches[1] != "" { - dom = matches[2] - } - return &Address{ - Domain: dom, - Bus: matches[3], - Slot: matches[4], - Function: matches[5], - } - } - return nil -} - // New returns a pointer to an Info struct that contains information about the // PCI devices on the host system func New(opts ...*option.Option) (*Info, error) { diff --git a/pkg/pci/pci_linux.go b/pkg/pci/pci_linux.go index 3d62f44a..aa3c8b91 100644 --- a/pkg/pci/pci_linux.go +++ b/pkg/pci/pci_linux.go @@ -6,7 +6,6 @@ package pci import ( - "fmt" "io/ioutil" "os" "path/filepath" @@ -16,6 +15,7 @@ import ( "github.com/jaypipes/ghw/pkg/context" "github.com/jaypipes/ghw/pkg/linuxpath" + pciaddr "github.com/jaypipes/ghw/pkg/pci/address" "github.com/jaypipes/ghw/pkg/topology" "github.com/jaypipes/ghw/pkg/util" ) @@ -34,7 +34,7 @@ func (i *Info) load() error { func getDeviceModaliasPath(ctx *context.Context, address string) string { paths := linuxpath.New(ctx) - pciAddr := AddressFromString(address) + pciAddr := pciaddr.FromString(address) if pciAddr == nil { return "" } @@ -47,7 +47,7 @@ func getDeviceModaliasPath(ctx *context.Context, address string) string { func getDeviceRevision(ctx *context.Context, address string) string { paths := linuxpath.New(ctx) - pciAddr := AddressFromString(address) + pciAddr := pciaddr.FromString(address) if pciAddr == nil { return "" } @@ -276,11 +276,13 @@ func findPCIProgrammingInterface( func (info *Info) GetDevice(address string) *Device { fp := getDeviceModaliasPath(info.ctx, address) if fp == "" { + info.ctx.Warn("error finding modalias info for device %q", address) return nil } modaliasInfo := parseModaliasFile(fp) if modaliasInfo == nil { + info.ctx.Warn("error parsing modalias info for device %q", address) return nil } @@ -354,7 +356,7 @@ func (info *Info) ListDevices() []*Device { // address and append to the returned array. links, err := ioutil.ReadDir(paths.SysBusPciDevices) if err != nil { - _, _ = fmt.Fprintf(os.Stderr, "error: failed to read /sys/bus/pci/devices") + info.ctx.Warn("failed to read /sys/bus/pci/devices") return nil } var dev *Device @@ -362,7 +364,7 @@ func (info *Info) ListDevices() []*Device { addr := link.Name() dev = info.GetDevice(addr) if dev == nil { - _, _ = fmt.Fprintf(os.Stderr, "error: failed to get device information for PCI address %s\n", addr) + info.ctx.Warn("failed to get device information for PCI address %s", addr) } else { devs = append(devs, dev) } diff --git a/pkg/pci/pci_test.go b/pkg/pci/pci_test.go index 50d03843..44368e8c 100644 --- a/pkg/pci/pci_test.go +++ b/pkg/pci/pci_test.go @@ -8,8 +8,6 @@ package pci_test import ( "os" - "reflect" - "strings" "testing" "github.com/jaypipes/ghw/pkg/context" @@ -17,73 +15,6 @@ import ( "github.com/jaypipes/ghw/pkg/pci" ) -func TestPCIAddressFromString(t *testing.T) { - - tests := []struct { - addrStr string - expected *pci.Address - // AddressFromString is more flexible than String() and wants - // to accept addresses not in full canonical form, as long as - // it can do the right thing - e.g. a sane default Domain exists. - // Thus we need to sometimes skip the Address -> string check. - skipStringTest bool - }{ - { - addrStr: "00:00.0", - expected: &pci.Address{ - Domain: "0000", - Bus: "00", - Slot: "00", - Function: "0", - }, - skipStringTest: true, - }, - { - addrStr: "0000:00:00.0", - expected: &pci.Address{ - Domain: "0000", - Bus: "00", - Slot: "00", - Function: "0", - }, - }, - { - addrStr: "0000:03:00.0", - expected: &pci.Address{ - Domain: "0000", - Bus: "03", - Slot: "00", - Function: "0", - }, - }, - { - addrStr: "0000:03:00.A", - expected: &pci.Address{ - Domain: "0000", - Bus: "03", - Slot: "00", - Function: "a", - }, - }, - } - for x, test := range tests { - got := pci.AddressFromString(test.addrStr) - if !reflect.DeepEqual(got, test.expected) { - t.Fatalf("Test #%d failed. Expected %v but got %v", x, test.expected, got) - } - - if test.skipStringTest { - continue - } - - addrStr := got.String() - // addresses are case insensitive - if !strings.EqualFold(addrStr, test.addrStr) { - t.Fatalf("Test #%d failed. Expected %q but got %q (case insensitive match)", x, test.addrStr, addrStr) - } - } -} - func TestPCI(t *testing.T) { if _, ok := os.LookupEnv("GHW_TESTING_SKIP_PCI"); ok { t.Skip("Skipping PCI tests.") diff --git a/pkg/snapshot/clonetree.go b/pkg/snapshot/clonetree.go index 01c22bb7..ba9f8f10 100644 --- a/pkg/snapshot/clonetree.go +++ b/pkg/snapshot/clonetree.go @@ -43,11 +43,15 @@ func CloneTreeInto(scratchDir string) error { } // ExpectedCloneContent return a slice of glob patterns which represent the pseudofiles -// ghw cares about. The intended usage of this function is to validate a clone tree, -// checking that the content matches the expectations. +// ghw cares about. +// The intended usage of this function is to validate a clone tree, checking that the +// content matches the expectations. +// Beware: the content is host-specific, because the content pertaining some subsystems, +// most notably PCI, is host-specific and unpredictable. func ExpectedCloneContent() []string { fileSpecs := ExpectedCloneStaticContent() fileSpecs = append(fileSpecs, ExpectedCloneNetContent()...) + fileSpecs = append(fileSpecs, ExpectedClonePCIContent()...) return fileSpecs } @@ -59,12 +63,6 @@ func ExpectedCloneStaticContent() []string { "/etc/mtab", "/proc/cpuinfo", "/proc/meminfo", - "/sys/bus/pci/devices/*", - "/sys/devices/pci*/*/irq", - "/sys/devices/pci*/*/local_cpulist", - "/sys/devices/pci*/*/modalias", - "/sys/devices/pci*/*/numa_node", - "/sys/devices/pci*/pci_bus/*/cpulistaffinity", "/sys/devices/system/cpu/cpu*/cache/index*/*", "/sys/devices/system/cpu/cpu*/topology/*", "/sys/devices/system/memory/block_size_bytes", diff --git a/pkg/snapshot/clonetree_net.go b/pkg/snapshot/clonetree_net.go index b16629f2..0a67e41d 100644 --- a/pkg/snapshot/clonetree_net.go +++ b/pkg/snapshot/clonetree_net.go @@ -14,6 +14,8 @@ import ( ) const ( + // warning: don't use the context package here, this means not even the linuxpath package. + // TODO(fromani) remove the path duplication sysClassNet = "/sys/class/net" ) diff --git a/pkg/snapshot/clonetree_pci.go b/pkg/snapshot/clonetree_pci.go new file mode 100644 index 00000000..503a562d --- /dev/null +++ b/pkg/snapshot/clonetree_pci.go @@ -0,0 +1,148 @@ +// +// Use and distribution licensed under the Apache license version 2. +// +// See the COPYING file in the root project directory for full text. +// + +package snapshot + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + + pciaddr "github.com/jaypipes/ghw/pkg/pci/address" +) + +const ( + // root directory: entry point to start scanning the PCI forest + // warning: don't use the context package here, this means not even the linuxpath package. + // TODO(fromani) remove the path duplication + sysBusPCIDir = "/sys/bus/pci/devices" +) + +// ExpectedClonePCIContent return a slice of glob patterns which represent the pseudofiles +// ghw cares about, pertaining to PCI devices only. +// Beware: the content is host-specific, because the PCI topology is host-dependent and unpredictable. +func ExpectedClonePCIContent() []string { + var fileSpecs []string + pciRoots := []string{ + sysBusPCIDir, + } + for { + if len(pciRoots) == 0 { + break + } + pciRoot := pciRoots[0] + pciRoots = pciRoots[1:] + specs, roots := scanPCIDeviceRoot(pciRoot) + pciRoots = append(pciRoots, roots...) + fileSpecs = append(fileSpecs, specs...) + } + return fileSpecs +} + +// scanPCIDeviceRoot reports a slice of glob patterns which represent the pseudofiles +// ghw cares about pertaining to all the PCI devices connected to the bus connected from the +// given root; usually (but not always) a CPU packages has 1+ PCI(e) roots, forming the first +// level; more PCI bridges are (usually) attached to this level, creating deep nested trees. +// hence we need to scan all possible roots, to make sure not to miss important devices. +// +// note about notifying errors. This function and its helper functions do use trace() everywhere +// to report recoverable errors, even though it would have been appropriate to use Warn(). +// This is unfortunate, and again a byproduct of the fact we cannot use context.Context to avoid +// circular dependencies. +// TODO(fromani): switch to Warn() as soon as we figure out how to break this circular dep. +func scanPCIDeviceRoot(root string) (fileSpecs []string, pciRoots []string) { + trace("scanning PCI device root %q\n", root) + + perDevEntries := []string{ + "class", + "device", + "irq", + "local_cpulist", + "modalias", + "numa_node", + "revision", + "vendor", + } + entries, err := ioutil.ReadDir(root) + if err != nil { + return []string{}, []string{} + } + for _, entry := range entries { + entryName := entry.Name() + if addr := pciaddr.FromString(entryName); addr == nil { + // doesn't look like a entry we care about + // This is by far and large the most likely path + // hence we should NOT trace/warn here. + continue + } + + entryPath := filepath.Join(root, entryName) + pciEntry, err := findPCIEntryFromPath(root, entryName) + if err != nil { + trace("error scanning %q: %v", entryName, err) + continue + } + + trace("PCI entry is %q\n", pciEntry) + fileSpecs = append(fileSpecs, entryPath) + for _, perNetEntry := range perDevEntries { + fileSpecs = append(fileSpecs, filepath.Join(pciEntry, perNetEntry)) + } + + if isPCIBridge(entryPath) { + trace("adding new PCI root %q\n", entryName) + pciRoots = append(pciRoots, pciEntry) + } + } + return fileSpecs, pciRoots +} + +func findPCIEntryFromPath(root, entryName string) (string, error) { + entryPath := filepath.Join(root, entryName) + fi, err := os.Lstat(entryPath) + if err != nil { + return "", fmt.Errorf("stat(%s) failed: %v\n", entryPath, err) + } + if fi.Mode()&os.ModeSymlink == 0 { + // regular file, nothing to resolve + return entryPath, nil + } + // resolve symlink + target, err := os.Readlink(entryPath) + trace("entry %q is symlink resolved to %q\n", entryPath, target) + if err != nil { + return "", fmt.Errorf("readlink(%s) failed: %v - skipped\n", entryPath, err) + } + return filepath.Clean(filepath.Join(root, target)), nil +} + +func isPCIBridge(entryPath string) bool { + subNodes, err := ioutil.ReadDir(entryPath) + if err != nil { + // this is so unlikely we don't even return error. But we trace just in case. + trace("error scanning device entry path %q: %v", entryPath, err) + return false + } + for _, subNode := range subNodes { + if !subNode.IsDir() { + continue + } + if addr := pciaddr.FromString(subNode.Name()); addr != nil { + // we got an entry in the directory pertaining to this device + // which is a directory itself and it is named like a PCI address. + // Hence we infer the device we are considering is a PCI bridge of sorts. + // This is is indeed a bit brutal, but the only possible alternative + // (besides blindly copying everything in /sys/bus/pci/devices) is + // to detect the type of the device and pick only the bridges. + // This approach duplicates the logic within the `pci` subkpg + // - or forces us into awkward dep cycles, and has poorer forward + // compatibility. + return true + } + } + return false +}