Merge branch 'main' into numa-memory-info

alias.go

@@ -27,8 +27,8 @@ type WithOption = option.Option
 var (
 	WithChroot       = option.WithChroot
 	WithSnapshot     = option.WithSnapshot
-	WithAlterter     = option.WithAlerter
-	WithNullAlterter = option.WithNullAlerter
+	WithAlerter      = option.WithAlerter
+	WithNullAlerter  = option.WithNullAlerter
 	// match the existing environ variable to minimize surprises
 	WithDisableWarnings = option.WithNullAlerter
 	WithDisableTools    = option.WithDisableTools

pkg/block/block.go

@@ -7,8 +7,10 @@
 package block
 
 import (
+	"encoding/json"
 	"fmt"
 	"math"
+	"strconv"
 	"strings"
 
 	"github.com/jaypipes/ghw/pkg/context"
@@ -37,6 +39,19 @@ var (
 		DRIVE_TYPE_ODD:     "ODD",
 		DRIVE_TYPE_SSD:     "SSD",
 	}
+
+	// NOTE(fromani): the keys are all lowercase and do not match
+	// the keys in the opposite table `driveTypeString`.
+	// This is done because of the choice we made in
+	// DriveType::MarshalJSON.
+	// We use this table only in UnmarshalJSON, so it should be OK.
+	stringDriveType = map[string]DriveType{
+		"unknown": DRIVE_TYPE_UNKNOWN,
+		"hdd":     DRIVE_TYPE_HDD,
+		"fdd":     DRIVE_TYPE_FDD,
+		"odd":     DRIVE_TYPE_ODD,
+		"ssd":     DRIVE_TYPE_SSD,
+	}
 )
 
 func (dt DriveType) String() string {
@@ -47,7 +62,21 @@ func (dt DriveType) String() string {
 // get, let's lowercase the string output when serializing, in order to
 // "normalize" the expected serialized output
 func (dt DriveType) MarshalJSON() ([]byte, error) {
-	return []byte("\"" + strings.ToLower(dt.String()) + "\""), nil
+	return []byte(strconv.Quote(strings.ToLower(dt.String()))), nil
+}
+
+func (dt *DriveType) UnmarshalJSON(b []byte) error {
+	var s string
+	if err := json.Unmarshal(b, &s); err != nil {
+		return err
+	}
+	key := strings.ToLower(s)
+	val, ok := stringDriveType[key]
+	if !ok {
+		return fmt.Errorf("unknown drive type: %q", key)
+	}
+	*dt = val
+	return nil
 }
 
 // StorageController is a category of block storage controller/driver. It
@@ -75,17 +104,45 @@ var (
 		STORAGE_CONTROLLER_VIRTIO:  "virtio",
 		STORAGE_CONTROLLER_MMC:     "MMC",
 	}
+
+	// NOTE(fromani): the keys are all lowercase and do not match
+	// the keys in the opposite table `storageControllerString`.
+	// This is done/ because of the choice we made in
+	// StorageController::MarshalJSON.
+	// We use this table only in UnmarshalJSON, so it should be OK.
+	stringStorageController = map[string]StorageController{
+		"unknown": STORAGE_CONTROLLER_UNKNOWN,
+		"ide":     STORAGE_CONTROLLER_IDE,
+		"scsi":    STORAGE_CONTROLLER_SCSI,
+		"nvme":    STORAGE_CONTROLLER_NVME,
+		"virtio":  STORAGE_CONTROLLER_VIRTIO,
+		"mmc":     STORAGE_CONTROLLER_MMC,
+	}
 )
 
 func (sc StorageController) String() string {
 	return storageControllerString[sc]
 }
 
+func (sc *StorageController) UnmarshalJSON(b []byte) error {
+	var s string
+	if err := json.Unmarshal(b, &s); err != nil {
+		return err
+	}
+	key := strings.ToLower(s)
+	val, ok := stringStorageController[key]
+	if !ok {
+		return fmt.Errorf("unknown storage controller: %q", key)
+	}
+	*sc = val
+	return nil
+}
+
 // NOTE(jaypipes): since serialized output is as "official" as we're going to
 // get, let's lowercase the string output when serializing, in order to
 // "normalize" the expected serialized output
 func (sc StorageController) MarshalJSON() ([]byte, error) {
-	return []byte("\"" + strings.ToLower(sc.String()) + "\""), nil
+	return []byte(strconv.Quote(strings.ToLower(sc.String()))), nil
 }
 
 // Disk describes a single disk drive on the host system. Disk drives provide

pkg/block/block_test.go

@@ -7,11 +7,16 @@
 package block_test
 
 import (
+	"encoding/json"
+	"io/ioutil"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/jaypipes/ghw/pkg/block"
+
+	"github.com/jaypipes/ghw/testdata"
 )
 
 // nolint: gocyclo
@@ -23,7 +28,7 @@ func TestBlock(t *testing.T) {
 	info, err := block.New()
 
 	if err != nil {
-		t.Fatalf("Expected no error creating BlockInfo, but got %v", err)
+		t.Fatalf("Expected no error creating block.Info, but got %v", err)
 	}
 	tpb := info.TotalPhysicalBytes
 
@@ -73,3 +78,69 @@ func TestBlock(t *testing.T) {
 		}
 	}
 }
+
+func TestBlockMarshalUnmarshal(t *testing.T) {
+	blocks, err := block.New()
+	if err != nil {
+		t.Fatalf("Expected no error creating block.Info, but got %v", err)
+	}
+
+	data, err := json.Marshal(blocks)
+	if err != nil {
+		t.Fatalf("Expected no error marshaling block.Info, but got %v", err)
+	}
+
+	var bi *block.Info
+	err = json.Unmarshal(data, &bi)
+	if err != nil {
+		t.Fatalf("Expected no error unmarshaling block.Info, but got %v", err)
+	}
+}
+
+type blockData struct {
+	Block block.Info `json:"block"`
+}
+
+func TestBlockUnmarshal(t *testing.T) {
+	testdataPath, err := testdata.SamplesDirectory()
+	if err != nil {
+		t.Fatalf("Expected nil err when detecting the samples directory, but got %v", err)
+	}
+
+	data, err := ioutil.ReadFile(filepath.Join(testdataPath, "dell-r610-block.json"))
+	if err != nil {
+		t.Fatalf("Expected nil err when reading the sample data, but got %v", err)
+	}
+
+	var bd blockData
+	err = json.Unmarshal(data, &bd)
+	if err != nil {
+		t.Fatalf("Expected no error unmarshaling block.Info, but got %v", err)
+	}
+
+	// to learn why we check these values, please review the "dell-r610-block.json" sample
+	sda := findDiskByName(bd.Block.Disks, "sda")
+	if sda == nil {
+		t.Fatalf("unexpected error: can't find 'sda' in the test data")
+	}
+	if sda.DriveType != block.DRIVE_TYPE_HDD || sda.StorageController != block.STORAGE_CONTROLLER_SCSI {
+		t.Fatalf("inconsistent data for sda: %s", sda)
+	}
+
+	zram0 := findDiskByName(bd.Block.Disks, "zram0")
+	if zram0 == nil {
+		t.Fatalf("unexpected error: can't find 'zram0' in the test data")
+	}
+	if zram0.DriveType != block.DRIVE_TYPE_SSD {
+		t.Fatalf("inconsistent data for zram0: %s", zram0)
+	}
+}
+
+func findDiskByName(disks []*block.Disk, name string) *block.Disk {
+	for _, disk := range disks {
+		if disk.Name == name {
+			return disk
+		}
+	}
+	return nil
+}

pkg/context/context.go

@@ -11,8 +11,8 @@ import (
 	"github.com/jaypipes/ghw/pkg/snapshot"
 )
 
-// Concrete merged set of configuration switches that act as an execution
-// context when calling internal discovery methods
+// Context contains the merged set of configuration switches that act as an
+// execution context when calling internal discovery methods
 type Context struct {
 	Chroot               string
 	EnableTools          bool
@@ -24,10 +24,37 @@ type Context struct {
 	alert                option.Alerter
 }
 
+// WithContext returns an option.Option that contains a pre-existing Context
+// struct. This is useful for some internal code that sets up snapshots.
+func WithContext(ctx *Context) *option.Option {
+	return &option.Option{
+		Context: ctx,
+	}
+}
+
+// Exists returns true if the supplied (merged) Option already contains
+// a context.
+//
+// TODO(jaypipes): We can get rid of this when we combine the option and
+// context packages, which will make it easier to detect the presence of a
+// pre-setup Context.
+func Exists(opt *option.Option) bool {
+	return opt != nil && opt.Context != nil
+}
+
 // New returns a Context struct pointer that has had various options set on it
 func New(opts ...*option.Option) *Context {
 	merged := option.Merge(opts...)
-	ctx := &Context{
+	var ctx *Context
+	if merged.Context != nil {
+		var castOK bool
+		ctx, castOK = merged.Context.(*Context)
+		if !castOK {
+			panic("passed in a non-Context for the WithContext() function!")
+		}
+		return ctx
+	}
+	ctx = &Context{
 		alert:  option.EnvOrDefaultAlerter(),
 		Chroot: *merged.Chroot,
 	}

pkg/gpu/gpu_linux.go

@@ -101,7 +101,7 @@ func (i *Info) load() error {
 // Loops through each GraphicsCard struct and attempts to fill the DeviceInfo
 // attribute with PCI device information
 func gpuFillPCIDevice(ctx *context.Context, cards []*GraphicsCard) {
-	pci, err := pci.NewWithContext(ctx)
+	pci, err := pci.New(context.WithContext(ctx))
 	if err != nil {
 		return
 	}
@@ -117,7 +117,7 @@ func gpuFillPCIDevice(ctx *context.Context, cards []*GraphicsCard) {
 // system is not a NUMA system, the Node field will be set to nil.
 func gpuFillNUMANodes(ctx *context.Context, cards []*GraphicsCard) {
 	paths := linuxpath.New(ctx)
-	topo, err := topology.NewWithContext(ctx)
+	topo, err := topology.New(context.WithContext(ctx))
 	if err != nil {
 		// Problem getting topology information so just set the graphics card's
 		// node to nil

pkg/option/option.go

@@ -113,7 +113,7 @@ func EnvOrDefaultTools() bool {
 type Option struct {
 	// To facilitate querying of sysfs filesystems that are bind-mounted to a
 	// non-default root mountpoint, we allow users to set the GHW_CHROOT environ
-	// vairable to an alternate mountpoint. For instance, assume that the user of
+	// variable to an alternate mountpoint. For instance, assume that the user of
 	// ghw is a Golang binary being executed from an application container that has
 	// certain host filesystems bind-mounted into the container at /host. The user
 	// would ensure the GHW_CHROOT environ variable is set to "/host" and ghw will
@@ -133,6 +133,11 @@ type Option struct {
 	// PathOverrides optionally allows to override the default paths ghw uses internally
 	// to learn about the system resources.
 	PathOverrides PathOverrides
+
+	// Context may contain a pointer to a `Context` struct that is constructed
+	// during a call to the `context.WithContext` function. Only used internally.
+	// This is an interface to get around recursive package import issues.
+	Context interface{}
 }
 
 // SnapshotOptions contains options for handling of ghw snapshots
@@ -202,6 +207,8 @@ func WithPathOverrides(overrides PathOverrides) *Option {
 // a debug/troubleshoot aid more something users wants to do regularly.
 // Hence we allow that only via the environment variable for the time being.
 
+// Merge accepts one or more Options and merges them together, returning the
+// merged Option
 func Merge(opts ...*Option) *Option {
 	merged := &Option{}
 	for _, opt := range opts {
@@ -221,6 +228,9 @@ func Merge(opts ...*Option) *Option {
 		if opt.PathOverrides != nil {
 			merged.PathOverrides = opt.PathOverrides
 		}
+		if opt.Context != nil {
+			merged.Context = opt.Context
+		}
 	}
 	// Set the default value if missing from mergeOpts
 	if merged.Chroot == nil {

pkg/option/option_test.go

@@ -93,7 +93,7 @@ func TestOption(t *testing.T) {
 				option.WithChroot("/my/chroot/dir"),
 				option.WithSnapshot(option.SnapshotOptions{
 					Path:      "/my/snapshot/dir",
-					Root:      stringPtr("/my/overriden/chroot/dir"),
+					Root:      stringPtr("/my/overridden/chroot/dir"),
 					Exclusive: true,
 				}),
 			},
@@ -102,7 +102,7 @@ func TestOption(t *testing.T) {
 				Chroot: stringPtr("/my/chroot/dir"),
 				Snapshot: &option.SnapshotOptions{
 					Path:      "/my/snapshot/dir",
-					Root:      stringPtr("/my/overriden/chroot/dir"),
+					Root:      stringPtr("/my/overridden/chroot/dir"),
 					Exclusive: true,
 				},
 			},

pkg/pci/pci.go

@@ -155,27 +155,35 @@ func (i *Info) String() string {
 // 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) {
-	return NewWithContext(context.New(opts...))
-}
-
-// NewWithContext returns a pointer to an Info struct that contains information about
-// the PCI devices on the host system. Use this function when you want to consume
-// the topology package from another package (e.g. gpu)
-func NewWithContext(ctx *context.Context) (*Info, error) {
+	merged := option.Merge(opts...)
+	ctx := context.New(merged)
 	// by default we don't report NUMA information;
 	// we will only if are sure we are running on NUMA architecture
-	arch := topology.ARCHITECTURE_SMP
-	topo, err := topology.NewWithContext(ctx)
-	if err == nil {
-		arch = topo.Architecture
-	} else {
-		ctx.Warn("error detecting system topology: %v", err)
-	}
 	info := &Info{
-		arch: arch,
+		arch: topology.ARCHITECTURE_SMP,
 		ctx:  ctx,
 	}
-	if err := ctx.Do(info.load); err != nil {
+
+	// we do this trick because we need to make sure ctx.Setup() gets
+	// a chance to run before any subordinate package is created reusing
+	// our context.
+	loadDetectingTopology := func() error {
+		topo, err := topology.New(context.WithContext(ctx))
+		if err == nil {
+			info.arch = topo.Architecture
+		} else {
+			ctx.Warn("error detecting system topology: %v", err)
+		}
+		return info.load()
+	}
+
+	var err error
+	if context.Exists(merged) {
+		err = loadDetectingTopology()
+	} else {
+		err = ctx.Do(loadDetectingTopology)
+	}
+	if err != nil {
 		return nil, err
 	}
 	return info, nil

pkg/pci/pci_linux_test.go

@@ -152,7 +152,7 @@ func TestPCIMalformedModalias(t *testing.T) {
 	var dev *pci.Device
 	dev = info.ParseDevice("0000:00:01.0", "pci:junk")
 	if dev != nil {
-		t.Fatalf("Parsed succesfully junk data")
+		t.Fatalf("Parsed successfully junk data")
 	}
 
 	dev = info.ParseDevice("0000:00:01.0", "pci:v00008086d00005916sv000017AAsd0000224Bbc03sc00i00extrajunkextradataextraextra")