Modernize sources, add golangci-lint #466

kolyshkin · 2025-08-13T01:41:30Z

This can be split into a few major blocks:

Modernize source code to use facilities available in Go 1.22+.
Fix issues spotted by golangci-lint (in a minimal configuration).
Gofumpt the code.
Add golangci-lint to CI, remove old stuff (gofmt / go vet).

I tried hard to minimize the changes and be conservative, but might have failed in a few places.

Copilot

Pull Request Overview

This PR modernizes the go-systemd codebase to leverage Go 1.22+ features while adding golangci-lint static analysis and code formatting. The changes focus on updating syntax, fixing linter issues, and standardizing the codebase.

Modernizes Go syntax using newer language features (range over integers, simplified composite literals)
Replaces deprecated ioutil with os package functions and updates build constraints syntax
Adds golangci-lint configuration and GitHub Actions workflow while removing manual fmt/vet checks

Reviewed Changes

Copilot reviewed 46 out of 46 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
util/*.go	Remove old build constraint syntax, replace ioutil with os.ReadFile, simplify variable declarations
unit/*.go	Use modern range syntax, simplified composite literals, replace ioutil with io package
sdjournal/*.go	Update package documentation, use modern syntax, improve test helpers
dbus/*.go	Update package documentation, use modern syntax, fix variable naming
journal/*.go	Replace ioutil with os package, update build constraints
activation/*.go	Improve error handling in tests, use modern syntax
.golangci.yml	Add golangci-lint configuration with minimal settings
.github/workflows/go.yml	Replace manual fmt/vet with golangci-lint action

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-13T02:11:02Z

unit/deserialize.go

 		}

-		l.buf.UnreadRune()
+		_ = l.buf.UnreadRune()


Consider handling the error from UnreadRune() instead of discarding it, as this could indicate a serious issue with the buffer state.

Suggested change

_ = l.buf.UnreadRune()

if err := l.buf.UnreadRune(); err != nil {

return nil, err

}

Copilot · 2025-08-13T02:11:02Z

dbus/subscription.go

 			case "org.freedesktop.systemd1.Manager.JobRemoved":
 				unitName := signal.Body[2].(string)
-				c.sysobj.Call("org.freedesktop.systemd1.Manager.GetUnit", 0, unitName).Store(&unitPath)
+				_ = c.sysobj.Call("org.freedesktop.systemd1.Manager.GetUnit", 0, unitName).Store(&unitPath)


Consider handling the error from the D-Bus call instead of discarding it, as this could indicate communication issues with systemd.

Suggested change

_ = c.sysobj.Call("org.freedesktop.systemd1.Manager.GetUnit", 0, unitName).Store(&unitPath)

err := c.sysobj.Call("org.freedesktop.systemd1.Manager.GetUnit", 0, unitName).Store(&unitPath)

if err != nil {

log.Printf("failed to get unit path for %s: %v", unitName, err)

continue

}

Copilot · 2025-08-13T02:11:02Z

journal/journal_unix.go

 		 */
 		fmt.Fprintln(w, name)
-		binary.Write(w, binary.LittleEndian, uint64(len(value)))
+		_ = binary.Write(w, binary.LittleEndian, uint64(len(value)))


Consider handling the error from binary.Write() instead of discarding it, as this could indicate issues with writing journal data.

Suggested change

_ = binary.Write(w, binary.LittleEndian, uint64(len(value)))

if err := binary.Write(w, binary.LittleEndian, uint64(len(value))); err != nil {

fmt.Fprintf(os.Stderr, "failed to write variable size for %s: %v\n", name, err)

}

Copilot · 2025-08-13T02:11:03Z

dbus/methods.go

 	var result string
-	dbus.Store(signal.Body, &id, &job, &unit, &result)
+
+	_ = dbus.Store(signal.Body, &id, &job, &unit, &result)


Consider handling the error from dbus.Store() instead of discarding it, as this could indicate malformed D-Bus signals.

Suggested change

_ = dbus.Store(signal.Body, &id, &job, &unit, &result)

if err := dbus.Store(signal.Body, &id, &job, &unit, &result); err != nil {

log.Printf("dbus: failed to decode signal body in jobComplete: %v", err)

return

}

Copilot · 2025-08-13T02:11:03Z

activation/files_unix.go

+		defer func() {
+			_ = os.Unsetenv("LISTEN_PID")
+			_ = os.Unsetenv("LISTEN_FDS")
+			_ = os.Unsetenv("LISTEN_FDNAMES")


Consider handling the error from os.Unsetenv() instead of discarding it, as this could indicate permission issues or other environment problems.

Suggested change

_ = os.Unsetenv("LISTEN_FDNAMES")

if err := os.Unsetenv("LISTEN_PID"); err != nil {

log.Printf("failed to unset LISTEN_PID: %v", err)

}

if err := os.Unsetenv("LISTEN_FDS"); err != nil {

log.Printf("failed to unset LISTEN_FDS: %v", err)

}

if err := os.Unsetenv("LISTEN_FDNAMES"); err != nil {

log.Printf("failed to unset LISTEN_FDNAMES: %v", err)

}

Luap99

LGTM mostly, only a few comments

Luap99 · 2025-08-13T09:14:15Z

unit/doc.go

@@ -0,0 +1,26 @@
+// Copyright 2015 CoreOS, Inc.


What should do about the copyright header?
Copyright 2015 CoreOS, Inc. doesn't seem to make sense anymore.

Or just drop it entirely from the files? I don't think it is required, at least our other projects don't include it every file either.

Yes, as far as I know there's no requirement to have a copyright in every source file. Will remove.

Luap99 · 2025-08-13T09:17:37Z

unit/deserialize.go

+// ErrLineTooLong gets returned when a line is too long for systemd to handle.
+var ErrLineTooLong = fmt.Errorf("line too long (max %d bytes)", SYSTEMD_LINE_MAX)


I don't mind that change but it seems unrelated to this commit.

My bad, it's gofumpt. Removed from this patch.

Luap99 · 2025-08-13T09:21:45Z

dbus/methods.go

+	// No way to return an error.
+	_ = c.KillUnitWithTarget(ctx, name, All, signal)


I agree with the change but I wonder if we should mark this function as deprecated and let people use KillUnitWithTarget given this here just eats the error silently.

Was thinking the same thing. Amended.

Luap99 · 2025-08-13T09:27:15Z

unit/deserialize.go

 		}

-		l.buf.UnreadRune()
+		_ = l.buf.UnreadRune()


might be worth to a comment here saying that this should never error as we called ReadRune() before

kolyshkin · 2025-08-13T17:25:17Z

@Luap99 addressed all your comments (and some of copilot ones).

Since Go 1.18, any is a type alias for interface{}. Brought to you by modernize -category=efaceany -fix -test ./... Signed-off-by: Kir Kolyshkin <[email protected]>

Since Go 1.22 one can use "for i := range n" as a shortcut for "for i := 0; i < n; i++". Brought to you by modernize -category=rangeint -fix -test ./... Signed-off-by: Kir Kolyshkin <[email protected]>

Also, simplify the checking code. Signed-off-by: Kir Kolyshkin <[email protected]>

Reported by golangci-lint: login1/dbus.go:114:18: func Session.toInterface is unused (unused) func (s Session) toInterface() []any { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Reported by golangci-lint: machine1/dbus_test.go:151:2: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: As of Go 1.20 there is no reason to call Seed with a random value. Programs that call Seed with a known value to get a specific sequence of results should use New(NewSource(seed)) to obtain a local random generator. (staticcheck) rand.Seed(time.Now().UnixNano()) ^ Signed-off-by: Kir Kolyshkin <[email protected]>

The io/ioutil package is deprecated since Go 1.16. This patch replaces - ioutil.TempDir -> t.TempDir (in tests) - ioutil.TempFile -> os.CreateTemp - ioutil.ReadFile -> os.ReadFile - ioutil.ReadAll -> io.ReadAll Signed-off-by: Kir Kolyshkin <[email protected]>

Fixes the following govet warning: dbus/methods_test.go:1449:26: nilness: tautological condition: non-nil != nil (govet) if tch == nil || (tch != nil && tch.Name == target && tch.ActiveState != "active") { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

Setenv is available for tests since go 1.17. Technically, this changes the test logic a bit because instead of using Unsetenv, tests are now using Setenv with empty value. Practically, this makes no difference as the code being tested treats empty (but set) value the same way as unset. Signed-off-by: Kir Kolyshkin <[email protected]>

...instead of \"%s\". Signed-off-by: Kir Kolyshkin <[email protected]>

Let's put common testing code into functions, and also use t.Cleanup, t.Helper, and t.Fatalf instead of returning an error. This makes the code smaller, and (indirectly) fixes a bunch of linter warnings like these: sdjournal/journal_test.go:225:60: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint) return nil, nil, fmt.Errorf("Error opening journal: %s", err) ^ sdjournal/journal_test.go:157:15: Error return value of `j.Close` is not checked (errcheck) defer j.Close() ^ sdjournal/journal_test.go:165:3: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck) t.Fatalf(err.Error()) ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Like this: dbus/subscription_test.go:104:2: SA5001: should check error returned from New() before deferring conn.Close() (staticcheck) defer conn.Close() ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Like this one: activation/files_test.go:56:3: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck) t.Fatalf(err.Error()) ^ Signed-off-by: Kir Kolyshkin <[email protected]>

1. Use io.Reader to generalise connectStringWritten and drop connectStringWrittenNet. 2. Don't return anything as the return value is never used. 3. Use t.Helper. Signed-off-by: Kir Kolyshkin <[email protected]>

unit/deserialize.go:96:31: SA4001: &*x will be simplified to x. It will not copy x. (staticcheck) options = append(options, &(*opt)) ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

dbus/methods.go:871:16: ST1016: methods on the same type should have the same receiver name (seen 1x "conn", 92x "c") (staticcheck) Signed-off-by: Kir Kolyshkin <[email protected]>

Co-authored-by: Claude Code Signed-off-by: Kir Kolyshkin <[email protected]>

Fix the following warning: SA4004: the surrounding loop is unconditionally terminated Indeed, the for loop here only iterates once, and thus the code is slightly hard to read. Reimplement the same logic without using loop. While at it, simplify the check for \\ at EOL by using bytes.HasSuffix. Co-authored-by: Claude Code Signed-off-by: Kir Kolyshkin <[email protected]>

sdjournal/journal_test.go:350:34: SA1024: cutset contains duplicate characters (staticcheck) if got[1] != strings.Trim(want, delim) { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

This fixes the following staticcheck warning: journal/journal_unix.go:216:6: QF1001: could apply De Morgan's law (staticcheck) if !(('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || c == '_') { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Indeed, we should not use/test deprecated methods. This fixes the following warnings: machine1/dbus_test.go:51:19: SA1019: sd_dbus.New is deprecated: use NewWithContext instead. (staticcheck) dbusConn, err := sd_dbus.New() ^ machine1/dbus_test.go:56:26: SA1019: dbusConn.GetServiceProperty is deprecated: use GetServicePropertyContext instead. (staticcheck) mainPIDProperty, err := dbusConn.GetServiceProperty(testServiceName, "MainPID") ^ Signed-off-by: Kir Kolyshkin <[email protected]>

This one: activation/files_unix.go:42:20: Error return value of `os.Unsetenv` is not checked (errcheck) Since - current unix implementation of os.Unsetenv never returns an error; - there is no way to return an error; let's just ignore it. Signed-off-by: Kir Kolyshkin <[email protected]>

There is no way to return an error from KillUnitContext, and we already have KillUnitWithTarget which returns an error. - explicitly ignore the error in KillUnitContext; - deprecate KillUnitContext in favor of KillUnitWithTarget; - fix the KillUnit deprecation warning to point to KillUnitWithTarget; - improve KillUnitWithTarget and Who docs. Signed-off-by: Kir Kolyshkin <[email protected]>

Ignore a few warnings like this: Error return value of `xxx` is not checked (errcheck) In all cases either there's no way to return an error, or (it seems) the code is working on a "best effort" basis. Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

Add a helper to run sleep process with a proper cleanup, so we don't forget to call wait on a process we ran, and check for any errors. Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

1. Make use of t which was unused before. 2. Ignore stopUnit return value. 3. Check os.Remove return value (which can fail the test early). Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

Gofumpt is a superset of gofmt, enabling some extra rules resulting, for the most part, in a more readable code. This replaces gofmt (which will be removed in the next commit). Signed-off-by: Kir Kolyshkin <[email protected]>

Brought to you by go fix ./... Signed-off-by: Kir Kolyshkin <[email protected]>

This also removes go vet and gofmt, as those are built into golangci-lint. Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

Luap99

LGTM

kolyshkin force-pushed the modernize branch 2 times, most recently from 7d5ea6e to e407489 Compare August 13, 2025 01:46

kolyshkin marked this pull request as ready for review August 13, 2025 02:09

kolyshkin requested review from Copilot and Luap99 August 13, 2025 02:09

Copilot AI reviewed Aug 13, 2025

View reviewed changes

Luap99 reviewed Aug 13, 2025

View reviewed changes

kolyshkin force-pushed the modernize branch from 84fdfbe to acdf3e2 Compare August 13, 2025 17:18

kolyshkin added 21 commits August 13, 2025 10:25

Replace interface{} with any

9ee5faf

Since Go 1.18, any is a type alias for interface{}. Brought to you by modernize -category=efaceany -fix -test ./... Signed-off-by: Kir Kolyshkin <[email protected]>

Use for range for integers

07efb51

Since Go 1.22 one can use "for i := range n" as a shortcut for "for i := 0; i < n; i++". Brought to you by modernize -category=rangeint -fix -test ./... Signed-off-by: Kir Kolyshkin <[email protected]>

sdjournal/journal_test.go: use slices.Contains

6c64a8d

Also, simplify the checking code. Signed-off-by: Kir Kolyshkin <[email protected]>

login1: remove unused method

376e0cd

Reported by golangci-lint: login1/dbus.go:114:18: func Session.toInterface is unused (unused) func (s Session) toInterface() []any { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Remove io/ioutil usage

1f212c8

The io/ioutil package is deprecated since Go 1.16. This patch replaces - ioutil.TempDir -> t.TempDir (in tests) - ioutil.TempFile -> os.CreateTemp - ioutil.ReadFile -> os.ReadFile - ioutil.ReadAll -> io.ReadAll Signed-off-by: Kir Kolyshkin <[email protected]>

dbus: fix some godoc comments

d168ac7

Signed-off-by: Kir Kolyshkin <[email protected]>

journal,sdjournal: use %q in tests

3cbff4d

...instead of \"%s\". Signed-off-by: Kir Kolyshkin <[email protected]>

dbus: fix SA5001 linter warning in tests

d997b93

Like this: dbus/subscription_test.go:104:2: SA5001: should check error returned from New() before deferring conn.Close() (staticcheck) defer conn.Close() ^ Signed-off-by: Kir Kolyshkin <[email protected]>

activation: simplify/unify connectStringWritten

adf9320

1. Use io.Reader to generalise connectStringWritten and drop connectStringWrittenNet. 2. Don't return anything as the return value is never used. 3. Use t.Helper. Signed-off-by: Kir Kolyshkin <[email protected]>

unit: fix SA4001 staticcheck warning

4dcd007

unit/deserialize.go:96:31: SA4001: &*x will be simplified to x. It will not copy x. (staticcheck) options = append(options, &(*opt)) ^ Signed-off-by: Kir Kolyshkin <[email protected]>

machine1: fix some godoc strings

5586a97

Signed-off-by: Kir Kolyshkin <[email protected]>

dbus: fix ST1016 staticcheck warning

a196f27

dbus/methods.go:871:16: ST1016: methods on the same type should have the same receiver name (seen 1x "conn", 92x "c") (staticcheck) Signed-off-by: Kir Kolyshkin <[email protected]>

unit: add package doc

9317ea5

Co-authored-by: Claude Code Signed-off-by: Kir Kolyshkin <[email protected]>

sdjournal: fix SA1024 staticcheck warning

b83d8a7

sdjournal/journal_test.go:350:34: SA1024: cutset contains duplicate characters (staticcheck) if got[1] != strings.Trim(want, delim) { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

journal: apply De Morgan law

456ef98

This fixes the following staticcheck warning: journal/journal_unix.go:216:6: QF1001: could apply De Morgan's law (staticcheck) if !(('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || c == '_') { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin added 13 commits August 13, 2025 10:25

activation: fix errcheck warnings in tests

1151292

Signed-off-by: Kir Kolyshkin <[email protected]>

dbus: avoid zombies in test

9102adf

Add a helper to run sleep process with a proper cleanup, so we don't forget to call wait on a process we ran, and check for any errors. Signed-off-by: Kir Kolyshkin <[email protected]>

dbus: TestFreezer: check runStopUnit error

f82fcb4

Signed-off-by: Kir Kolyshkin <[email protected]>

dbus: fix stopUnit test helper

1313182

1. Make use of t which was unused before. 2. Ignore stopUnit return value. 3. Check os.Remove return value (which can fail the test early). Signed-off-by: Kir Kolyshkin <[email protected]>

Fix some "error value not checked" linter warnings

b9c5324

Signed-off-by: Kir Kolyshkin <[email protected]>

Gofumpt the code

c6dae46

Gofumpt is a superset of gofmt, enabling some extra rules resulting, for the most part, in a more readable code. This replaces gofmt (which will be removed in the next commit). Signed-off-by: Kir Kolyshkin <[email protected]>

Remove obsoleted build tags

c763162

Brought to you by go fix ./... Signed-off-by: Kir Kolyshkin <[email protected]>

ci: enable golangci-lint

08292ea

This also removes go vet and gofmt, as those are built into golangci-lint. Signed-off-by: Kir Kolyshkin <[email protected]>

scripts/ci-runner: less Fedora packages

60c4c55

Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the modernize branch from acdf3e2 to 60c4c55 Compare August 13, 2025 17:25

kolyshkin requested a review from Luap99 August 13, 2025 17:25

Luap99 approved these changes Aug 13, 2025

View reviewed changes

Luap99 merged commit 446c54a into coreos:main Aug 13, 2025
15 checks passed

kolyshkin mentioned this pull request Sep 16, 2025

daemon: Adds SdNotifyBarrier #343

Closed

-		_ = l.buf.UnreadRune()
+		if err := l.buf.UnreadRune(); err != nil {
+			return nil, err
+		}

-				_ = c.sysobj.Call("org.freedesktop.systemd1.Manager.GetUnit", 0, unitName).Store(&unitPath)
+				err := c.sysobj.Call("org.freedesktop.systemd1.Manager.GetUnit", 0, unitName).Store(&unitPath)
+				if err != nil {
+					log.Printf("failed to get unit path for %s: %v", unitName, err)
+					continue
+				}

-		_ = binary.Write(w, binary.LittleEndian, uint64(len(value)))
+		if err := binary.Write(w, binary.LittleEndian, uint64(len(value))); err != nil {
+			fmt.Fprintf(os.Stderr, "failed to write variable size for %s: %v\n", name, err)
+		}

-	_ = dbus.Store(signal.Body, &id, &job, &unit, &result)
+	if err := dbus.Store(signal.Body, &id, &job, &unit, &result); err != nil {
+		log.Printf("dbus: failed to decode signal body in jobComplete: %v", err)
+		return
+	}

-			_ = os.Unsetenv("LISTEN_FDNAMES")
+			if err := os.Unsetenv("LISTEN_PID"); err != nil {
+				log.Printf("failed to unset LISTEN_PID: %v", err)
+			}
+			if err := os.Unsetenv("LISTEN_FDS"); err != nil {
+				log.Printf("failed to unset LISTEN_FDS: %v", err)
+			}
+			if err := os.Unsetenv("LISTEN_FDNAMES"); err != nil {
+				log.Printf("failed to unset LISTEN_FDNAMES: %v", err)
+			}

		// ErrLineTooLong gets returned when a line is too long for systemd to handle.
		var ErrLineTooLong = fmt.Errorf("line too long (max %d bytes)", SYSTEMD_LINE_MAX)

		// No way to return an error.
		_ = c.KillUnitWithTarget(ctx, name, All, signal)

Modernize sources, add golangci-lint #466

Modernize sources, add golangci-lint #466

Uh oh!

Conversation

kolyshkin commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Aug 13, 2025

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Aug 13, 2025 •

edited

Loading