From 38b4cf9b57a9a71f1ab0e0935677c5eb529e91e3 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 26 May 2021 09:11:36 +0200 Subject: [PATCH 1/5] context: fix copypasta in the docs Fix wrong comment, likely due to trivial copy/paste mistake. No code changes. Signed-off-by: Francesco Romani --- pkg/context/context.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/context/context.go b/pkg/context/context.go index e5ac3471..addf3584 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -56,7 +56,7 @@ func New(opts ...*option.Option) *Context { return ctx } -// FromEnv returns an Option that has been populated from the environs or +// FromEnv returns a Context that has been populated from the environs or // default options values func FromEnv() *Context { chrootVal := option.EnvOrDefaultChroot() From 39a3cdfcc47c621b39fbdb1927951bdca8d1a094 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 26 May 2021 09:13:22 +0200 Subject: [PATCH 2/5] context: add refcount to allow context reuse The PR https://github.com/jaypipes/ghw/pull/236 fixes a snapshot directory leakage issue, but also highlight a context lifecycle/ownership issue. In general in ghw each package creates its own context, including the cases on which a package is consumed by another, for example `topology` and `pci`. However, in this case, it would make more sense to reuse the context from the parent package, because the two packages are tightly coupled. Before the introduction of the the transparent snapshot support (https://github.com/jaypipes/ghw/pull/202), the above mechanism worked, because each context, each time, was consuming over and over again the same parameters (e.g. environment variables); besides some resource waste, this had no negative effect. When introducing snapshots in the picture, repeatedly unpacking the same snapshot to consume the same data is much more wasteful. So it makes sense now to introduce explicitly the concept of dependent context. To move forward and make the ownership more explicit, we add an internal reference count in context. This is used only to track the context setup/teardown process. When contexts are reused, we always have a parent module -which creates and owns the context- which passes its context to a subordinate module, because the parent wants to consume the service offered by the subordinate. The subordinate (possibly further nested, or more than one) should never attempt Setup() and Teardown(). These are responsabilities of the parent, outer module, which owns the context. It's impractical to conditionally call Setup() and Teardown(), so the functions internally use and check the reference count to do the right thing automatically. The following are are idempotent (and safe and quickly return success) - calling multiple times Setup() on a ready context - calling multiple times Teardown() on a un-ready context Signed-off-by: Francesco Romani --- pkg/context/context.go | 50 +++++++++++++++++--- pkg/context/context_test.go | 91 +++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 6 deletions(-) diff --git a/pkg/context/context.go b/pkg/context/context.go index addf3584..269ebb32 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -22,6 +22,7 @@ type Context struct { PathOverrides option.PathOverrides snapshotUnpackedPath string alert option.Alerter + refCount int } // New returns a Context struct pointer that has had various options set on it @@ -89,8 +90,14 @@ func (ctx *Context) Do(fn func() error) error { // You should call `Setup` just once. It is safe to call `Setup` if you don't make // use of optional extra features - `Setup` will do nothing. func (ctx *Context) Setup() error { + if ctx.RefCount() > 1 { + // someone else came before and fixed things already! + ctx.incRefCount() + return nil + } if ctx.SnapshotPath == "" { - // nothing to do! + // nothing to do + ctx.incRefCount() return nil } @@ -108,26 +115,57 @@ func (ctx *Context) Setup() error { } _, err = snapshot.UnpackInto(ctx.SnapshotPath, root, flags) } - if err != nil { - return err + if err == nil { + ctx.Chroot = root + ctx.incRefCount() + return nil } - ctx.Chroot = root - return nil + // not ready: must try again + return err } // Teardown releases any resource acquired by Setup. // You should always call `Teardown` if you called `Setup` to free any resources // acquired by `Setup`. Check `Do` for more automated management. func (ctx *Context) Teardown() error { + if ctx.RefCount() > 1 { + // too early, the last one will do the cleaning + ctx.decRefCount() + return nil + } if ctx.snapshotUnpackedPath == "" { // if the client code provided the unpack directory, // then it is also in charge of the cleanup. + ctx.decRefCount() + return nil + } + err := snapshot.Cleanup(ctx.snapshotUnpackedPath) + if err == nil { + ctx.decRefCount() return nil } - return snapshot.Cleanup(ctx.snapshotUnpackedPath) + // else need to try again later + return err } func (ctx *Context) Warn(msg string, args ...interface{}) { ctx.alert.Printf("WARNING: "+msg, args...) } + +func (ctx *Context) IsReady() bool { + return ctx.refCount > 0 +} + +// RefCount() must be used only in testing code +func (ctx *Context) RefCount() int { + return ctx.refCount +} + +func (ctx *Context) incRefCount() { + ctx.refCount++ +} + +func (ctx *Context) decRefCount() { + ctx.refCount-- +} diff --git a/pkg/context/context_test.go b/pkg/context/context_test.go index e12d3f74..5cb92e1b 100644 --- a/pkg/context/context_test.go +++ b/pkg/context/context_test.go @@ -40,3 +40,94 @@ func TestSnapshotContext(t *testing.T) { t.Fatalf("Expected the uncompressed dir to be deleted: %s", uncompressedDir) } } + +// nolint: gocyclo +func TestContextReadiness(t *testing.T) { + ctx := context.New() + if ctx.IsReady() { + t.Fatalf("context ready before Setup()") + } + + ctx.Do(func() error { + if !ctx.IsReady() { + t.Fatalf("context NOT ready inside Do()") + } + return nil + }) + + if ctx.IsReady() { + t.Fatalf("context ready after Teardown()") + } +} + +// nolint: gocyclo +func TestContextReadinessNested(t *testing.T) { + ctx := context.New() + if ctx.IsReady() { + t.Fatalf("context ready before Setup()") + } + + ctx.Do(func() error { + if !ctx.IsReady() { + t.Fatalf("context NOT ready inside outer Do()") + } + ctx.Do(func() error { + if !ctx.IsReady() { + t.Fatalf("context NOT ready inside inner Do()") + } + return nil + }) + if !ctx.IsReady() { + t.Fatalf("context NOT ready after inner Do()") + } + return nil + }) + + if ctx.IsReady() { + t.Fatalf("context ready after Teardown() - refcount = %d", ctx.RefCount()) + } +} + +// nolint: gocyclo +func TestContextReadinessDeeplyNested(t *testing.T) { + // we don't expect more nesting than this atm + ctx := context.New() + if ctx.IsReady() { + t.Fatalf("context ready before Setup()") + } + if ctx.RefCount() != 0 { + t.Fatalf("context refcount unexpected value %d", ctx.RefCount()) + } + + ctx.Do(func() error { + if !ctx.IsReady() { + t.Fatalf("context NOT ready inside outer Do()") + } + ctx.Do(func() error { + if !ctx.IsReady() { + t.Fatalf("context NOT ready inside middle Do()") + } + ctx.Do(func() error { + if !ctx.IsReady() { + t.Fatalf("context NOT ready inside inner Do()") + } + return nil + }) + if !ctx.IsReady() { + t.Fatalf("context NOT ready after inner Do()") + } + return nil + }) + if !ctx.IsReady() { + t.Fatalf("context NOT ready after middle Do()") + } + return nil + }) + + if ctx.IsReady() { + t.Fatalf("context ready after Teardown() - refcount = %d", ctx.RefCount()) + } + if ctx.RefCount() != 0 { + t.Fatalf("context refcount unexpected value %d", ctx.RefCount()) + } +} From c78845cecfb495817f9db703afdac3bbffd4d15e Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 26 May 2021 10:21:02 +0200 Subject: [PATCH 3/5] context: add functions to reuse context Add NewWithContext() functions to initialize modules reusing already prepared contexts. This saves resources and, most important, make sure that dependent package have a consistent view with the topmost, coordinating package. IOW, this change is motivated by the need of making the sharing and ownersjip links more evident, rather than resource optimization, which is "just" a nice bonus. Signed-off-by: Francesco Romani --- pkg/baseboard/baseboard.go | 9 ++++++++- pkg/bios/bios.go | 9 ++++++++- pkg/block/block.go | 9 ++++++++- pkg/chassis/chassis.go | 9 ++++++++- pkg/cpu/cpu.go | 9 ++++++++- pkg/gpu/gpu.go | 9 ++++++++- pkg/memory/memory.go | 11 ++++++++++- pkg/net/net.go | 9 ++++++++- pkg/product/product.go | 9 ++++++++- 9 files changed, 74 insertions(+), 9 deletions(-) diff --git a/pkg/baseboard/baseboard.go b/pkg/baseboard/baseboard.go index f503194c..93461678 100644 --- a/pkg/baseboard/baseboard.go +++ b/pkg/baseboard/baseboard.go @@ -57,7 +57,14 @@ func (i *Info) String() string { // New returns a pointer to an Info struct containing information about the // host's baseboard func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to an Info struct containing information about the +// host's baseboard, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err diff --git a/pkg/bios/bios.go b/pkg/bios/bios.go index 85a7c64b..b5abfcf3 100644 --- a/pkg/bios/bios.go +++ b/pkg/bios/bios.go @@ -50,7 +50,14 @@ func (i *Info) String() string { // New returns a pointer to a Info struct containing information // about the host's BIOS func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to a Info struct containing information +// about the host's BIOS, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err diff --git a/pkg/block/block.go b/pkg/block/block.go index 7a2e4e38..d8ce2ffe 100644 --- a/pkg/block/block.go +++ b/pkg/block/block.go @@ -134,7 +134,14 @@ type Info struct { // New returns a pointer to an Info struct that describes the block storage // resources of the host system. func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to an Info struct that describes the block storage +// resources of the host system, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err diff --git a/pkg/chassis/chassis.go b/pkg/chassis/chassis.go index e11f0447..e610fbd8 100644 --- a/pkg/chassis/chassis.go +++ b/pkg/chassis/chassis.go @@ -94,7 +94,14 @@ func (i *Info) String() string { // New returns a pointer to a Info struct containing information // about the host's chassis func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to a Info struct containing information +// about the host's chassis, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err diff --git a/pkg/cpu/cpu.go b/pkg/cpu/cpu.go index 2fa0cd2d..48644b60 100644 --- a/pkg/cpu/cpu.go +++ b/pkg/cpu/cpu.go @@ -117,7 +117,14 @@ type Info struct { // New returns a pointer to an Info struct that contains information about the // CPUs on the host system func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to an Info struct that contains information about the +// CPUs on the host system, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err diff --git a/pkg/gpu/gpu.go b/pkg/gpu/gpu.go index 65864c7e..e14e3efb 100644 --- a/pkg/gpu/gpu.go +++ b/pkg/gpu/gpu.go @@ -56,7 +56,14 @@ type Info struct { // New returns a pointer to an Info struct that contains information about the // graphics cards on the host system func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to an Info struct that contains information about the +// graphics cards on the host system, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err diff --git a/pkg/memory/memory.go b/pkg/memory/memory.go index 93605d2e..54cd19f0 100644 --- a/pkg/memory/memory.go +++ b/pkg/memory/memory.go @@ -34,8 +34,17 @@ type Info struct { Modules []*Module `json:"modules"` } +// New returns a pointer to an Info struct containing information about the +// host's memory. func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to an Info struct containing information about the +// host's memory, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err diff --git a/pkg/net/net.go b/pkg/net/net.go index 8994d112..d225c79d 100644 --- a/pkg/net/net.go +++ b/pkg/net/net.go @@ -49,7 +49,14 @@ type Info struct { // New returns a pointer to an Info struct that contains information about the // network interface controllers (NICs) on the host system func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to an Info struct that contains information about the +// network interface controllers (NICs) on the host system, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err diff --git a/pkg/product/product.go b/pkg/product/product.go index 6a2e1ee0..2daa763b 100644 --- a/pkg/product/product.go +++ b/pkg/product/product.go @@ -73,7 +73,14 @@ func (i *Info) String() string { // New returns a pointer to a Info struct containing information // about the host's product func New(opts ...*option.Option) (*Info, error) { - ctx := context.New(opts...) + return NewWithContext(context.New(opts...)) +} + +// New returns a pointer to a Info struct containing information +// about the host's product, reusing a given context. +// Use this function when you want to consume this package from another, +// ensuring the two see a coherent set of resources. +func NewWithContext(ctx *context.Context) (*Info, error) { info := &Info{ctx: ctx} if err := ctx.Do(info.load); err != nil { return nil, err From 31471909c522514357a603f416bff557312479d0 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Sun, 11 Apr 2021 19:49:18 +0200 Subject: [PATCH 4/5] pci: ensure context ordering the pci package want to use the topology package, and passes its context down to it. However, nontrivial initialization is performed in pci.Info.load(), when snapshots are consumed - most notably in test. Hence, we need to ensure that `topology.WithContext` is called only after context.Setup() is called for the pci context. This is needed because in turn the pci packaged doesn't know (nor it should) if its context is a root (= top level) context, hence if it owns the resources which are passed to the dependent packages. Signed-off-by: Francesco Romani --- pkg/pci/pci.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/pkg/pci/pci.go b/pkg/pci/pci.go index c8176e68..36f54243 100644 --- a/pkg/pci/pci.go +++ b/pkg/pci/pci.go @@ -160,18 +160,22 @@ func New(opts ...*option.Option) (*Info, error) { func NewWithContext(ctx *context.Context) (*Info, error) { // 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. + if err := ctx.Do(func() error { + topo, err := topology.NewWithContext(ctx) + if err == nil { + info.arch = topo.Architecture + } else { + ctx.Warn("error detecting system topology: %v", err) + } + return info.load() + }); err != nil { return nil, err } return info, nil From 59dde1d7d3a07456171e495a583f6992cf453aaa Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 7 Apr 2021 18:35:52 +0200 Subject: [PATCH 5/5] context: detect and report conflicting options `WithChroot` and `WithSnapshot` conflicts to each other, and the users can get surprising results if they try to use them at the same time. Now: chroot is expected to be much more popular than snapshots, being the latter useful mostly for tests or offline debug/troubleshoot, but still the code should detect and handle these conflicts. This is made a bit awkward because the current `context` pkg API. Once we are free to break compatibility, we can make the flow more linear. Signed-off-by: Francesco Romani --- pkg/context/context.go | 13 +++++++++++++ pkg/option/option.go | 7 +++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/pkg/context/context.go b/pkg/context/context.go index 269ebb32..9a44ec4a 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -7,6 +7,8 @@ package context import ( + "fmt" + "github.com/jaypipes/ghw/pkg/option" "github.com/jaypipes/ghw/pkg/snapshot" ) @@ -23,6 +25,7 @@ type Context struct { snapshotUnpackedPath string alert option.Alerter refCount int + err error } // New returns a Context struct pointer that has had various options set on it @@ -54,6 +57,13 @@ func New(opts ...*option.Option) *Context { ctx.PathOverrides = merged.PathOverrides } + // New is not allowed to return error - it would break the established API. + // so the only way out is to actually do the checks here and record the error, + // and return it later, at the earliest possible occasion, in Setup() + if ctx.SnapshotPath != "" && ctx.Chroot != option.DefaultChroot { + // The env/client code supplied a value, but we are will overwrite it when unpacking shapshots! + ctx.err = fmt.Errorf("Conflicting options: chroot %q and snapshot path %q", ctx.Chroot, ctx.SnapshotPath) + } return ctx } @@ -90,6 +100,9 @@ func (ctx *Context) Do(fn func() error) error { // You should call `Setup` just once. It is safe to call `Setup` if you don't make // use of optional extra features - `Setup` will do nothing. func (ctx *Context) Setup() error { + if ctx.err != nil { + return ctx.err + } if ctx.RefCount() > 1 { // someone else came before and fixed things already! ctx.incRefCount() diff --git a/pkg/option/option.go b/pkg/option/option.go index b2db7188..90e57658 100644 --- a/pkg/option/option.go +++ b/pkg/option/option.go @@ -14,7 +14,10 @@ import ( ) const ( - defaultChroot = "/" + DefaultChroot = "/" +) + +const ( envKeyChroot = "GHW_CHROOT" envKeyDisableWarnings = "GHW_DISABLE_WARNINGS" envKeyDisableTools = "GHW_DISABLE_TOOLS" @@ -57,7 +60,7 @@ func EnvOrDefaultChroot() string { if val, exists := os.LookupEnv(envKeyChroot); exists { return val } - return defaultChroot + return DefaultChroot } // EnvOrDefaultSnapshotPath returns the value of the GHW_SNAPSHOT_PATH environs variable