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.

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

ConnectToPort() needs to Dup() its VirtioSocketConnection #54

Closed
cfergeau opened this issue Sep 26, 2022 · 4 comments · Fixed by #63
Closed

ConnectToPort() needs to Dup() its VirtioSocketConnection #54

cfergeau opened this issue Sep 26, 2022 · 4 comments · Fixed by #63
Labels
bug Something isn't working

Comments

@cfergeau
Copy link
Contributor

Describe the bug

The VirtioSocketConnection created through ConnectToPort() is closed after its completion handler returns.

To Reproduce

cfergeau@fd9f5cc

Expected behavior

The VirtioSocketConnection created through ConnectToPort() can be used outside of its completion hander.

Environment (please complete the following information):

  • macOS Version: 12.6
  • Go Version: 1.19

Additional context

#53 has a proposed fix for this
@cfergeau cfergeau added the bug Something isn't working label Sep 26, 2022
@Code-Hex
Copy link
Owner

Code-Hex commented Oct 8, 2022 •
edited
Loading

I think we should fix this problem to use net.FileConn because of this API uses nonblocking connection and syscall dup.

@cfergeau
Copy link
Contributor Author

I looked at net.FileConn, and with respects to v3, I would recommend removing func (v *VirtioSocketConnection) FileDescriptor() uintptr from the API. We would need more code to support it together with net.FileConn, and I don't think it's needed now that VirtioSocketConnection implements net.Conn

@cfergeau cfergeau mentioned this issue Oct 10, 2022
Closed
5 tasks
cfergeau added a commit to cfergeau/vz that referenced this issue Oct 11, 2022
@cfergeau
vsock: Use net.FileConn to create VirtioSocketConnection
ea8461c 
Currently, newVirtioSocketConnection manually sets its objc file
descriptor to non-blocking, calls dup() on it, ... so that it
matches what go expects from a file descriptor.

go provides a helper doing all of this for us: net.FileConn()
This commit makes use of that helper. This will allow for more code
simplifications in the next commits.

However this removes vz.VirtioSocketConnection.FileDescriptor() since
the file descriptor was created internally by net.FileConn().
It would be possible to keep it, but I'm not sure it's really useful.
It would be more consistent with other go APIs to add a File() method if
we want to expose this.

One side-effect of this change is to fix connections returned by
ConnectToPort. At the moment, they are missing a call to Dup() for the
file descriptor they use. Without this, their file descriptor is closed
too early, and the connection cannot be used.

This fixes Code-Hex#54
@cfergeau
Copy link
Contributor Author

I think we should fix this problem to use net.FileConn

I've tried this, this solves this bug, and this allows for some nice code simplifications. I've put these changes in https://github.com/cfergeau/vz/tree/vsock-upstream
Let me know if/when you want a PR for this.

@Code-Hex
Copy link
Owner

@cfergeau Exactly, I want the code but a little bit needs to fix a point.
I would be happy to accept this amount of change. Could you send PR?

This was referenced Oct 12, 2022
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

vsock: Use net.FileConn to create VirtioSocketConnection cfergeau/vz
2 participants
@cfergeau @Code-Hex