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

x/sys/unix: standardize Kevent_t Udata as integer, not pointer #32153

Open
wI2L opened this issue May 20, 2019 · 19 comments

Comments

@wI2L
Copy link
Contributor

commented May 20, 2019

unix.SetKevent only takes mode(eg. Filter field) and flags parameter.

Other fields also have different types depending on the ach/platform, say Udata which is either a *byte or a int32.

Since Go1 compat promise prevent us from changing the SetKevent function, perhaps it could be feasible to add new methods to the Kevent_t type to set the others fields, similar to *Iovec.SetLen for example.

Fflags and Data methods looks straightforward to implement, on the model of SetKevent, since types are integers on all platforms/archs.

However, so far, I am unsure about the type of the Udata parameter for the eponym method. I typically would do kev.Udata = (*byte)(unsafe.Pointer(&foobar)) on my own currently for darwin/amd64, but I guess that exposing unsafe.Pointer as a parameter type is not a good solution.

@gopherbot gopherbot added this to the Unreleased milestone May 20, 2019

@wI2L wI2L changed the title x/sys/unix: support for all fields of Kevent_t in SetKevent proposal: x/sys/unix: support for all fields of Kevent_t in SetKevent May 20, 2019

@gopherbot gopherbot added the Proposal label May 20, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented May 20, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

unsafe.Pointer seems to me to be the only reasonable choice for the Udata field. The C type is void * and unsafe.Pointer is the Go equivalent.

@wI2L

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Udata field types:

*byte

  • darwin_386
  • darwin_amd64
  • darwin_arm
  • darwin_arm64
  • dragonfly_amd64
  • openbsd_386
  • openbsd_amd64
  • freebsd_amd64
  • freebsd_386
  • freebsd_arm

int32

  • netbsd_386
  • netbds_arm

int64

  • netbsd_amd64
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Using int32 and int64 is not OK if people store pointers in the field. Conversely, using *byte is not OK if people store non-pointers in the field. Unfortunately, Go does not permit a union of pointer and non-pointer types.

@wI2L

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@ianlancetaylor Any idea why *byte isn't used as type for netbsd ?
According to NetBSD manpage for kevent, Udata is of type intptr_t. A Go uintptr could have been used instead of int32 and int64, right ?

@tklauser

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@wI2L It's an intptr_t on netbsd by definition (see NetBSD/src@537e2d7) which cgo then translates to the respective int{32,64} Go type.

@wI2L

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@tklauser Understood. Can we override this to use a uintptr type instead (assuming that making that change is acceptable) ?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

There is no perfect choice here, but it seems better for the field to consistently be *byte or unsafe.Pointer on all systems.

@rsc rsc changed the title proposal: x/sys/unix: support for all fields of Kevent_t in SetKevent proposal: x/sys/unix: support for all fields of Kevent_t, EpollEvent May 28, 2019

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Merged #32192 into this one, so we can have one discussion instead of two.

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Aren't these structures held inside the kernel and passed back to user space? If so, isn't storing a Go pointer in them - and hiding that pointer in the kernel, invisible to the garbage collector - going to cause problems? If so, it seems like an int type would be preferable.

It's unclear how much we can change at all right now, of course.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

You're right, of course, it does have to be an integer.

This is x/sys/unix, so we don't have strict backward compatibility.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

We should probably unify the different architectures and OS's to the greatest extent possible. If it's an opaque word then it should probably be uintptr; else uint64. Using unsafe.Pointer would technically be OK but would certainly encourage bad behavior (you could only safely put non-Go pointers there).

It looks like we got it wrong everywhere, and Kevent udata should be uintptr.

For EpollEvent, I am a little worried about the union having both a uint64 and an int fd. It sounds like we should use uint64 consistently, but if any kernel API requires the use of the fd field in that struct, it will be unclear on 32-bit systems which half of the uint64 matters. We could do a uintptr + pad-on-32-bit instead too. That would be consistent with Kevent.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

It looks like we should definitely eliminate any unsafe.Pointer in Kevent and change to uintptr.

For epoll, does anyone know of any kernel APIs that require the use of one field or another? If there is only one field (in particular, we suspect fd) then we should keep that field being the one that's exposed.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Following up on #32153 (comment), I'm going to split the two seeming outcomes by system. First Kevent.

Our Kevent_t definitions disagree about the type of the Udata field:

ztypes_darwin_386.go:	Udata  *byte
ztypes_darwin_amd64.go:	Udata  *byte
ztypes_darwin_arm.go:	Udata  *byte
ztypes_darwin_arm64.go:	Udata  *byte
ztypes_dragonfly_amd64.go:	Udata  *byte
ztypes_freebsd_386.go:	Udata  *byte
ztypes_freebsd_amd64.go:	Udata  *byte
ztypes_freebsd_arm.go:	Udata  *byte
ztypes_freebsd_arm64.go:	Udata  *byte
ztypes_netbsd_386.go:	Udata  int32
ztypes_netbsd_amd64.go:	Udata     int64
ztypes_netbsd_arm.go:	Udata     int32
ztypes_netbsd_arm64.go:	Udata     int64
ztypes_openbsd_386.go:	Udata  *byte
ztypes_openbsd_amd64.go:	Udata  *byte
ztypes_openbsd_arm.go:	Udata  *byte

Based on the discussion above, it sounds like we should standardize on Udata int32/int64 (depending on architecture). Clearly the kernel can't be given Go pointers.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Next EpollEvent. All our Linux structs say:

type EpollEvent struct {
	Events uint32
	PadFd  int32
	Fd     int32
	Pad    int32
}

On 32-bit systems the PadFd field is missing, but otherwise they're consistent. In the absence of any evidence that this is causing harm to anyone (and since we're not sending any pointers to the kernel), it seems like the best course of action for EpollEvent is to do nothing at all.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Based on the previous two comments, it sounds like the right outcome here is to fix up Kevent_t to not use pointers and leave EpollEvent alone. I will retitle this and would like to hear feedback from people who agree or disagree, especially @wI2L.

@rsc rsc changed the title proposal: x/sys/unix: support for all fields of Kevent_t, EpollEvent proposal: x/sys/unix: standardize Kevent_t Udata as integer, not pointer Aug 20, 2019

@wI2L

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@rsc Thanks for the follow-up on this issue.
When I originally filled the issue, I was trying to achieve sketchy stuff, by storing a pointer to a net.Conn inside the Kevent_t.Udata field without clearly realizing the issue you pointed out in #32153 (comment).
However, could you explain what made you change your view about the udata field type ? Unless I misunderstood something, you mentionned int #32153 (comment) that we should use an uintptr.

Regarding EpollEvent, I fully agree, of the fields of the epoll_data union, fd is the one to have.

Finally, I'd like to bubble up one the subjects of the original issue I filled that has been ignored so far, regarding the SetKeventmethod. I haven't looked back to the definitions of Kevent_t for all archs/platforms, but it seems important that this method accept all the fields for which we have type discrepancies. Ident, Filters and Flags are already accepted, remains Fflags and Data. About Udata, since you are proposing to use int64 or int32, depending on the architecture, the method could accept it aswell to simplify the usage of the type (as it already does for the Ident field).

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@wI2L, as far as uintptr vs int32/int64, it just seems more consistent with all the other fields and the like to keep using sized integers. We don't use uintptr in those generated structs very often, if at all.

Re SetKevent, that's really a more general problem, that field types that vary size across system or architecture are difficult to initialize. It might make sense to think about how to address that problem in a general way that applies to all the syscall structs, not just Kevent. (For example, the package has a special case TimeToTimespec function, but nothing for most of the other structs.) It's unclear what to do in general but maybe someone will come up with a clever general solution. Let's keep this issue just about the type of the field for now; feel free to file a separate issue about SetKevent if you think that's the right path forward.

Using int32/int64 for udata in Kevent_t (as currently titled) seems like a likely accept.

Leaving a week for any final objections before accepting.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Marked this last week as likely accept w/ call for last comments (#32153 (comment)).
No comments, so accepting.

@ianlancetaylor ianlancetaylor changed the title proposal: x/sys/unix: standardize Kevent_t Udata as integer, not pointer x/sys/unix: standardize Kevent_t Udata as integer, not pointer Sep 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.