Skip to content

Added HV Socket known IDs, Dial, bug fixes #239

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.

Sign up for GitHub

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
Merged
helsaawy merged 3 commits into from
Jul 21, 2022
Merged

Added HV Socket known IDs, Dial, bug fixes #239

helsaawy merged 3 commits into from
Jul 21, 2022

Conversation

@helsaawy
Copy link
Contributor

@helsaawy helsaawy commented Mar 23, 2022 •
edited
Loading

Added:

  • Well-know Hyper-V VMIDs for parents, children, and loopback.
  • VSock interop service GUID.
  • Dial() and DialContext() to dial a specific Hyper-V socket at a known address (along with a corresponding HvsockDialer struct.

Bug fixes:

  • Dial (and Listen) now properly initialize and set properties of their sockets after ConnectEx (and AcceptEx).
  • The socketError used by bind was incorrect, it should be int32(-1), not uintptr(^0)

Created a sockets package, currently only with syscalls to Bind, ConnectEx and GetSockName, bypassing syscall/windows restrictions on the types that cane be used

Signed-off-by: Hamza El-Saawy [email protected]

@helsaawy helsaawy requested a review from a team as a code owner March 23, 2022 19:57
@helsaawy helsaawy mentioned this pull request Mar 23, 2022
Merged
kevpar
kevpar reviewed Mar 23, 2022
View reviewed changes
kevpar
kevpar reviewed Mar 23, 2022
View reviewed changes
@kevpar
Copy link
Member

kevpar commented Mar 24, 2022

This seems like a change that should be split across multiple PRs. For instance can we add sockets package, and then use it in another PR?

I'd also like to understand better what the motivation is for these changes. What can a user of go-winio not do without these changes?

kevpar
kevpar reviewed Mar 24, 2022
View reviewed changes
@helsaawy
Copy link
Contributor Author

helsaawy commented Mar 24, 2022 •
edited
Loading

This seems like a change that should be split across multiple PRs. For instance can we add sockets package, and then use it in another PR?

I'd also like to understand better what the motivation is for these changes. What can a user of go-winio not do without these changes?

Without the sockets package, a user wouldn't be able to call GetPeerName, Bind, or ConnectEx on an arbitrary socket type (the syscall packages only allow them on types implemented internal to them). We had our own bind internally here, but it wasnt exported and required handling raw pointers.

Those three (GetPeerName, Bind, andConnectEx) are fairly useless on their own, but they allow use to easily define our own Dial for HV sockets.

I could add sockets alone, but I think it would look fairly unnecessary, as without Hvsock defining its own socket type, it would appear fairly redundant with golang's socket functions.

That said, I can split this into sockets and HV sockets updates (or HV sockets feature additions and bug fixes).

kevpar
kevpar reviewed Apr 2, 2022
View reviewed changes
kevpar
kevpar reviewed Apr 2, 2022
View reviewed changes
@helsaawy helsaawy mentioned this pull request May 5, 2022
Merged
3 tasks
dcantah
dcantah reviewed May 5, 2022
View reviewed changes
dcantah
dcantah reviewed May 5, 2022
View reviewed changes
dcantah
dcantah reviewed May 5, 2022
View reviewed changes
dcantah
dcantah reviewed May 5, 2022
View reviewed changes
@helsaawy helsaawy force-pushed the he/hvsock branch 2 times, most recently from df6977a to 144d754 Compare May 5, 2022 20:40
dcantah
dcantah reviewed Jun 15, 2022
View reviewed changes
@helsaawy
Added HV Socket known IDs, Dial, bug fixes
9a46672 
Added:
* Well-know Hyper-V VMIDs for parents, children, and loopback.
* VSock interop service GUID.
* `Dial()` and `DialContext()` to dial a specific Hyper-V socket at a
  known address (along with a corresponding `HvsockDialer` struct.

Bug fixes:
* Dial (and Listen) now properly initialize and set properties of their
  sockets after ConnectEx (and AcceptEx).
* The `socketError` used by `bind` was incorrect, it should be `int32(-1)`,
  not `uintptr(^0)`
* Return errors for `(*HvsockConn) SetDeadline`

Created a `sockets` package, currently only with syscalls to `Bind`,
`ConnectEx` and `GetSockName`, bypassing `syscall/windows` restrictions
on the types that can do so.

Signed-off-by: Hamza El-Saawy <[email protected]>
dcantah
dcantah reviewed Jul 1, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 1, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 1, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 1, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 1, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 1, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 1, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 1, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 5, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 5, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 5, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 5, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 5, 2022
View reviewed changes
@helsaawy
PR: error handling bug, rebase, unexport, naming
a615ab2 
* comments and todos statements
* spelling
* removed dead code
* changed names to be more conventional
* unexported socket code
* made `(*HvsockDialer) Dial` take `Context`, removed `DialContext`
* added default `Dial` function
* rebased onto main
* cleaned up `Dial(` retry loop

Signed-off-by: Hamza El-Saawy <[email protected]>
@helsaawy helsaawy force-pushed the he/hvsock branch 2 times, most recently from 0997ca6 to a615ab2 Compare July 5, 2022 23:04
dcantah
dcantah reviewed Jul 6, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 13, 2022
View reviewed changes
dcantah
dcantah reviewed Jul 13, 2022
View reviewed changes
@msscotb msscotb assigned anmaxvl and unassigned kevpar Jul 13, 2022
@helsaawy helsaawy merged commit d68e55c into microsoft:master Jul 21, 2022
@helsaawy helsaawy deleted the he/hvsock branch July 21, 2022 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevpar kevpar kevpar left review comments

@anmaxvl anmaxvl anmaxvl approved these changes

+1 more reviewer

@dcantah dcantah dcantah approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@anmaxvl anmaxvl

@dcantah dcantah

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@helsaawy @kevpar @anmaxvl @dcantah