Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mountinfo: BSDs no longer need cgo nor reflect #114

Merged
merged 1 commit into from May 18, 2022

Conversation

Red54
Copy link
Contributor

@Red54 Red54 commented Apr 19, 2022

Alternative PR for #113

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 */)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you define const MntWait = 1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defined MNT_WAIT.

"reflect"
"unsafe"
)
func getInfo(is []int8) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to int8SliceToString.

@AkihiroSuda
Copy link
Member

Please squash commits

@thaJeztah
Copy link
Member

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
Copy link
Contributor Author

Red54 commented Apr 19, 2022

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
Copy link
Contributor Author

Red54 commented Apr 19, 2022

I reproduced it, and I will fix it.

@Red54
Copy link
Contributor Author

Red54 commented Apr 19, 2022

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

Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
Comment on lines +18 to +20
mountinfo.Mountpoint = int8SliceToString(entry.F_mntonname[:])
mountinfo.FSType = int8SliceToString(entry.F_fstypename[:])
mountinfo.Source = int8SliceToString(entry.F_mntfromname[:])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda
Copy link
Member

@kolyshkin @thaJeztah Can we merge this? 🙏

@thaJeztah
Copy link
Member

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
Copy link
Collaborator

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.

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin kolyshkin merged commit 28ef7ed into moby:main May 18, 2022
@AkihiroSuda
Copy link
Member

@kolyshkin Can we have a new release? 🙏

Comment on lines +6 to +13
var bs []byte
for _, i := range is {
if i == 0 {
break
}
bs = append(bs, byte(i))
}
return string(bs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrited with unsafe.
#117

@akhramov
Copy link
Contributor

akhramov commented Jun 4, 2022

@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
Copy link
Collaborator

@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:

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

@akhramov
Copy link
Contributor

akhramov commented Jun 5, 2022

@kolyshkin thanks! So, something like #118?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants