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

fixed VirtioSocketConnection impl #80

Merged
merged 1 commit into from
Oct 15, 2022

Conversation

Code-Hex
Copy link
Owner

Because (*VirtioSocketConnection).Conn.LocalAddr() and (*VirtioSocketConnection).LocalAddr() are different. RemoteAddr method too.

Because (*VirtioSocketConnection).Conn.LocalAddr() and (*VirtioSocketConnection).LocalAddr() are different. RemoteAddr method too.
@Code-Hex Code-Hex merged commit c875c7d into master Oct 15, 2022
@Code-Hex Code-Hex deleted the fix/VirtioSocketConnection-impl branch October 15, 2022 15:55
@cfergeau
Copy link
Contributor

Do you have more details about this bug?
I used commit 464091b. I called conn.LocalAddr() and conn.RemoteAddr() with the net.Conn returned by ConnectToPort, and (*VirtioSocketConnection).LocalAddr() is called as expected.
(*VirtioSocketConnection).Conn.LocalAddr() is only called if I used conn.Conn.LocalAddr().
I also tested https://go.dev/play/p/YUbREAsRG_c

In short, I don't understand what the problem was, the code before this PR looks correct to me :(

@Code-Hex
Copy link
Owner Author

Code-Hex commented Oct 17, 2022

Your example is mistaken.
Here is the same situation as VirtioSocketConnection

https://go.dev/play/p/DC88VlpJzqY

@cfergeau
Copy link
Contributor

This also works as expected? If defined, the methods from the Embed type are used. If we explicitly request the methods from Embed.Test, then the methods from Base are used.
I don't understand why it was needed to readd rawConn.

@Code-Hex
Copy link
Owner Author

Code-Hex commented Oct 17, 2022

Yes, Instead of returning an interface, the library returns the struct as is.

The standard Go library does not return address information in vsock format. Therefore, you need to create your own library in the same way as this one. Before the modification, the address information available from the exported Conn field was different from the address information returned by this VirtioSocketConnection struct. Users would therefore be confused. Therefore I created a private rawConn field and modified it to only use address information in vsock format.

I'm committing to this library in my spare time (because not my main work for this) and I hate doing this explanation so much because it's a waste of my time. And Some people just make bad issues and it's unpleasant for me. 😢

Why do I need to elaborate on this to such people? I want to focus on the issues I need to work on now. (However, @cfergeau You have been a contributor for a long time, so I explained.)

@cfergeau
Copy link
Contributor

cfergeau commented Oct 17, 2022

Users would therefore be confused.

Thanks a lot for coping with me, this is appreciated! This is the part I was missing. It's not a technical issue you were solving, you were improving the external API. I thought there was a bug I did not see!

For what it's worth, this can also be solved with:

diff --git a/socket.go b/socket.go
index 47f33ee..67e659a 100644
--- a/socket.go
+++ b/socket.go
@@ -177,8 +177,12 @@ func shouldAcceptNewConnectionHandler(listenerPtr, connPtr, devicePtr unsafe.Poi
 // to the Go struct.
 //
 // see: https://developer.apple.com/documentation/virtualization/vzvirtiosocketconnection?language=objc
+
+// Make the net.Conn embedded in `VirtioSocketConnection` private
+type internalConn net.Conn
+
 type VirtioSocketConnection struct {
-       net.Conn
+       internalConn
        laddr *Addr // local
        raddr *Addr // remote
 }
@@ -192,7 +196,7 @@ func newVirtioSocketConnection(ptr unsafe.Pointer) (*VirtioSocketConnection, err
                return nil, err
        }
        conn := &VirtioSocketConnection{
-               Conn: rawConn,
+               internalConn: rawConn,
                laddr: &Addr{
                        CID:  unix.VMADDR_CID_HOST,
                        Port: (uint32)(vzVirtioSocketConnection.destinationPort),

This way, you don't need to reimplement Read/Write/Close/...

Why do I need to elaborate on this to such people?

I know it annoys you, but you have similar expectations when someone contributes external code. If something is not clear to you, you will ask for clarifications, or ask for a detailed issue. This is good, but a side effect is that sometimes people will be looking for clarifications regarding your code.

@Code-Hex
Copy link
Owner Author

Thank you for everything you do for me too.

I don't intend to modify the code you have presented as it is a matter of writing preference.

For what it's worth, this can also be solved with:

I am not at all uncomfortable explaining this to people outside the crc members. I feel bad because of an issue that someone makes on a personal emotional level.

I know it annoys you, but you have similar expectations when someone contributes external code. If something is not clear to you, you will ask for clarifications, or ask for a detailed issue. This is good, but a side effect is that sometimes people will be looking for clarifications regarding your code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants