From c47f907a27fd636df1aadee6abcb6059c4f751f8 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Thu, 20 Feb 2020 11:07:00 -0800 Subject: [PATCH] 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 --- cmd/tailscale/tailscale.go | 15 +++++----- ipn/backend.go | 2 +- ipn/fake.go | 8 +++-- ipn/handle.go | 15 +++++----- ipn/local.go | 29 +++++++++++------- ipn/message.go | 4 +-- ipn/message_test.go | 3 +- ipn/prefs.go | 60 ++++++++++++-------------------------- ipn/prefs_test.go | 14 ++++----- 9 files changed, 69 insertions(+), 81 deletions(-) diff --git a/cmd/tailscale/tailscale.go b/cmd/tailscale/tailscale.go index 837dd857f..5c65c60c4 100644 --- a/cmd/tailscale/tailscale.go +++ b/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 { diff --git a/ipn/backend.go b/ipn/backend.go index 8988115f1..1695e7076 100644 --- a/ipn/backend.go +++ b/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 diff --git a/ipn/fake.go b/ipn/fake.go index 544993bd9..bb81385f6 100644 --- a/ipn/fake.go +++ b/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) diff --git a/ipn/handle.go b/ipn/handle.go index 397e08631..7ace718c7 100644 --- a/ipn/handle.go +++ b/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) } diff --git a/ipn/local.go b/ipn/local.go index 459df9492..8f65f2908 100644 --- a/ipn/local.go +++ b/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. diff --git a/ipn/message.go b/ipn/message.go index f326d32a4..dee6ec6fb 100644 --- a/ipn/message.go +++ b/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}}) } diff --git a/ipn/message_test.go b/ipn/message_test.go index dfc4067d1..4559f1d4d 100644 --- a/ipn/message_test.go +++ b/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) diff --git a/ipn/prefs.go b/ipn/prefs.go index cfc51137e..5da0685c0 100644 --- a/ipn/prefs.go +++ b/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) { diff --git a/ipn/prefs_test.go b/ipn/prefs_test.go index 302526909..cc29a023a 100644 --- a/ipn/prefs_test.go +++ b/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") } }