Conversation
|
This was previously discussed a bit in:
Currently, multiple implementations of this functionality exist, and they all tend Alternatively, we can try add something like this to golang.org/x/sys/unix, |
|
Speaking somewhat selfishly, I do kind of prefer the slice-based approach I have in cyphar/filepath-securejoin#64 now because it works for pre-3.0 kernels ("2.6" is not a useful version check) but it also seems incredibly unlikely anyone is going to be using this package for pre-3.0 kernels (though, programs might be run in the linux26 personality -- but version checks there are kind of useless for >5.0 kernels). For cyphar/filepath-securejoin we also have an additional restriction -- the code needs to work on Go 1.18 so that people can backport cyphar/filepath-securejoin to old release branches without needing to upgrade their Go compiler (we ran into this issue last year with podman). |
You're right, I totally missed that this code here doesn't allow to check for x.y.z versions, I guess because there was no need to do so.
This is not a problem, I pushed the updated version which works with Go 1.18 (a few other packages in this repo also support Go 1.18). |
|
I guess I can also change to a slice instead of two numbers. |
|
@cyphar I guess I like your implementation better, feel free to submit it for inclusion here (maybe with an "internal/compat" package, too?) |
|
I recall we had some packages in moby as well for kernel versions (although they went beyond just Linux); https://github.com/moby/moby/blob/v2.0.0-beta.0/pkg/parsers/kernel/kernel.go And a minimal (non-exported) implementation in the profiles package for seccomp; we could also consider exposing it as a package in that / those modules? https://github.com/moby/profiles/blob/seccomp/v0.1.0/seccomp/kernel_linux.go Wondering if there's any interfaces that Go consumes nowadays with their work on generics (e.g. the version defined as a string with a |
|
@thaJeztah FYI the one in runc came from containerd which came from the Docker seccomp one. Personally I agree with @kolyshkin that the one we have right now in runc is overengineered (and uses incredibly confusing terminology). The seccomp one you linked is very similar to the runc/containerd one. The new Docker one is a little more useful in some areas but I still think it's a bit overcomplicated. I don't think you generally need to know the trailing As for generics, there is |
|
Yes, I agree that some of the existing implementations took it too far; perhaps it seemed like a good idea at some point, but the whole KernelVersion structs as a requirement in various places made it just cumbersome to use; one of the reasons I moved it internal to try to prevent further spreading. And, yes, for the Linux kernel, I think major.minor (for better words, ISTR they're not strictly named that, which is where I think the What if we would make the return a typed integer? type Version intThen bit-shift the major so that we have a single, comparable int for the version? return Version(values[0]<<16 | values[1])For convenience we could add Major and Minor methods; func (v Version) Major() int {
return int(v) >> 16
}
func (v Version) Minor() int {
return int(v) & 0xFFFF
}Quick write up of that idea; diff --git a/uname/kernel_version_ge.go b/uname/kernel_version_ge.go
index 78e2ff0..d2633c9 100644
--- a/uname/kernel_version_ge.go
+++ b/uname/kernel_version_ge.go
@@ -5,7 +5,5 @@ package uname
// KernelVersionGE checks if the running kernel version
// is greater than or equal to the provided version.
func KernelVersionGE(x, y int) bool {
- xx, yy := KernelVersion()
-
- return xx > x || (xx == x && yy >= y)
+ return KernelVersion() >= Version(x<<16|y)
}
diff --git a/uname/kernel_version_ge_test.go b/uname/kernel_version_ge_test.go
index 4d53950..133e63f 100644
--- a/uname/kernel_version_ge_test.go
+++ b/uname/kernel_version_ge_test.go
@@ -7,7 +7,8 @@ import (
)
func TestKernelVersionGE(t *testing.T) {
- major, minor := KernelVersion()
+ v := KernelVersion()
+ major, minor := v.Major(), v.Minor()
t.Logf("Running on kernel %d.%d", major, minor)
tests := []struct {
@@ -55,6 +56,7 @@ func TestKernelVersionGE(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
+
got := KernelVersionGE(tt.x, tt.y)
if got != tt.want {
t.Errorf("KernelVersionGE(%d, %d): got %v, want %v", tt.x, tt.y, got, tt.want)
diff --git a/uname/kernel_version_linux.go b/uname/kernel_version_linux.go
index 6b0bb01..4cb5aa5 100644
--- a/uname/kernel_version_linux.go
+++ b/uname/kernel_version_linux.go
@@ -8,13 +8,23 @@ import (
"syscall"
)
+type Version int
+
+func (v Version) Major() int {
+ return int(v) >> 16
+}
+
+func (v Version) Minor() int {
+ return int(v) & 0xFFFF
+}
+
// KernelVersion returns major and minor kernel version numbers
// parsed from the [syscall.Uname] Release field, or (0, 0) if
// the version can't be obtained or parsed.
-func KernelVersion() (major, minor int) {
+func KernelVersion() Version {
var uname syscall.Utsname
if err := syscall.Uname(&uname); err != nil {
- return
+ return 0
}
var (
@@ -36,5 +46,5 @@ func KernelVersion() (major, minor int) {
}
}
- return values[0], values[1]
+ return Version(values[0]<<16 | values[1])
}
diff --git a/uname/kernel_version_other.go b/uname/kernel_version_other.go
index e190481..481dfd6 100644
--- a/uname/kernel_version_other.go
+++ b/uname/kernel_version_other.go
@@ -6,6 +6,6 @@
package uname
-func KernelVersion() (major int, minor int) {
- return 0, 0
+func KernelVersion() Version {
+ return 0
}
diff --git a/uname/kernel_version_test.go b/uname/kernel_version_test.go
index c38fa45..b515bfd 100644
--- a/uname/kernel_version_test.go
+++ b/uname/kernel_version_test.go
@@ -7,7 +7,8 @@ import (
)
func TestKernelVersion(t *testing.T) {
- x, y := KernelVersion()
+ v := KernelVersion()
+ x, y := v.Major(), v.Minor()
t.Logf("KernelVersion: %d.%d (GOOS: %s)", x, y, runtime.GOOS)
switch runtime.GOOS {
case "linux": |
|
The single version number approach is what a lot of C programs use in general for version checks (though usually they multiply by 100 or something rather than bit-shifting, to make it easier to debug values). I think (like most C patterns) it's a little too easy to shoot yourself in the foot, but at a push I wouldn't be against it either. (Not sure if the interface would be super nice to use either.) |
OTOH this code is extremely simple, it's just a screenful of code and no extra dependencies (other than x/sys/unix), while #205 has some additional ones for testing, and compat layer for generics, which feels like a lot already. I am aiming for something extremely simple here (but don't want to go with bit shifting or Any opinions? |
uname/kernel_version_ge.go
Outdated
| // Copyright 2025 The uname Authors. | ||
|
|
There was a problem hiding this comment.
If we add copyright headers (which we really should start doing in other modules as well), should we also start adding a license header to go with it?
I think in this case, the Module will be Apache 2 licensed, with the forked code being relicensed.
For the Apache 2 licenses I'm always a bit sad about the amount of boilerplating that results from it; more so if code gets moved around between projects (so now accumulating a whole bunch of headers).
While the Apache 2 guidelines still mentions "take this template and apply it to each file", and/or add a Notice file I THINK (but IANAL), SPDX headers are generally acceptable now, which ... definitely could make things much less verbose;
So for "new" files, probably something like:
// SPDX-FileCopyrightText: 2025 The uname Authors.
// SPDX-License-Identifier: Apache-2.0And for the files we forked from Go;
// SPDX-FileCopyrightText: 2022 The Go Authors
// SPDX-FileCopyrightText: 2025 The uname Authors.
// SPDX-License-Identifier: Apache-2.0
// Copyright 2022 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE.BSD file.There was a problem hiding this comment.
I've added proper copyright headers (KernelVersionGE is now included so it's also copyright by Go authors).
| // KernelVersionGE checks if the running kernel version | ||
| // is greater than or equal to the provided version. | ||
| func KernelVersionGE(x, y int) bool { | ||
| xx, yy := KernelVersion() |
There was a problem hiding this comment.
Not sure how much changes we want to make to the forked files, but if we're open to making changes, then I usually like calling non-exported functions when used internally, and have a single exported "implementation" (which can be backed by a non-exported platform-specific one); it can make it more clear / discoverable what code is used, and what not., and having a single exported function gives a good place to maintain the GoDoc.
So along these lines;
// KernelVersionGE checks if the running kernel version
// is greater than or equal to the provided version.
func KernelVersionGE(x, y int) bool {
xx, yy := kernelVersion()
// etc.The platform-agnostic file would have the exported
// KernelVersion returns major and minor kernel version numbers
// parsed from the [syscall.Uname] Release field, or (0, 0) if
// the version can't be obtained or parsed.
func KernelVersion() (major, minor int) {
return kernelVersion()
72abf7b to
a63f9d5
Compare
uname/kernel_version_ge_test.go
Outdated
| @@ -0,0 +1,72 @@ | |||
| // SPDX-FileCopyrightText: 2025 The Go Authors | |||
| // SPDX-FileCopyrightText: 2025 The uname Authors. | |||
There was a problem hiding this comment.
Thanks! Looks like you picked uname here, and moby/sys for some of the other ones 🙈
This is a tiny package to check the Linux kernel version. It is taken from the golang sources (src/internal/syscall/unix), with the following changes: - removed irrelevant parts, including FreeBSD and Solaris kernelVersion implementations; - added a tiny test case for KernelVersion; - added some documentation; - added proper copyright headers. The code is covered by the "BSD 3-clause" license, which is compatible with Apache license. The history of the code before the fork can be seen here (oldest first): - https://go-review.googlesource.com/c/go/+/424896 - https://go-review.googlesource.com/c/go/+/427675 - https://go-review.googlesource.com/c/go/+/427676 - https://go-review.googlesource.com/c/go/+/700796 This is aimed to replace the relevant containerd package (github.com/containerd/containerd/pkg/kernelversion) and its forks. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
maybe we should rename |
|
Yeah; was considering that as well. It could be I recall we also have a package in docker / moby, which provides this, which also supports other OSes (Windows, macOS); so if such functionality would be OK to include in future, then making the name more generic would definitely be something to consider. https://pkg.go.dev/github.com/docker/docker@v28.5.2+incompatible/pkg/parsers/kernel |
This is a tiny package to check the Linux kernel version. It is taken
from the golang sources (src/internal/syscall/unix), with the irrelevant
parts, including FreeBSD and Solaris implementations, removed.
The code is covered by the "BSD 3-clause" license, which is compatible
with Apache license.
The history of the code before the fork can be seen here (oldest first):
On top of that, this PR includes:
KernelVersion;KernelVersionGEfunction (same as inhttps://go-review.googlesource.com/c/go/+/700796).
This is aimed to replace the relevant containerd package
(github.com/containerd/containerd/pkg/kernelversion) and its forks.
The difference is smaller code and simpler API.
Usage example: