Merge pull request #464 from dims/pci-vpd-chroot

pci: make Device.VPD respect the caller's chroot

Author: jaypipes

pkg/pci/pci.go

@@ -9,7 +9,6 @@ package pci
 import (
 	"encoding/json"
 	"fmt"
-	"sync"
 
 	"github.com/jaypipes/pcidb"
 
@@ -56,11 +55,6 @@ type Device struct {
 	// no particular order. Populated after enumeration; not included in
 	// JSON output.
 	Children []*Device `json:"-"`
-
-	// Cached VPD result (lazy). Guarded by vpdOnce.
-	vpdOnce sync.Once
-	vpd     *VPD
-	vpdErr  error
 }
 
 type devIdent struct {

pkg/pci/pci_stub.go

@@ -56,6 +56,6 @@ var ErrVPDUnavailable = errors.New("vpd: no sysfs directory associated with devi
 var ErrVPDNotPresent = errors.New("vpd: not present for device")
 
 // VPD returns ErrVPDUnavailable on non-Linux platforms.
-func (d *Device) VPD() (*VPD, error) {
+func (d *Device) VPD(ctx context.Context) (*VPD, error) {
 	return nil, ErrVPDUnavailable
 }

pkg/pci/vpd_linux.go

@@ -85,8 +85,9 @@ var ErrVPDUnavailable = errors.New("vpd: no sysfs directory associated with devi
 var ErrVPDNotPresent = errors.New("vpd: not present for device")
 
 // VPD returns the parsed Vital Product Data for the device, reading it
-// from the device's sysfs `vpd` file on first call and caching the
-// result for subsequent calls.
+// from the device's sysfs `vpd` file. The sysfs root is taken from ctx,
+// so a ctx built via option.WithChroot reads VPD from inside the
+// chroot; context.Background() reads the live /sys.
 //
 // The sysfs VPD file is typically root-readable only. Callers running
 // without sufficient privilege will receive a wrapped permission
@@ -96,22 +97,15 @@ var ErrVPDNotPresent = errors.New("vpd: not present for device")
 // address (e.g. those constructed via Info.ParseDevice) and
 // ErrVPDNotPresent for devices whose sysfs entry exists but exposes
 // no `vpd` file.
-func (d *Device) VPD() (*VPD, error) {
-	d.vpdOnce.Do(func() {
-		d.vpd, d.vpdErr = d.readVPD()
-	})
-	return d.vpd, d.vpdErr
-}
-
-func (d *Device) readVPD() (*VPD, error) {
+func (d *Device) VPD(ctx context.Context) (*VPD, error) {
 	if d.Address == "" {
 		return nil, ErrVPDUnavailable
 	}
 	pciAddr := pciaddr.FromString(d.Address)
 	if pciAddr == nil {
 		return nil, ErrVPDUnavailable
 	}
-	paths := linuxpath.New(context.Background())
+	paths := linuxpath.New(ctx)
 	vpdPath := filepath.Join(paths.SysBusPciDevices, pciAddr.String(), "vpd")
 	data, err := os.ReadFile(vpdPath)
 	if err != nil {

pkg/pci/vpd_test.go

@@ -10,10 +10,16 @@
 package pci
 
 import (
+	"context"
 	"encoding/binary"
 	"errors"
+	"os"
+	"path/filepath"
 	"reflect"
 	"testing"
+
+	"github.com/jaypipes/ghw/internal/config"
+	"github.com/jaypipes/ghw/pkg/option"
 )
 
 // buildVPD assembles a synthetic VPD byte stream from a sequence of
@@ -211,7 +217,59 @@ func TestDeviceVPDUnavailableForParsedDevice(t *testing.T) {
 	// Info.ParseDevice) must surface ErrVPDUnavailable rather than
 	// trying to read a path-less file.
 	d := &Device{}
-	if _, err := d.VPD(); !errors.Is(err, ErrVPDUnavailable) {
+	if _, err := d.VPD(context.Background()); !errors.Is(err, ErrVPDUnavailable) {
 		t.Errorf("got %v, want ErrVPDUnavailable", err)
 	}
 }
+
+// TestDeviceVPDHonorsChroot is the regression test for Device.VPD
+// reading the live /sys regardless of option.WithChroot.
+func TestDeviceVPDHonorsChroot(t *testing.T) {
+	chroot := t.TempDir()
+	const addr = "0000:00:00.0"
+	realDir := filepath.Join(chroot, "sys", "devices", "pci0000:00", addr)
+	if err := os.MkdirAll(realDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	devsDir := filepath.Join(chroot, "sys", "bus", "pci", "devices")
+	if err := os.MkdirAll(devsDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	const modalias = "pci:v000010DEd000022B1sv00000000sd00000000bc06sc04i00"
+	if err := os.WriteFile(filepath.Join(realDir, "modalias"), []byte(modalias+"\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	rel, err := filepath.Rel(devsDir, realDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(rel, filepath.Join(devsDir, addr)); err != nil {
+		t.Fatal(err)
+	}
+	vpd := buildVPD([]struct {
+		tag  byte
+		body []byte
+	}{
+		{vpdTagLargeIdent, []byte("chroot-test")},
+	})
+	if err := os.WriteFile(filepath.Join(realDir, "vpd"), vpd, 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	ctx := config.ContextFromArgs(option.WithChroot(chroot), config.WithDisableTopology())
+	info, err := New(ctx)
+	if err != nil {
+		t.Fatalf("pci.New: %v", err)
+	}
+	if len(info.Devices) != 1 || info.Devices[0].Address != addr {
+		t.Fatalf("expected one device at %s, got %+v", addr, info.Devices)
+	}
+
+	v, err := info.Devices[0].VPD(ctx)
+	if err != nil {
+		t.Fatalf("VPD: %v", err)
+	}
+	if v.Identifier != "chroot-test" {
+		t.Errorf("got identifier %q, want %q", v.Identifier, "chroot-test")
+	}
+}