-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: create opts package #316
Conversation
} | ||
|
||
// NewTCPOptions creates a [TCPOptions] for the given [net.TCPConn]. | ||
func NewTCPOptions(conn *net.TCPConn) (TCPOptions, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to accept a TCPConn
? Can we just accept a net.Conn
and returns error if it's not a TCPConn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it doesn't accept every type of net.Conn. I would rather be explicit about it.
Also, options for UDP may differ from options for TCP.
x/sockopt/sockopt.go
Outdated
opt := &hopLimitOption{} | ||
switch { | ||
case addr.Addr().Is4(): | ||
conn := ipv4.NewConn(conn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we create a new conn
here, is it the same as the old conn
(passed in through the parameter)? What are the relationship between them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ipv4
/ipv6
packages have this Conn abstraction that is only used to set options. It's not technically a connection, more like an options interface:
I've renamed the local one to ipConn
to avoid confusions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it wraps around an existing fd
, though the source code is actually located in the net
package rather than the x/net
: https://cs.opensource.google/go/go/+/master:src/net/rawconn.go;l=79;drc=9088883cf4a5181cf796c968bbce5a5bc3edc7ab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another look?
x/sockopt/sockopt.go
Outdated
opt := &hopLimitOption{} | ||
switch { | ||
case addr.Addr().Is4(): | ||
conn := ipv4.NewConn(conn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ipv4
/ipv6
packages have this Conn abstraction that is only used to set options. It's not technically a connection, more like an options interface:
I've renamed the local one to ipConn
to avoid confusions.
} | ||
|
||
// NewTCPOptions creates a [TCPOptions] for the given [net.TCPConn]. | ||
func NewTCPOptions(conn *net.TCPConn) (TCPOptions, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it doesn't accept every type of net.Conn. I would rather be explicit about it.
Also, options for UDP may differ from options for TCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. LGTM!
As per #313.
This is needed for some of the new strategies.