mountinfo: BSDs no longer need cgo nor reflect #114

Red54 · 2022-04-19T05:03:37Z

Alternative PR for #113

AkihiroSuda · 2022-04-19T05:22:32Z

mountinfo/mountinfo_bsd.go

-	count := int(C.getmntinfo(&rawEntries, C.MNT_WAIT))
-	if count == 0 {
-		return nil, fmt.Errorf("failed to call getmntinfo")
+	count, err := syscall.Getfsstat(nil, 1 /* MNT_WAIT */)


Could you define const MntWait = 1 ?

I defined MNT_WAIT.

AkihiroSuda · 2022-04-19T05:23:24Z

mountinfo/mountinfo_bsd.go

-	"reflect"
-	"unsafe"
-)
+func getInfo(is []int8) string {


This function name isn't descriptive.
func stringFromInt8Slice([]int8) string might be better.

I renamed it to int8SliceToString.

AkihiroSuda · 2022-04-19T05:23:35Z

Please squash commits

thaJeztah · 2022-04-19T10:34:25Z

Looks like one function is not defined in at least one of the variants

Error: mountinfo_bsd.go:36:16: undeclared name: `getMountinfo` (typecheck)
		mountinfo := getMountinfo(entry)
		             ^
make: *** [lint] Error 1
Error: Process completed with exit code 2.

Red54 · 2022-04-19T10:45:38Z

Looks like one function is not defined in at least one of the variants

Error: mountinfo_bsd.go:36:16: undeclared name: `getMountinfo` (typecheck)
		mountinfo := getMountinfo(entry)
		             ^
make: *** [lint] Error 1
Error: Process completed with exit code 2.

This function is defined in mountinfo/mountinfo_freebsd.go for FreeBSD and Darwin.

And it has passed golangci-lint on FreeBSD.

I think this is a problem with GitHub Actions, but I don't have a MAC, maybe you can have a try.

Red54 · 2022-04-19T13:35:51Z

I reproduced it, and I will fix it.

Red54 · 2022-04-19T15:41:05Z

I renamed mountinfo_freebsd.go to mountinfo_freebsdlike.go, and it got fixed.

mountinfo/mountinfo_bsd.go

mountinfo/mountinfo_freebsdlike.go

Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>

kolyshkin · 2022-04-21T23:51:04Z

mountinfo/mountinfo_openbsd.go

+	mountinfo.Mountpoint = int8SliceToString(entry.F_mntonname[:])
+	mountinfo.FSType = int8SliceToString(entry.F_fstypename[:])
+	mountinfo.Source = int8SliceToString(entry.F_mntfromname[:])


I guess you forgot to switch to unix.ByteSliceToString here.

No, this is correct.

In syscall, freebsd, darwin, and openbsd are all int8 arrays.
https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/syscall/ztypes_freebsd_amd64.go;l=145
https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/syscall/ztypes_darwin_amd64.go;l=100
https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/syscall/ztypes_openbsd_amd64.go;l=117

But in golang.org/x/sys/unix, freebsd and darwin are byte arrays, only openbsd is int8 array.
https://cs.opensource.google/go/x/sys/+/33da011f:unix/ztypes_freebsd_amd64.go;l=129
https://cs.opensource.google/go/x/sys/+/33da011f:unix/ztypes_darwin_amd64.go;l=100
https://cs.opensource.google/go/x/sys/+/33da011f:unix/ztypes_openbsd_amd64.go;l=99

This is why I chose syscall before.

Hmm, weird.

This is probably something that needs to be fixed in x/sys/unix. Something similar to e.g. https://go-review.googlesource.com/c/sys/+/359674 needs to be done.

If you have openbsd running, you can prepare a patch to x/sys/unix similar to the above; if not, I can try to do that or we can maybe kindly ask @tklauser

Okay, I will make a patch to x/sys/unix for OpenBSD.

Submitted.
https://go-review.googlesource.com/c/sys/+/401734

AkihiroSuda · 2022-05-17T17:23:04Z

@kolyshkin @thaJeztah Can we merge this? 🙏

thaJeztah · 2022-05-17T21:38:32Z

I see the intent was to upstream changes to golang.org/x/sys; https://github.com/moby/sys/pull/114/files#r855976616, but looking at that pull request, it looks like there's things to address.

Not sure if that's a blocker for this change (although if golang.org/x/sys will change things and it breaks this code, I guess that's not desirable)

@kolyshkin as you were looking at that part; perhaps you have ideas?

kolyshkin · 2022-05-18T01:28:00Z

So, https://go-review.googlesource.com/c/sys/+/401734 has a comment that needs addressing. I think @Red54 should look into it.

Nevertheless, it's not clear if the change will be accepted, so let's merge this for now.

kolyshkin

LGTM

AkihiroSuda · 2022-05-18T10:38:30Z

@kolyshkin Can we have a new release? 🙏

kolyshkin · 2022-05-18T20:54:10Z

mountinfo/mountinfo_openbsd.go

+	var bs []byte
+	for _, i := range is {
+		if i == 0 {
+			break
+		}
+		bs = append(bs, byte(i))
+	}
+	return string(bs)


Sorry I overlooked it. This function is probably doing some excessive allocations and copying and should be rewritten

I can not think of a better way without unsafe. If you are sure, then I will rewrite it with unsafe.

Rewrited with unsafe.
#117

akhramov · 2022-06-04T16:55:55Z

@kolyshkin re: moby/sys release required for FreeBSD support in nerdctl.

Is there any way to fast track the release? I see it's just the OpenBSD code that is preventing us from moving forward. If that's the case, maybe we could use the older (cgo) implementation for OpenBSD? Would that be viable?

cc @Red54 & @AkihiroSuda

kolyshkin · 2022-06-04T17:52:26Z

@kolyshkin re: moby/sys release required for FreeBSD support in nerdctl.

Is there any way to fast track the release? I see it's just the OpenBSD code that is preventing us from moving forward. If that's the case, maybe we could use the older (cgo) implementation for OpenBSD? Would that be viable?

cc @Red54 & @AkihiroSuda

What is needed to be done is:

bumping x/sys/unix to a version that includes https://go-review.googlesource.com/c/sys/+/407195
removing int8SliceToString implementation, and use unix.ByteSliceToString where it was used.

@Red54 @akhramov feel free to work on this. Once done and merged, we can cut a release.

akhramov · 2022-06-05T11:37:27Z

@kolyshkin thanks! So, something like #118?

Red54 mentioned this pull request Apr 19, 2022

mountinfo: BSDs no longer need cgo #113

Closed

AkihiroSuda reviewed Apr 19, 2022

View reviewed changes

Red54 force-pushed the bsd-no-reflect branch from 58b54b6 to 793af3b Compare April 19, 2022 10:20

AkihiroSuda approved these changes Apr 19, 2022

View reviewed changes

AkihiroSuda requested review from kolyshkin and thaJeztah April 19, 2022 10:27

Red54 force-pushed the bsd-no-reflect branch from 793af3b to bdd2e0c Compare April 19, 2022 15:36

kolyshkin reviewed Apr 21, 2022

View reviewed changes

mountinfo/mountinfo_bsd.go Outdated Show resolved Hide resolved

kolyshkin reviewed Apr 21, 2022

View reviewed changes

mountinfo/mountinfo_freebsdlike.go Outdated Show resolved Hide resolved

mountinfo: BSDs no longer need cgo nor reflect

8dfa55d

Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>

Red54 force-pushed the bsd-no-reflect branch from bdd2e0c to 8dfa55d Compare April 21, 2022 10:06

kolyshkin reviewed Apr 21, 2022

View reviewed changes

AkihiroSuda requested a review from kolyshkin April 26, 2022 04:45

gmshake mentioned this pull request May 7, 2022

[#137] FreeBSD support containerd/nerdctl#361

Merged

kolyshkin approved these changes May 18, 2022

View reviewed changes

kolyshkin merged commit 28ef7ed into moby:main May 18, 2022

Red54 mentioned this pull request May 18, 2022

[#868] FreeBSD support containerd/nerdctl#993

Closed

AkihiroSuda mentioned this pull request May 18, 2022

Update mountinfo to support FreeBSD containerd/nerdctl#1067

Closed

kolyshkin reviewed May 18, 2022

View reviewed changes

Red54 mentioned this pull request May 19, 2022

mountinfo_openbsd: Rewrite int8SliceToString with unsafe #117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mountinfo: BSDs no longer need cgo nor reflect #114

mountinfo: BSDs no longer need cgo nor reflect #114

Red54 commented Apr 19, 2022

AkihiroSuda Apr 19, 2022

Red54 Apr 19, 2022

AkihiroSuda Apr 19, 2022

Red54 Apr 19, 2022

AkihiroSuda commented Apr 19, 2022

thaJeztah commented Apr 19, 2022

Red54 commented Apr 19, 2022

Red54 commented Apr 19, 2022

Red54 commented Apr 19, 2022

kolyshkin Apr 21, 2022

Red54 Apr 22, 2022

kolyshkin Apr 22, 2022

Red54 Apr 22, 2022

Red54 Apr 22, 2022

AkihiroSuda commented May 17, 2022

thaJeztah commented May 17, 2022

kolyshkin commented May 18, 2022

kolyshkin left a comment

AkihiroSuda commented May 18, 2022

kolyshkin May 18, 2022

Red54 May 19, 2022

Red54 May 19, 2022

akhramov commented Jun 4, 2022 •

edited

Loading

kolyshkin commented Jun 4, 2022

akhramov commented Jun 5, 2022

mountinfo: BSDs no longer need cgo nor reflect #114

mountinfo: BSDs no longer need cgo nor reflect #114

Conversation

Red54 commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Apr 19, 2022

thaJeztah commented Apr 19, 2022

Red54 commented Apr 19, 2022

Red54 commented Apr 19, 2022

Red54 commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented May 17, 2022

thaJeztah commented May 17, 2022

kolyshkin commented May 18, 2022

kolyshkin left a comment

Choose a reason for hiding this comment

AkihiroSuda commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhramov commented Jun 4, 2022 • edited Loading

kolyshkin commented Jun 4, 2022

akhramov commented Jun 5, 2022

akhramov commented Jun 4, 2022 •

edited

Loading