ndp.Dial uses (*icmp.PacketConn).SetChecksum which is not implemented on Windows

[rtpt-erikgeiser]

As far as I am aware, this module cannot be used on Windows because the ndp.Dial method returns errNotImplemented. This error originates from the following call to (*icmp.PacketConn).SetChecksum:

ndp/conn.go

Lines 66 to 70 in 17ab9e3

	// Calculate and place ICMPv6 checksum at correct offset in all messages.
	const chkOff = 2
	if err := pc.SetChecksum(true, chkOff); err != nil {
	return nil, nil, err
	}

Now, I don't know that much about ndp, however, during my testing, at least sending router advertisements seemed to work just fine when the call to SetChecksum was removed. Disclaimer: I do not know why this call is necessary and what may break if it is removed. However, if it is indeed optional, I would propose the following solutions:

1. Separate ndp.Dial for Windows

Implement a seperate ndp.Dial function with Windows build tags where the lines above are removed.

2. Allowing users to setup the connection themselves

Almost everything in ndp.Dial can be done by the user itself. However, at the end ndp.newConn is used to construct a *ndp.Conn from an *icmp.PacketConn. As ndp.newConn is not exported and as all properties of ndp.Conn are also private, this currently cannot be done by the user. If ndp.newConn was exported, users could work around this issue themselves. This solution would also work fine alongside ndp.Dial as it grants the user more flexibility to work around future issues.

3. Skipping SetChecksum at runtime on Windows

Another less preferable solution would be to just skip the SetChecksum call on Windows at runtime as follows:

if runtime.GOOS != "windows" {
	// Calculate and place ICMPv6 checksum at correct offset in all
	// messages, is is not implemented by x/net/ipv6 on Windows.
	const chkOff = 2
	if err := pc.SetChecksum(true, chkOff); err != nil {
		return nil, nil, fmt.Errorf("set checksum: %w", err)
	}
}

If you decide on a preferred solution, I could also create a PR if you want.

[rtpt-erikgeiser]

I just noticed that the second possible solution is closely related to #10. So maybe both issues can be resolved together.

[mdlayher]

Option 3 seems reasonable to me for now, though honestly I have no idea which other parts of x/net/* work on Windows as I am on *nix 99% of the time. A PR would be welcome for that option.

As for #10, I'll think on that some more but don't plan to do it immediately. Thanks!

[rtpt-erikgeiser]

Okay I can submit a PR. But first, could you tell me why SetChecksum is called in the first place? If this call is needed for functionality other than router advertisements, this change could be a bad idea because it would imply that the library works with Windows when it fact it does not. Again, I only tested router advertisements after removing the call as this is my only usecase. If the checksum is optional then option 3 would indeed be fine and I'll submit the PR. I just don't want to introduce anything that makes this library worse.

[mdlayher]

Honestly I consider Windows support for any of my libraries to be best effort and would rather not change any existing functionality for Unix. It's been a long time since I've reviewed this code so let's make the minimum change to get Windows working and leave everything else alone for now.