From b5a101d2bf250688e7adda3da88643ae5deae6eb Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Fri, 19 Nov 2021 12:43:44 +0100 Subject: [PATCH 1/9] alias: trivial style fix Makes gofmt happy. Signed-off-by: Francesco Romani --- alias.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/alias.go b/alias.go index 666200bc..2e679a96 100644 --- a/alias.go +++ b/alias.go @@ -25,10 +25,10 @@ import ( type WithOption = option.Option var ( - WithChroot = option.WithChroot - WithSnapshot = option.WithSnapshot - WithAlerter = option.WithAlerter - WithNullAlerter = option.WithNullAlerter + WithChroot = option.WithChroot + WithSnapshot = option.WithSnapshot + WithAlerter = option.WithAlerter + WithNullAlerter = option.WithNullAlerter // match the existing environ variable to minimize surprises WithDisableWarnings = option.WithNullAlerter WithDisableTools = option.WithDisableTools From 8b732cdb00e00f6cc6f4c6817e9fb561ecbbba3c Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 17 Nov 2021 20:02:53 +0100 Subject: [PATCH 2/9] gh actions: add check-format lane Signed-off-by: Francesco Romani --- .github/workflows/go.yml | 16 ++++++++++++++++ hack/check-format.sh | 9 +++++++++ 2 files changed, 25 insertions(+) create mode 100755 hack/check-format.sh diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index d9586348..c0624ca6 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -8,6 +8,22 @@ on: # see: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners jobs: + # tier 0: system-independent checks + format: + runs-on: ubuntu-20.04 + steps: + - uses: actions/checkout@v2 + with: + fetch-depth: 0 + + - name: set up golang + uses: actions/setup-go@v2 + with: + go-version: 1.16 + + - name: format + run: ./hack/check-format.sh + # tier-1 # main development platfotm, gets features first and it's most tested build-ubuntu-2004: diff --git a/hack/check-format.sh b/hack/check-format.sh new file mode 100755 index 00000000..42906f0f --- /dev/null +++ b/hack/check-format.sh @@ -0,0 +1,9 @@ +#!/bin/bash +set -eu + +DIFF=$( gofmt -s -d . ) +if [ -n "${DIFF}" ]; then + echo "${DIFF}" + exit 1 +fi +exit 0 From 35611d6588ba19fb15c1ae575a236c924235c326 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Fri, 19 Nov 2021 13:00:35 +0100 Subject: [PATCH 3/9] lint: remove dead code according to golint-ci, and confirmed by `git grep`, this code was lingering around unused. Let's get rid of it. Signed-off-by: Francesco Romani --- cmd/ghw-snapshot/main.go | 6 ------ pkg/block/block_linux.go | 15 --------------- pkg/pci/pci.go | 7 ------- pkg/util/util.go | 3 +-- 4 files changed, 1 insertion(+), 30 deletions(-) diff --git a/cmd/ghw-snapshot/main.go b/cmd/ghw-snapshot/main.go index 7bc3a176..87d00fd8 100644 --- a/cmd/ghw-snapshot/main.go +++ b/cmd/ghw-snapshot/main.go @@ -21,12 +21,6 @@ import ( ) var ( - // version of application at compile time (-X 'main.version=$(VERSION)'). - version = "(Unknown Version)" - // buildHash GIT hash of application at compile time (-X 'main.buildHash=$(GITCOMMIT)'). - buildHash = "No Git-hash Provided." - // buildDate of application at compile time (-X 'main.buildDate=$(BUILDDATE)'). - buildDate = "No Build Date Provided." // show debug output debug = false // output filepath to save snapshot to diff --git a/pkg/block/block_linux.go b/pkg/block/block_linux.go index 07db1067..235ca6af 100644 --- a/pkg/block/block_linux.go +++ b/pkg/block/block_linux.go @@ -457,18 +457,3 @@ func parseMountEntry(line string) *mountEntry { res.Options = opts return res } - -func partitionMountPoint(paths *linuxpath.Paths, part string) string { - mp, _, _ := partitionInfo(paths, part) - return mp -} - -func partitionType(paths *linuxpath.Paths, part string) string { - _, pt, _ := partitionInfo(paths, part) - return pt -} - -func partitionIsReadOnly(paths *linuxpath.Paths, part string) bool { - _, _, ro := partitionInfo(paths, part) - return ro -} diff --git a/pkg/pci/pci.go b/pkg/pci/pci.go index 7411277d..86cc7b25 100644 --- a/pkg/pci/pci.go +++ b/pkg/pci/pci.go @@ -9,7 +9,6 @@ package pci import ( "encoding/json" "fmt" - "regexp" "github.com/jaypipes/pcidb" @@ -27,12 +26,6 @@ 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})$`, - ) -) - type Device struct { // The PCI address of the device Address string `json:"address"` diff --git a/pkg/util/util.go b/pkg/util/util.go index 9b74d7e7..886bb895 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -17,8 +17,7 @@ import ( ) const ( - UNKNOWN = "unknown" - disableWarningsEnv = "GHW_DISABLE_WARNINGS" + UNKNOWN = "unknown" ) type closer interface { From 0fd4f1f371811161a56f9159a979a1bc5342b2d8 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Fri, 19 Nov 2021 13:03:18 +0100 Subject: [PATCH 4/9] lint: code simplification suggested by golint-ci Mechanical simplifications as suggested by the tool. Signed-off-by: Francesco Romani --- pkg/block/block_linux.go | 7 ++----- pkg/pci/pci_linux_test.go | 2 +- pkg/snapshot/clonetree_net_linux.go | 5 +---- pkg/snapshot/trace.go | 4 +--- pkg/topology/topology_linux.go | 2 +- 5 files changed, 6 insertions(+), 14 deletions(-) diff --git a/pkg/block/block_linux.go b/pkg/block/block_linux.go index 235ca6af..f68666d9 100644 --- a/pkg/block/block_linux.go +++ b/pkg/block/block_linux.go @@ -229,7 +229,7 @@ func diskPartUUID(ctx *context.Context, part string) string { return "" } - if out == nil || len(out) == 0 { + if len(out) == 0 { return "" } @@ -249,10 +249,7 @@ func diskIsRemovable(paths *linuxpath.Paths, disk string) bool { return false } removable := strings.TrimSpace(string(contents)) - if removable == "1" { - return true - } - return false + return removable == "1" } func disks(ctx *context.Context, paths *linuxpath.Paths) []*Disk { diff --git a/pkg/pci/pci_linux_test.go b/pkg/pci/pci_linux_test.go index 5782bc95..b352f413 100644 --- a/pkg/pci/pci_linux_test.go +++ b/pkg/pci/pci_linux_test.go @@ -81,7 +81,7 @@ func TestPCIDeviceRevision(t *testing.T) { }, } for _, tCase := range tCases { - t.Run(fmt.Sprintf("%s", tCase.addr), func(t *testing.T) { + t.Run(tCase.addr, func(t *testing.T) { dev := info.GetDevice(tCase.addr) if dev == nil { t.Fatalf("got nil device for address %q", tCase.addr) diff --git a/pkg/snapshot/clonetree_net_linux.go b/pkg/snapshot/clonetree_net_linux.go index 6c89a6cc..27b27573 100644 --- a/pkg/snapshot/clonetree_net_linux.go +++ b/pkg/snapshot/clonetree_net_linux.go @@ -21,10 +21,7 @@ func ExpectedCloneNetContent() []string { } filterLink := func(linkDest string) bool { - if strings.Contains(linkDest, "devices/virtual/net") { - return false - } - return true + return !strings.Contains(linkDest, "devices/virtual/net") } return cloneContentByClass("net", ifaceEntries, filterNone, filterLink) diff --git a/pkg/snapshot/trace.go b/pkg/snapshot/trace.go index ad8086bd..78c76121 100644 --- a/pkg/snapshot/trace.go +++ b/pkg/snapshot/trace.go @@ -9,9 +9,7 @@ package snapshot var trace func(msg string, args ...interface{}) func init() { - trace = func(msg string, args ...interface{}) { - return - } + trace = func(msg string, args ...interface{}) {} } func SetTraceFunction(fn func(msg string, args ...interface{})) { diff --git a/pkg/topology/topology_linux.go b/pkg/topology/topology_linux.go index 84fbbd2c..6844dd96 100644 --- a/pkg/topology/topology_linux.go +++ b/pkg/topology/topology_linux.go @@ -95,7 +95,7 @@ func distancesForNode(ctx *context.Context, nodeID int) ([]int, error) { } items := strings.Fields(strings.TrimSpace(string(data))) - dists := make([]int, len(items), len(items)) // TODO: can a NUMA cell be offlined? + dists := make([]int, len(items)) // TODO: can a NUMA cell be offlined? for idx, item := range items { dist, err := strconv.Atoi(item) if err != nil { From 00dca3efdd4e4830bdb645ef54bc444ffd227091 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Fri, 19 Nov 2021 13:17:42 +0100 Subject: [PATCH 5/9] snapshot: bubble up errors Let's not swallow errors, even if they are unlikely. This makes the troubleshooting experience a bit easier with very tiny extra maintenance cost. Signed-off-by: Francesco Romani --- cmd/ghw-snapshot/main.go | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/cmd/ghw-snapshot/main.go b/cmd/ghw-snapshot/main.go index 87d00fd8..efd2d5a8 100644 --- a/cmd/ghw-snapshot/main.go +++ b/cmd/ghw-snapshot/main.go @@ -41,19 +41,25 @@ func trace(msg string, args ...interface{}) { fmt.Printf(msg, args...) } -func systemFingerprint() string { +func systemFingerprint() (string, error) { hn, err := os.Hostname() if err != nil { - return "unknown" + return "unknown", err } m := md5.New() - io.WriteString(m, hn) - return fmt.Sprintf("%x", m.Sum(nil)) + _, err = io.WriteString(m, hn) + if err != nil { + return "unknown", err + } + return fmt.Sprintf("%x", m.Sum(nil)), nil } -func defaultOutPath() string { - fp := systemFingerprint() - return fmt.Sprintf("%s-%s-%s.tar.gz", runtime.GOOS, runtime.GOARCH, fp) +func defaultOutPath() (string, error) { + fp, err := systemFingerprint() + if err != nil { + return "unknown", err + } + return fmt.Sprintf("%s-%s-%s.tar.gz", runtime.GOOS, runtime.GOARCH, fp), nil } func execute(cmd *cobra.Command, args []string) error { @@ -69,7 +75,10 @@ func execute(cmd *cobra.Command, args []string) error { } if outPath == "" { - outPath = defaultOutPath() + outPath, err = defaultOutPath() + if err != nil { + return err + } trace("using default output filepath %s\n", outPath) } @@ -77,7 +86,9 @@ func execute(cmd *cobra.Command, args []string) error { } func main() { - rootCmd.Execute() + if err := rootCmd.Execute(); err != nil { + trace("execution failed: %v\n", err) + } } func init() { From 0ec2131cb7592a3d6243f443c101185d7206d3fa Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Fri, 19 Nov 2021 13:19:35 +0100 Subject: [PATCH 6/9] lint: gpu: test: silence lint warning It's test code, we can afford a little extra ugliness to make the lint happy. Signed-off-by: Francesco Romani --- pkg/gpu/gpu_linux_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/gpu/gpu_linux_test.go b/pkg/gpu/gpu_linux_test.go index c1992c69..3d6cdf16 100644 --- a/pkg/gpu/gpu_linux_test.go +++ b/pkg/gpu/gpu_linux_test.go @@ -48,7 +48,9 @@ func TestGPUWithoutNUMANodeInfo(t *testing.T) { if err != nil { t.Fatalf("Unable to unpack %q into %q: %v", workstationSnapshot, tmpRoot, err) } - defer snapshot.Cleanup(tmpRoot) + defer func() { + _ = snapshot.Cleanup(tmpRoot) + }() err = os.Remove(filepath.Join(tmpRoot, "/sys/class/drm/card0/device/numa_node")) if err != nil && !errors.Is(err, os.ErrNotExist) { From bbdebf140d69267c7a50a58d72e4e80ffe86f1a6 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Fri, 19 Nov 2021 13:13:58 +0100 Subject: [PATCH 7/9] context: log teardown errors Let's not lose teardown errors completely. We cannot really recover from teardown errors in `context.Do()`, but we can at least trace them to enable some troubleshooting. Signed-off-by: Francesco Romani --- pkg/context/context.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/context/context.go b/pkg/context/context.go index db8af77b..fb8de528 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -116,7 +116,12 @@ func (ctx *Context) Do(fn func() error) error { if err != nil { return err } - defer ctx.Teardown() + defer func() { + err := ctx.Teardown() + if err != nil { + ctx.Warn("teardown error: %v", err) + } + }() return fn() } From 121e3ffef74edb312c479dde1050129d7ac40847 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Fri, 19 Nov 2021 13:09:04 +0100 Subject: [PATCH 8/9] snapshot: fix pack walk The documentation of the callback (WalkFunc) of `filepath.Walk` reads like: The err argument reports an error related to path, signaling that Walk will not walk into that directory. The function can decide how to handle that error; as described earlier, returning the error will cause Walk to stop walking the entire tree. Here we want to always keep walking, and this is why we ignored the argument. We still want to do this, so let's rename the argument to signal the intent of the code, and to avoid unwanted shadowing with the local variable. Signed-off-by: Francesco Romani --- pkg/snapshot/pack.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/snapshot/pack.go b/pkg/snapshot/pack.go index 8d9bd959..94b5bb69 100644 --- a/pkg/snapshot/pack.go +++ b/pkg/snapshot/pack.go @@ -72,11 +72,12 @@ func PackWithWriter(fw io.Writer, sourceRoot string) error { } func createSnapshot(tw *tar.Writer, buildDir string) error { - return filepath.Walk(buildDir, func(path string, fi os.FileInfo, err error) error { + return filepath.Walk(buildDir, func(path string, fi os.FileInfo, _ error) error { if path == buildDir { return nil } var link string + var err error if fi.Mode()&os.ModeSymlink != 0 { trace("processing symlink %s\n", path) From 1979b8eb6e557fe9d52cf508dc32785b374ad058 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 17 Nov 2021 20:00:30 +0100 Subject: [PATCH 9/9] gh actions: add lint lane let's add tier-0 (static check) lanes, starting with the lint lane featuring golang-ci lint Signed-off-by: Francesco Romani --- .github/workflows/go.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index c0624ca6..7e035454 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -24,6 +24,18 @@ jobs: - name: format run: ./hack/check-format.sh + lint: + runs-on: ubuntu-20.04 + steps: + - name: Check out code + uses: actions/checkout@v2 + + - name: Verify + uses: golangci/golangci-lint-action@v2 + with: + version: v1.41.1 + args: --timeout=15m0s --verbose + # tier-1 # main development platfotm, gets features first and it's most tested build-ubuntu-2004: