Open bradfitz opened 5 years ago
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:
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?
I've exported KeyLen and removed ClearKey to avoid value/pointer confusion: https://github.com/mdlayher/wireguardctrl/commit/89ec2561f2658b562d135a082c8795435e09b906
Any further thoughts?
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.
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. :)
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.
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
Key is a bit weird now:
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:
At least in
PeerConfig.PublicKey Key
vsPeerConfig.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?