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/windows: fix Accept implementation #54212

Open
x1unix opened this issue Aug 2, 2022 · 13 comments
Open

x/sys/windows: fix Accept implementation #54212

x1unix opened this issue Aug 2, 2022 · 13 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@x1unix
Copy link

x1unix commented Aug 2, 2022

Currently windows.Accept (like syscall.Accept for GOOS=windows) contains a stub function which does nothing.

Looks like this function was introduced to mimic unix.Sockaddr.Accept method but it's just a useless stub.

I propose to fix it's signature and re-implement it as winsock's accept function.

In the end it will look something like this:

package windows

func Accept(fd windows.Handle, sockaddr, sockaddrLen uintptr)

Winsock accept function signature (see MSDN)

SOCKET WSAAPI accept(
  [in]      SOCKET   s,
  [out]     sockaddr *addr,
  [in, out] int      *addrlen
);

Instead of passing sockaddr raw pointer and it's size it's possible to also use Sockaddr interface and the same approach like in windows.Bind but it will limit method's potential use cases (it can't be used for custom sockets). See #54209.

@x1unix x1unix added the Proposal label Aug 2, 2022
@gopherbot gopherbot added this to the Proposal milestone Aug 2, 2022
@ianlancetaylor ianlancetaylor changed the title proposal: x/sys: fix windows.Accept implementation proposal: x/sys/windows: fix Accept implementation Aug 3, 2022
@ianlancetaylor
Copy link
Contributor

Windows programs mostly use windows.AcceptEx. If Accept can work I don't see a problem implementing it. But the only reason to implement it in x/sys/windows would be to match x/sys/unix, meaning that the function should be

func Accept(fd Handle) (nfd Handle, sa Sockaddr, err error)

@x1unix
Copy link
Author

x1unix commented Aug 3, 2022

@ianlancetaylor the problem is that in x/sys/unix, Accept is a method of Sockaddr interface but in windows package it's a dedicated function which just accepts some kind of handle (which makes it useless).

Winsock supports a classic accept function and accepts socket, sockaddr pointer and sockaddr struct length.

I would recommend to implement Sockaddr interface in the same way as it's done in x/sys/unix but to make it open for custom implementation so it can be reused for custom sockets.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 3, 2022
@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

For Unix, Accept shows up next to Sockaddr in pkg.go.dev because it is a constructor, not a method.

The Accept signature in x/sys/windows can't be changed, but it would be fine to implement.
If the signature is not changing and the behavior is changing from a stub to an actual implementation, that sounds perfectly fine and doesn't need more discussion.

Do I have that right?

@rsc
Copy link
Contributor

rsc commented Aug 3, 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) Aug 3, 2022
@x1unix
Copy link
Author

x1unix commented Aug 3, 2022

 @rsc I'm not really sure that it's possible to implement Accept method for windows with only 1 argument, because in winsocks signature is the same as for regular POSIX accept method.

That means that accept requires also sockaddr pointer and a size:

SOCKET WSAAPI accept(
  [in]      SOCKET   s,
  [out]     sockaddr *addr,
  [in, out] int      *addrlen
);

There also an AcceptEx function but it's much more complicated and non POSIX-compatible.

Do you know why it was initially designed with only 1 argument? And how it was supposed to be used?

Thank you.

@ianlancetaylor
Copy link
Contributor

The Windows accept function is the same as the Unix accept function. But the addr and addrlen parameters are just output parameters. There is no need to pass them in. So the Go version just passes down the address of local variables, and returns those variables. See, for example, https://go.googlesource.com/sys/+/refs/heads/master/unix/syscall_bsd.go#272.

@x1unix
Copy link
Author

x1unix commented Aug 3, 2022

@ianlancetaylor thank you for clarification :)

@PumpkinSeed
Copy link

The Windows accept function is the same as the Unix accept function. But the addr and addrlen parameters are just output parameters. There is no need to pass them in. So the Go version just passes down the address of local variables, and returns those variables. See, for example, https://go.googlesource.com/sys/+/refs/heads/master/unix/syscall_bsd.go#272.

One quick clarification, the addrlen is an [in, out] parameter, which means that it receives a value and based on that value it will refresh the value on the same memory address. I don't have experience with Accept specifically, but in some cases it's mandatory to pass the value into the syscall, and used the refreshed one. One specific example: WSALookupServiceNext also accepts a size, which value is 120 in the first syscall (the length of WSAQUERYSET), and updates to 1024. The next syscall supposed to used the same memory address and the refreshed value.

This doesn't mean that you need to update the function signature, but something to keep in mind.

@x1unix
Copy link
Author

x1unix commented Aug 5, 2022

@rsc can I contribute somehow to the implementation if this proposal will be accepted?

@ianlancetaylor
Copy link
Contributor

@x1unix Sure. Thanks.

@rsc
Copy link
Contributor

rsc commented Aug 10, 2022

It sounds like everyone agrees this is about implementing a stub function, keeping the API exactly the same. Assuming that's true, there's no need for a proposal.

@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

Removing from the proposal process now that the issue is simply to implement the existing stub API with no API signature changes or new APIs.

@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

Removed from the proposal process.
This was determined not to be a “significant change to the language, libraries, or tools”
or otherwise of significant importance or interest to the broader Go community.
— rsc for the proposal review group

@ianlancetaylor ianlancetaylor changed the title proposal: x/sys/windows: fix Accept implementation x/sys/windows: fix Accept implementation Oct 5, 2022
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Oct 5, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
No open projects
Development

No branches or pull requests

5 participants