ipn: use *Prefs rather than Prefs throughout.

Prefs has become a heavy object with non-memcpy copy
semantics. We should not pass such a thing by value.

Signed-off-by: David Anderson <dave@natulte.net>

cmd/tailscale/tailscale.go

@@ -77,14 +77,13 @@ func main() {
 
 	// TODO(apenwarr): fix different semantics between prefs and uflags
 	// TODO(apenwarr): allow setting/using CorpDNS
-	prefs := ipn.Prefs{
-		ControlURL:       *server,
-		WantRunning:      true,
-		RouteAll:         *routeall,
-		AllowSingleHosts: !*nuroutes,
-		UsePacketFilter:  !*nopf,
-		AdvertiseRoutes:  adv,
-	}
+	prefs := ipn.NewPrefs()
+	prefs.ControlURL = *server
+	prefs.WantRunning = true
+	prefs.RouteAll = *routeall
+	prefs.AllowSingleHosts = !*nuroutes
+	prefs.UsePacketFilter = !*nopf
+	prefs.AdvertiseRoutes = adv
 
 	c, err := safesocket.Connect(*socket, 0)
 	if err != nil {

ipn/backend.go

@@ -107,7 +107,7 @@ type Backend interface {
 	// SetPrefs install a new set of user preferences, including
 	// WantRunning.  This may cause the wireguard engine to
 	// reconfigure or stop.
-	SetPrefs(new Prefs)
+	SetPrefs(new *Prefs)
 	// RequestEngineStatus polls for an update from the wireguard
 	// engine. Only needed if you want to display byte
 	// counts. Connection events are emitted automatically without

ipn/fake.go

@@ -53,8 +53,12 @@ func (b *FakeBackend) Logout() {
 	b.newState(NeedsLogin)
 }
 
-func (b *FakeBackend) SetPrefs(new Prefs) {
-	b.notify(Notify{Prefs: &new})
+func (b *FakeBackend) SetPrefs(new *Prefs) {
+	if new == nil {
+		panic("FakeBackend.SetPrefs got nil prefs")
+	}
+
+	b.notify(Notify{Prefs: new.Copy()})
 	if new.WantRunning && !b.live {
 		b.newState(Starting)
 		b.newState(Running)

ipn/handle.go

@@ -23,7 +23,7 @@ type Handle struct {
 	netmapCache       *NetworkMap
 	engineStatusCache EngineStatus
 	stateCache        State
-	prefsCache        Prefs
+	prefsCache        *Prefs
 }
 
 func NewHandle(b Backend, logf logger.Logf, opts Options) (*Handle, error) {
@@ -47,7 +47,7 @@ func (h *Handle) Start(opts Options) error {
 	h.engineStatusCache = EngineStatus{}
 	h.stateCache = NoState
 	if opts.Prefs != nil {
-		h.prefsCache = *opts.Prefs
+		h.prefsCache = opts.Prefs.Copy()
 	}
 	xopts := opts
 	xopts.Notify = h.notify
@@ -69,7 +69,7 @@ func (h *Handle) notify(n Notify) {
 		h.stateCache = *n.State
 	}
 	if n.Prefs != nil {
-		h.prefsCache = *n.Prefs
+		h.prefsCache = n.Prefs.Copy()
 	}
 	if n.NetMap != nil {
 		h.netmapCache = n.NetMap
@@ -85,18 +85,19 @@ func (h *Handle) notify(n Notify) {
 	}
 }
 
-func (h *Handle) Prefs() Prefs {
+func (h *Handle) Prefs() *Prefs {
 	h.mu.Lock()
 	defer h.mu.Unlock()
 
-	return h.prefsCache
+	return h.prefsCache.Copy()
 }
 
-func (h *Handle) UpdatePrefs(updateFn func(old Prefs) (new Prefs)) {
+func (h *Handle) UpdatePrefs(updateFn func(p *Prefs)) {
 	h.mu.Lock()
 	defer h.mu.Unlock()
 
-	new := updateFn(h.prefsCache)
+	new := h.prefsCache.Copy()
+	updateFn(new)
 	h.prefsCache = new
 	h.b.SetPrefs(new)
 }

ipn/local.go

@@ -40,7 +40,7 @@ type LocalBackend struct {
 	// The mutex protects the following elements.
 	mu           sync.Mutex
 	stateKey     StateKey
-	prefs        Prefs
+	prefs        *Prefs
 	state        State
 	hiCache      tailcfg.Hostinfo
 	netMapCache  *controlclient.NetworkMap
@@ -207,8 +207,7 @@ func (b *LocalBackend) Start(opts Options) error {
 					b.logf("Failed to save new controlclient state: %v", err)
 				}
 			}
-			np := b.prefs
-			b.send(Notify{Prefs: &np})
+			b.send(Notify{Prefs: b.prefs.Copy()})
 		}
 		if new.NetMap != nil {
 			if b.netMapCache != nil && b.cmpDiff != nil {
@@ -277,8 +276,7 @@ func (b *LocalBackend) Start(opts Options) error {
 	blid := b.backendLogID
 	b.logf("Backend: logs: be:%v fe:%v\n", blid, opts.FrontendLogID)
 	b.send(Notify{BackendLogID: &blid})
-	nprefs := b.prefs // make a copy
-	b.send(Notify{Prefs: &nprefs})
+	b.send(Notify{Prefs: b.prefs.Copy()})
 
 	cli.Login(nil, controlclient.LoginDefault)
 	return nil
@@ -368,7 +366,7 @@ func (b *LocalBackend) loadStateWithLock(key StateKey, prefs *Prefs, legacyPath
 	if key == "" {
 		// Frontend fully owns the state, we just need to obey it.
 		b.logf("Using frontend prefs")
-		b.prefs = *prefs
+		b.prefs = prefs.Copy()
 		b.stateKey = ""
 		return nil
 	}
@@ -387,8 +385,13 @@ func (b *LocalBackend) loadStateWithLock(key StateKey, prefs *Prefs, legacyPath
 	if err != nil {
 		if err == ErrStateNotExist {
 			if legacyPath != "" {
-				b.prefs = *LoadPrefs(legacyPath, true)
-				b.logf("Imported state from relaynode for %q", key)
+				b.prefs, err = LoadPrefs(legacyPath, true)
+				if err != nil {
+					b.logf("Failed to load legacy prefs: %v", err)
+					b.prefs = NewPrefs()
+				} else {
+					b.logf("Imported state from relaynode for %q", key)
+				}
 			} else {
 				b.prefs = NewPrefs()
 				b.logf("Created empty state for %q", key)
@@ -492,14 +495,18 @@ func (b *LocalBackend) AdminPageURL() string {
 	return b.serverURL + "/admin/machines"
 }
 
-func (b *LocalBackend) Prefs() Prefs {
+func (b *LocalBackend) Prefs() *Prefs {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 
 	return b.prefs
 }
 
-func (b *LocalBackend) SetPrefs(new Prefs) {
+func (b *LocalBackend) SetPrefs(new *Prefs) {
+	if new == nil {
+		panic("SetPrefs got nil prefs")
+	}
+
 	b.mu.Lock()
 	old := b.prefs
 	new.Persist = old.Persist // caller isn't allowed to override this
@@ -527,7 +534,7 @@ func (b *LocalBackend) SetPrefs(new Prefs) {
 	}
 
 	b.logf("SetPrefs: %v\n", new.Pretty())
-	b.send(Notify{Prefs: &new})
+	b.send(Notify{Prefs: new})
 }
 
 // Note: return value may be nil, if we haven't received a netmap yet.

ipn/message.go

@@ -24,7 +24,7 @@ type StartArgs struct {
 }
 
 type SetPrefsArgs struct {
-	New Prefs
+	New *Prefs
 }
 
 type FakeExpireAfterArgs struct {
@@ -191,7 +191,7 @@ func (bc *BackendClient) Logout() {
 	bc.send(Command{Logout: &NoArgs{}})
 }
 
-func (bc *BackendClient) SetPrefs(new Prefs) {
+func (bc *BackendClient) SetPrefs(new *Prefs) {
 	bc.send(Command{SetPrefs: &SetPrefsArgs{New: new}})
 }
 

ipn/message_test.go

@@ -172,9 +172,8 @@ func TestClientServer(t *testing.T) {
 		t.Errorf("notes.NetMap == nil while h.NetMap != nil\nnotes:\n%v", nn)
 	}
 
-	h.UpdatePrefs(func(p Prefs) Prefs {
+	h.UpdatePrefs(func(p *Prefs) {
 		p.WantRunning = false
-		return p
 	})
 	flushUntil(Stopped)
 

ipn/prefs.go

@@ -116,11 +116,11 @@ func compareIPNets(a, b []wgcfg.CIDR) bool {
 	return true
 }
 
-func NewPrefs() Prefs {
-	return Prefs{
-		// Provide default values for options which are normally
-		// true, but might be missing from the json data for any
-		// reason. The json can still override them to false.
+func NewPrefs() *Prefs {
+	return &Prefs{
+		// Provide default values for options which might be missing
+		// from the json data for any reason. The json can still
+		// override them to false.
 		ControlURL:       "https://login.tailscale.com",
 		RouteAll:         true,
 		AllowSingleHosts: true,
@@ -130,7 +130,10 @@ func NewPrefs() Prefs {
 	}
 }
 
-func PrefsFromBytes(b []byte, enforceDefaults bool) (Prefs, error) {
+// PrefsFromBytes deserializes Prefs from a JSON blob. If
+// enforceDefaults is true, Prefs.RouteAll and Prefs.AllowSingleHosts
+// are forced on.
+func PrefsFromBytes(b []byte, enforceDefaults bool) (*Prefs, error) {
 	p := NewPrefs()
 	if len(b) == 0 {
 		return p, nil
@@ -146,11 +149,6 @@ func PrefsFromBytes(b []byte, enforceDefaults bool) (Prefs, error) {
 			log.Printf("Prefs parse: %v: %v\n", err, b)
 		}
 	}
-	if p.ControlURL == "" {
-		// TODO(danderson): compat shim, remove after
-		// Options.ServerURL has been gone for a release.
-		p.ControlURL = "https://login.tailscale.com"
-	}
 	if enforceDefaults {
 		p.RouteAll = true
 		p.AllowSingleHosts = true
@@ -158,48 +156,28 @@ func PrefsFromBytes(b []byte, enforceDefaults bool) (Prefs, error) {
 	return p, err
 }
 
+// Copy returns a deep copy of p.
 func (p *Prefs) Copy() *Prefs {
 	p2, err := PrefsFromBytes(p.ToBytes(), false)
 	if err != nil {
 		log.Fatalf("Prefs was uncopyable: %v\n", err)
 	}
-	return &p2
+	return p2
 }
 
-func LoadPrefs(filename string, enforceDefaults bool) *Prefs {
-	log.Printf("Loading prefs %v\n", filename)
+// LoadLegacyPrefs loads a legacy relaynode config file into Prefs
+// with sensible migration defaults set. If enforceDefaults is true,
+// Prefs.RouteAll and Prefs.AllowSingleHosts are forced on.
+func LoadPrefs(filename string, enforceDefaults bool) (*Prefs, error) {
 	data, err := ioutil.ReadFile(filename)
-	p := NewPrefs()
 	if err != nil {
-		log.Printf("Read: %v: %v\n", filename, err)
-		goto fail
+		return nil, fmt.Errorf("loading prefs from %q: %v", filename, err)
 	}
-	p, err = PrefsFromBytes(data, enforceDefaults)
+	p, err := PrefsFromBytes(data, false)
 	if err != nil {
-		log.Printf("Parse: %v: %v\n", filename, err)
-		goto fail
+		return nil, fmt.Errorf("decoding prefs in %q: %v", filename, err)
 	}
-	goto post
-fail:
-	log.Printf("failed to load config. Generating a new one.\n")
-	p = NewPrefs()
-	p.WantRunning = true
-post:
-	// Update: we changed our minds :)
-	// Versabank would like to persist the setting across reboots, for now,
-	// because they don't fully trust the system and want to be able to
-	// leave it turned off when not in use. Eventually we need to make
-	// all motivation for this go away.
-	if false {
-		// Usability note: we always want WantRunning = true on startup.
-		// That way, if someone accidentally disables their VPN and doesn't
-		// know how, rebooting will fix it.
-		// We still persist WantRunning just in case we change our minds on
-		// this topic.
-		p.WantRunning = true
-	}
-	log.Printf("Loaded prefs %v %v\n", filename, p.Pretty())
-	return &p
+	return p, nil
 }
 
 func SavePrefs(filename string, p *Prefs) {

ipn/prefs_test.go

@@ -180,8 +180,8 @@ func TestPrefsEqual(t *testing.T) {
 
 func checkPrefs(t *testing.T, p Prefs) {
 	var err error
-	var p2, p2c Prefs
-	var p2b Prefs
+	var p2, p2c *Prefs
+	var p2b *Prefs
 
 	pp := p.Pretty()
 	if pp == "" {
@@ -195,9 +195,9 @@ func checkPrefs(t *testing.T, p Prefs) {
 	if !p.Equals(&p) {
 		t.Fatalf("p != p\n")
 	}
-	p2 = p
+	p2 = p.Copy()
 	p2.RouteAll = true
-	if p.Equals(&p2) {
+	if p.Equals(p2) {
 		t.Fatalf("p == p2\n")
 	}
 	p2b, err = PrefsFromBytes(p2.ToBytes(), false)
@@ -210,11 +210,11 @@ func checkPrefs(t *testing.T, p Prefs) {
 	if p2p != p2bp {
 		t.Fatalf("p2p != p2bp\n%#v\n%#v\n", p2p, p2bp)
 	}
-	if !p2.Equals(&p2b) {
+	if !p2.Equals(p2b) {
 		t.Fatalf("p2 != p2b\n%#v\n%#v\n", p2, p2b)
 	}
-	p2c = *p2.Copy()
-	if !p2b.Equals(&p2c) {
+	p2c = p2.Copy()
+	if !p2b.Equals(p2c) {
 		t.Fatalf("p2b != p2c\n")
 	}
 }