wgtypes: comments on Key

[bradfitz]

Key is a bit weird now:

type Key [keyLen]byte

You expose its representation only halfway: you say it's a byte array (is that important?), but don't say its length.

If it's going to be opaque-ish, just go all the way and make it a struct with only hidden fields.

Also, the package isn't sure whether it's a pointer type or value type:

func ClearKey() *Key
func GenerateKey() (Key, error)

At least in PeerConfig.PublicKey Key vs PeerConfig.PresharedKey *Key the difference is whether it's required or not. But I would fix still make those consistent, and have the concept of a zero Key value, rather than having to use a pointer.

Or always use a pointer. Just be consistent?

[mdlayher]

You expose its representation only halfway: you say it's a byte array (is that important?), but don't say its length.

If it's going to be opaque-ish, just go all the way and make it a struct with only hidden fields.

A byte array still feels correct to me, though that does mean that the caller could potentially manipulate it directly. I'm okay with exporting KeyLen too. Not a huge deal either way I suppose.

Also, the package isn't sure whether it's a pointer type or value type:

Right, so the "ClearKey" thing is a recent development that can be used as a convenience to clear a specified key when configuring a device.

The tricky thing, and the reason I went with pointer in ClearKey and configuration, is that WireGuard acts differently depending on the value of the key:

• all zero key (output by ClearKey, but it returns pointer to it for convenience in config) removes a specified key all together
• non-zero key sets a new key

Nil pointer is my way of saying "do nothing with this field". I had also debated going the options functions route, but figured that this would be more concise and easier to work with.

Any suggestions?

[mdlayher]

I've exported KeyLen and removed ClearKey to avoid value/pointer confusion: https://github.com/mdlayher/wireguardctrl/commit/89ec2561f2658b562d135a082c8795435e09b906

Any further thoughts?

[bradfitz]

These are kinda gross:

    // ListenPort specifies a device's listening port, if not nil.
    ListenPort *int

    // FirewallMark specifies a device's firewall mark, if not nil.
    //
    // If non-nil and set to 0, the firewall mark will be cleared.
    FirewallMark *int

Protobuf3 moved away from using optional fields by making them pointers.

I'd just use zero to mean what nil currently means, and use -1 if you need a special value. (firewall mark of 0, or listening on ":0" which seems unlikely in this application). Do people commonly use firewall marks of 0? I don't myself.

[bradfitz]

Likewise for the *time.Duration.

And drop second comma here: (in several places)

A non-nil, zero-value, Key

Otherwise looks good. Thanks for listening to my nit picking. :)

[mdlayher]

The feedback is much appreciated, thanks. I may not get to this before GopherCon, so I'll leave the issue open for tracking what we can do to remove a few integer pointers.

[zx2c4]

This might come in handy: https://git.zx2c4.com/wireguard-windows/tree/conf/config.go It deals with optionals in various ways: https://git.zx2c4.com/wireguard-windows/tree/conf/writer.go