From 7e5e32775a02503fbaafe22d8b22b6eb7e7dfae1 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Tue, 11 Feb 2020 15:21:24 -0800
Subject: [PATCH] wgengine: flesh out some docs, minor cleanups

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 wgengine/router_fake.go  |  6 +++---
 wgengine/router_linux.go | 10 +++++++++-
 wgengine/userspace.go    |  4 ++--
 wgengine/wgengine.go     | 17 ++++++++++-------
 4 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/wgengine/router_fake.go b/wgengine/router_fake.go
index 8157e929a..e6227e977 100644
--- a/wgengine/router_fake.go
+++ b/wgengine/router_fake.go
@@ -16,11 +16,10 @@ type fakeRouter struct {
 }
 
 func NewFakeRouter(logf logger.Logf, tunname string, dev *device.Device, tuntap tun.Device, netChanged func()) Router {
-	r := fakeRouter{
+	return &fakeRouter{
 		logf:    logf,
 		tunname: tunname,
 	}
-	return &r
 }
 
 func (r *fakeRouter) Up() error {
@@ -33,6 +32,7 @@ func (r *fakeRouter) SetRoutes(rs RouteSettings) error {
 	return nil
 }
 
-func (r *fakeRouter) Close() {
+func (r *fakeRouter) Close() error {
 	r.logf("Warning: fakeRouter.Close: not implemented.\n")
+	return nil
 }
diff --git a/wgengine/router_linux.go b/wgengine/router_linux.go
index edd93d570..5c76171c8 100644
--- a/wgengine/router_linux.go
+++ b/wgengine/router_linux.go
@@ -57,6 +57,8 @@ func cmd(args ...string) *exec.Cmd {
 func (r *linuxRouter) Up() error {
 	out, err := cmd("ip", "link", "set", r.tunname, "up").CombinedOutput()
 	if err != nil {
+		// TODO: this should return an error; why is it calling log.Fatalf?
+		// Audit callers to make sure they're handling errors.
 		log.Fatalf("running ip link failed: %v\n%s", err, out)
 	}
 
@@ -154,6 +156,7 @@ func (r *linuxRouter) SetRoutes(rs RouteSettings) error {
 	r.local = rs.LocalAddr
 	r.routes = newRoutes
 
+	// TODO: this:
 	if false {
 		if err := r.replaceResolvConf(rs.DNS, rs.DNSDomains); err != nil {
 			errq = fmt.Errorf("replacing resolv.conf failed: %v", err)
@@ -162,12 +165,17 @@ func (r *linuxRouter) SetRoutes(rs RouteSettings) error {
 	return errq
 }
 
-func (r *linuxRouter) Close() {
+func (r *linuxRouter) Close() error {
+	var ret error
 	r.mon.Close()
 	if err := r.restoreResolvConf(); err != nil {
 		r.logf("failed to restore system resolv.conf: %v", err)
+		if ret == nil {
+			ret = err
+		}
 	}
 	// TODO(apenwarr): clean up iptables etc.
+	return ret
 }
 
 const (
diff --git a/wgengine/userspace.go b/wgengine/userspace.go
index e21bae855..d2e6242f9 100644
--- a/wgengine/userspace.go
+++ b/wgengine/userspace.go
@@ -205,7 +205,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string) error
 		e.peerSequence[i] = p.PublicKey
 	}
 
-	// TODO(apenwarr): get rid of silly uapi stuff for in-process comms
+	// TODO(apenwarr): get rid of uapi stuff for in-process comms
 	uapi, err := cfg.ToUAPI()
 	if err != nil {
 		return err
@@ -239,7 +239,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string) error
 
 	rs := RouteSettings{
 		LocalAddr:  cidr,
-		Cfg:        *cfg,
+		Cfg:        cfg,
 		DNS:        cfg.Interface.Dns,
 		DNSDomains: dnsDomains,
 	}
diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go
index 83538ad84..1971bd8d4 100644
--- a/wgengine/wgengine.go
+++ b/wgengine/wgengine.go
@@ -45,7 +45,7 @@ type RouteSettings struct {
 	LocalAddr  wgcfg.CIDR // TODO: why is this here? how does it differ from wgcfg.Config's info?
 	DNS        []net.IP
 	DNSDomains []string
-	Cfg        wgcfg.Config // TODO: value type here, but pointer below?
+	Cfg        *wgcfg.Config
 }
 
 // OnlyRelevantParts returns a string minimally describing the route settings.
@@ -58,17 +58,20 @@ func (rs *RouteSettings) OnlyRelevantParts() string {
 		rs.LocalAddr, rs.DNS, rs.DNSDomains, peers)
 }
 
-// Router is the TODO.
+// Router is responsible for managing the system route table.
+//
+// There's only one instance, and one per-OS implementation.
 type Router interface {
 	// Up brings the router up.
-	// TODO: more than once? after Close?
 	Up() error
-	// SetRoutes sets the routes.
-	// TODO: while running?
+
+	// SetRoutes is called regularly on network map updates.
+	// It's how you kernel route table entries are populated for
+	// each peer.
 	SetRoutes(RouteSettings) error
+
 	// Close closes the router.
-	// TODO: return an error? does this block?
-	Close()
+	Close() error
 }
 
 // Engine is the Tailscale WireGuard engine interface.