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

freebsd: Remove the cgo dependency from transport_unixcred_freebsd.go #332

Merged
merged 1 commit into from
Oct 29, 2022

Conversation

dfr
Copy link
Contributor

@dfr dfr commented Aug 18, 2022

This also stops the code from wrongly using sizeof(struct ucred) which
is unrelated to SCM_CREDS. The size of the correct cmsgcreds structure
is hard-coded here - this ABI has not changed for ~25 years.

The added dependency on golang.org/x/sys/unix could be avoided if
necessary, e.g. by using unsafe.Sizeof(uintptr).

Fixes #331

@dfr dfr changed the title freebsd: Remove the cgo depenency from transport_unixcred_freebsd.go freebsd: Remove the cgo dependency from transport_unixcred_freebsd.go Aug 18, 2022
This also stops the code from wrongly using sizeof(struct ucred) which
is unrelated to SCM_CREDS. The size of the correct cmsgcreds structure
is hard-coded here - this ABI has not changed for ~25 years.

The added dependency on golang.org/x/sys/unix could be avoided if
necessary, e.g. by using unsafe.Sizeof(uintptr).

Fixes godbus#331
@dfr
Copy link
Contributor Author

dfr commented Oct 12, 2022

Rebased

@dfr
Copy link
Contributor Author

dfr commented Oct 12, 2022

@guelfey PTAL

@guelfey
Copy link
Member

guelfey commented Oct 29, 2022

Sorry for the delay. Also see #318. As mentioned there, IMO the details of this struct should be handled in sys/unix; I opened golang/go#51711 for that, but didn't get around to fixing that. This would make it a bit more proper / "future-proof" as there is tooling there to create these struct / size definitions for all architectures (even though here it wouldn't matter).

@dfr
Copy link
Contributor Author

dfr commented Oct 29, 2022

No worries about the delay - thanks for taking a look. I agree that it would be best to have the struct definition in x/sys but as I mentioned above, this ABI is extremely stable and is the same across 32bit and 64bit platforms so perhaps that doesn't have to block?

Reading the golang issue, I have a very slight preference to call the struct Cmsgcreds - while the APIs are similar, they are not the same as you point out and Cmsgcreds additionally gives access to all the user's groups. I can comment there if that helps and I'm happy to make a PR along the lines suggested using whichever name consensus prefers.

@guelfey
Copy link
Member

guelfey commented Oct 29, 2022

Hm fair point, unless some future architecture will switch to 64 bits for process IDs or similar 😄 but that really rather seems unlikely to me

@guelfey guelfey merged commit 4b691ce into godbus:master Oct 29, 2022
@dfr dfr deleted the freebsd-unixcred branch October 29, 2022 15:53
dfr added a commit to dfr/podman that referenced this pull request Oct 30, 2022
This pulls in godbus/dbus#332 allowing dbus to
build without cgo on FreeBSD. This will allow freebsd targets in the cross
build.

[NO NEW TESTS NEEDED]

Signed-off-by: Doug Rabson <dfr@rabson.org>
ghuntley pushed a commit to coder/coder that referenced this pull request Dec 7, 2022
Coder fails to build [1] on FreeBSD:

transport_unix.go:52:10: cannot use t (variable of type *unixTransport) as type transport in return statement:
        *unixTransport does not implement transport (missing SendNullByte method)

Update godbus dependency to latest, post upstream PR #332 [2] via #237

Fixes: #4516

[1] #4516
[2] godbus/dbus#332
[3] godbus/dbus#237
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.

FreeBSD build depends on cgo
2 participants