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

Consistently connect to Unix sockets #2922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Nov 19, 2024

See commit message.

@tamird
Copy link
Contributor Author

tamird commented Nov 19, 2024

r? @nirs

pkg/networks/usernet/client.go Outdated Show resolved Hide resolved
pkg/vz/network_darwin.go Show resolved Hide resolved
pkg/vz/network_darwin.go Outdated Show resolved Hide resolved
@tamird tamird force-pushed the avoid-type-assertion branch 2 times, most recently from c1c8174 to 4bea537 Compare November 19, 2024 19:00
@AkihiroSuda AkihiroSuda added this to the v1.0.2 milestone Nov 20, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have to introduce a new netutil package just for avoiding this single type assertion.

If you want to avoid a potential panic, you can just check conn, ok := unixConn.(*net.UnixConn); if !ok { return errors.New(...)}

@AkihiroSuda AkihiroSuda removed this from the v1.0.2 milestone Nov 20, 2024
@tamird
Copy link
Contributor Author

tamird commented Nov 20, 2024

I don't think we have to introduce a new netutil package just for avoiding this single type assertion.

Because this pattern occurs in a number of places (connecting to a UNIX socket, not the type assertion), this is reducing all that duplication.

If you want to avoid a potential panic, you can just check conn, ok := unixConn.(*net.UnixConn); if !ok { return errors.New(...)}

I don't think a panic is a real concern. This is just cleanup.

@tamird
Copy link
Contributor Author

tamird commented Nov 21, 2024

@nirs this is what you wanted, no?

@nirs
Copy link
Member

nirs commented Nov 21, 2024

@nirs this is what you wanted, no?

I suggested to add a helper which would solve the issue for the vz package. The current change is little bit bigger and handle all unix sockets in lima (good), but it is hard to sell it as a way to avoid type assertions since we have only one.

I did have time to review all the changes, but I think this will be easier to swallow if we change the commit/pr title/message to describe why creating unix socket in this way is better.

@tamird tamird changed the title Avoid net.UnixConn type assertions Extract helper for connecting to Unix sockets Nov 21, 2024
@tamird
Copy link
Contributor Author

tamird commented Nov 21, 2024

Updated both the PR title and the commit message.

@tamird
Copy link
Contributor Author

tamird commented Nov 24, 2024

Would appreciate a review here!

@AkihiroSuda
Copy link
Member

The amount of the deduplicated code is trivial.
I'm not sure this is worth adding a new "util" pkg.

Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the unneeded and possibly unwanted change to ignore the context in many places, the actual change is very small, maybe only in vz/network_darwin.go?

But since the change is so small, I'm not sure it worth the effort. The benefit is very small. Better to invest time on stuff that add more value.

pkg/hostagent/port_darwin.go Outdated Show resolved Hide resolved
pkg/hostagent/port_darwin.go Outdated Show resolved Hide resolved
pkg/httpclientutil/httpclientutil_others.go Outdated Show resolved Hide resolved
pkg/httpclientutil/httpclientutil_windows.go Outdated Show resolved Hide resolved
pkg/networks/usernet/client.go Outdated Show resolved Hide resolved
pkg/qemu/qemu_driver.go Outdated Show resolved Hide resolved
pkg/qemu/qemu_driver.go Outdated Show resolved Hide resolved
pkg/netutil/netutil.go Outdated Show resolved Hide resolved
@tamird tamird force-pushed the avoid-type-assertion branch 3 times, most recently from 90c60ea to 5d68f9a Compare November 25, 2024 19:32
@tamird tamird changed the title Extract helper for connecting to Unix sockets Consistently connect to Unix sockets Nov 25, 2024
- Always use net.ResolveUnixAddr rather than constructing net.UnixAddr
  directly.
- Use net.Dialer.DialContext in usernet.
- Avoid type assertion on darwin.

Signed-off-by: Tamir Duberstein <[email protected]>
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.

3 participants