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

Jump to bottom

x/sys/unix: add RecvmsgBuffers and SendmsgBuffers #52885

Closed
ianlancetaylor opened this issue May 13, 2022 · 14 comments
Closed

x/sys/unix: add RecvmsgBuffers and SendmsgBuffers #52885

ianlancetaylor opened this issue May 13, 2022 · 14 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Proposals (old)
Milestone
Backlog

Comments

@ianlancetaylor
Copy link
Contributor

The golang.org/x/sys/unix package was created with Recvmsg and Sendmsg functions copied from the ones in the syscall package. Those date back to https://go.dev/cl/2331044, committed in 2010 for #1101. Unfortunately, those Recvmsg and Sendmsg functions do not correspond to the recvmsg and sendmsg system calls. The system calls take a pointer to a Msghdr struct. The Recvmsg and Sendmsg functions build a Msghdr instance, but they don't support the full functionality of recvmsg and sendmsg: they don't permit setting up the scatter/gather array.

Working directly with a Msghdr is awkward for the x/sys/unix routines. It seems better to provide some translation for the socket address. Still, we should provide access to the scatter/gather array. I propose the following:

// RecvmsgBufs receives a message from a socket using the recvmsg system call.
// The flags are passed to recvmsg. Any non-control data read is scattered into bufs. The results are:
//  - n is the number of non-control data read into bufs
//  - oobn is the number of control data read into oob; this may be interpreted using [ParseSocketControlMessage]
//  - recvflags is flags returned by recvmsg
//  - from is the address of the sender
func RecvmsgBufs(fd int, bufs [][]byte, oob []byte, flags int) (n, oobn int, recvflags int, from Sockaddr, err error)

// SendmsgBufs sends a message on a socket to an address using the sendmsg system call.
// The flags are passed to sendmsg. Any non-control data writen is gathered from bufs.
// The function returns the number of bytes written to the socket.
func SendmsgBufs(fd int, bufs [][]byte, oob []byte, to Sockaddr, flags int) (int, error)

Part of the goal of these functions is to simply the golang.org/x/net/internal/socket package, which currently reaches into the syscall package for syscall.recvmsg and syscall.sendmsg.

CC @tklauser @mdlayher
@ianlancetaylor ianlancetaylor added this to the Proposal milestone May 13, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 13, 2022
@ianlancetaylor ianlancetaylor mentioned this issue May 13, 2022
Closed
@tklauser
Copy link
Member

I agree, these seem like useful functions to have in x/sys/unix.

@jech
Copy link

jech commented May 13, 2022

Minor nit: I suggest renaming oob []byte to control []byte, so that the function is easier to correlate with the manual page, and the ParseSocketControlMessage is easier to find.

@mdlayher
Copy link
Member

I think the general historical trend has been to mimic the C API names.

SGTM on adding these functions.

@rsc
Copy link
Contributor

rsc commented May 18, 2022

Since we spelled out net.Buffers for a [][]byte, should we call this Buffers instead of Bufs too?

@rsc
Copy link
Contributor

rsc commented May 18, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) May 18, 2022
@jech
Copy link

jech commented May 19, 2022

Since we spelled out net.Buffers for a [][]byte, should we call this Buffers instead of Bufs too?

I could be wrong, but it is my understanding that the reason why net.Buffers is a named type is that it has two methods attached (it implements Reader and WriterTo). I don't think that's required or even desirable here.

@ianlancetaylor
Copy link
Contributor Author

We're not going to actually use net.Buffers here. The question is whether the new functions should be named RecvmsgBuffers and SendmsgBuffers.

@ianlancetaylor ianlancetaylor changed the title proposal: x/sys/unix: add RecvmsgBufs ad SendmsgBufs proposal: x/sys/unix: add RecvmsgBufs and SendmsgBufs May 19, 2022
@jech
Copy link

jech commented May 19, 2022 via email

@rsc
Copy link
Contributor

rsc commented May 25, 2022

I think we should probably spell out Buffers but otherwise it sounds like no one objects to this.

@rsc rsc changed the title proposal: x/sys/unix: add RecvmsgBufs and SendmsgBufs proposal: x/sys/unix: add RecvmsgBuffers and SendmsgBuffers May 25, 2022
@rsc rsc moved this from Active to Likely Accept in Proposals (old) May 25, 2022
@rsc
Copy link
Contributor

rsc commented May 25, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Jun 1, 2022
@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: x/sys/unix: add RecvmsgBuffers and SendmsgBuffers x/sys/unix: add RecvmsgBuffers and SendmsgBuffers Jun 1, 2022
@rsc rsc modified the milestones: Proposal, Backlog Jun 1, 2022
@gopherbot
Copy link

Change https://go.dev/cl/412496 mentions this issue: unix: change Solaris Iovec.Base to *byte

@gopherbot
Copy link

Change https://go.dev/cl/412497 mentions this issue: unix: add RecvmsgBuffers and SendmsgBuffers

gopherbot pushed a commit to golang/sys that referenced this issue Jun 22, 2022
@ianlancetaylor @gopherbot
unix: change Solaris Iovec.Base to *byte
175b2fd 
Every other Iovec.Base field is *byte, and that is the only reasonable
value for Go. This is not backward compatible but we've made changes
like this before.

For golang/go#52885

Change-Id: I9a313a7931966a2483a322edd5c06c8cdca2557a
Reviewed-on: https://go-review.googlesource.com/c/sys/+/412496
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Rakoczy <alex@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/419914 mentions this issue: unix: fix sendmsgN return value for empty iovecs and non-empty oob

gopherbot pushed a commit to golang/sys that referenced this issue Jul 31, 2022
@tklauser @gopherbot
unix: fix sendmsgN return value for empty iovecs and non-empty oob
a90be44 
Don't shadow the empty var when determining whether to send a single
byte when iovecs are empty but oob is non-empty. This will lead to the
n value correctly being reset to 0 before return.

No test because it's not possible to trigger this case on all platforms,
e.g. darwin where sendmsg with empty buf and non-empty oob returns
EINVAL.

This was introduced by CL 412497 and CL 419396.

Updates golang/go#52885

Change-Id: Iafc5a4b22e10b396ba5f7d4f2ac1c50df195a125
Reviewed-on: https://go-review.googlesource.com/c/sys/+/419914
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Florian Lehner <lehner.florian86@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@nshalman nshalman mentioned this issue Oct 2, 2022
Closed
This was referenced Nov 22, 2022
Closed
Closed
@golang golang locked and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
No open projects
Proposals (old)
Accepted
Milestone
Backlog
Development

No branches or pull requests
6 participants
@rsc @jech @tklauser @mdlayher @ianlancetaylor @gopherbot