-
Notifications
You must be signed in to change notification settings - Fork 22
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
ndp: initial Conn implementation #2
Conversation
} | ||
|
||
// Dial dials a NDP connection using the specified interface. It returns | ||
// a Conn and the link-local IPv6 address of the interface. |
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.
Why does the caller need to know the lladdr? From my reading of NDP, it's only needed to source the packets, so could stay internal to this package?
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.
It's useful, at least for now, until we have more specialized methods available. Then I can consider dropping it.
client.go
Outdated
} | ||
|
||
// Calculate and place ICMPv6 checksum at correct offset in all messages. | ||
if err := pc.SetChecksum(true, 2); err != nil { |
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.
magic number 2. Make it an internal const?
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.
Done.
client.go
Outdated
// The default control message used when none is specified. | ||
cm: &ipv6.ControlMessage{ | ||
// Note that hop limit is always 255 in NDP. | ||
HopLimit: 255, |
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.
Fascinating, I would have expected it to be 1, to force it to stay link-local. TIL.
client.go
Outdated
// | ||
// If cm is nil, a default control message will be sent. If dst is nil, the | ||
// message will be sent to the IPv6 link-local all nodes address for the Conn. | ||
func (c *Conn) WriteTo(m Message, cm *ipv6.ControlMessage, dst net.IP) 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.
Does dst==nil need to be special? Can't you make the caller pass in the all-nodes multicast address if that's what they want to send to?
Just thinking of that because it makes the API surface more regular, rather than "nil is magic".
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.
Done.
client.go
Outdated
continue | ||
} | ||
|
||
if !linkLocalPrefix.Contains(ipn.IP) { |
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.
Use https://golang.org/pkg/net/#IP.IsLinkLocalUnicast instead?
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.
Done.
No tests as of now because this is going to need some more thought, but it's a start.
/cc @miekg @danderson