ipn: move Options.Notify to its own method

We already had SetNotifyCallback elsewhere on controlclient, so use
that name.

Baby steps towards some CLI refactor work.

Updates tailscale/tailscale#1436

cmd/tailscale/cli/up.go

@@ -238,44 +238,44 @@ func runUp(ctx context.Context, args []string) error {
 	startLoginInteractive := func() { loginOnce.Do(func() { bc.StartLoginInteractive() }) }
 
 	bc.SetPrefs(prefs)
-
-	opts := ipn.Options{
-		StateKey: ipn.GlobalDaemonStateKey,
-		AuthKey:  upArgs.authKey,
-		Notify: func(n ipn.Notify) {
-			if n.ErrMessage != nil {
-				msg := *n.ErrMessage
-				if msg == ipn.ErrMsgPermissionDenied {
-					switch runtime.GOOS {
-					case "windows":
-						msg += " (Tailscale service in use by other user?)"
-					default:
-						msg += " (try 'sudo tailscale up [...]')"
-					}
+	bc.SetNotifyCallback(func(n ipn.Notify) {
+		if n.ErrMessage != nil {
+			msg := *n.ErrMessage
+			if msg == ipn.ErrMsgPermissionDenied {
+				switch runtime.GOOS {
+				case "windows":
+					msg += " (Tailscale service in use by other user?)"
+				default:
+					msg += " (try 'sudo tailscale up [...]')"
 				}
-				fatalf("backend error: %v\n", msg)
 			}
-			if s := n.State; s != nil {
-				switch *s {
-				case ipn.NeedsLogin:
-					printed = true
-					startLoginInteractive()
-				case ipn.NeedsMachineAuth:
-					printed = true
-					fmt.Fprintf(os.Stderr, "\nTo authorize your machine, visit (as admin):\n\n\t%s/admin/machines\n\n", upArgs.server)
-				case ipn.Starting, ipn.Running:
-					// Done full authentication process
-					if printed {
-						// Only need to print an update if we printed the "please click" message earlier.
-						fmt.Fprintf(os.Stderr, "Success.\n")
-					}
-					cancel()
+			fatalf("backend error: %v\n", msg)
+		}
+		if s := n.State; s != nil {
+			switch *s {
+			case ipn.NeedsLogin:
+				printed = true
+				startLoginInteractive()
+			case ipn.NeedsMachineAuth:
+				printed = true
+				fmt.Fprintf(os.Stderr, "\nTo authorize your machine, visit (as admin):\n\n\t%s/admin/machines\n\n", upArgs.server)
+			case ipn.Starting, ipn.Running:
+				// Done full authentication process
+				if printed {
+					// Only need to print an update if we printed the "please click" message earlier.
+					fmt.Fprintf(os.Stderr, "Success.\n")
 				}
+				cancel()
 			}
-			if url := n.BrowseToURL; url != nil {
-				fmt.Fprintf(os.Stderr, "\nTo authenticate, visit:\n\n\t%s\n\n", *url)
-			}
-		},
+		}
+		if url := n.BrowseToURL; url != nil {
+			fmt.Fprintf(os.Stderr, "\nTo authenticate, visit:\n\n\t%s\n\n", *url)
+		}
+	})
+
+	opts := ipn.Options{
+		StateKey: ipn.GlobalDaemonStateKey,
+		AuthKey:  upArgs.authKey,
 	}
 
 	// On Windows, we still run in mostly the "legacy" way that
@@ -293,8 +293,7 @@ func runUp(ctx context.Context, args []string) error {
 		opts.Prefs = prefs
 	}
 
-	// We still have to Start right now because it's the only way to
-	// set up notifications and whatnot. This causes a bunch of churn
+	// We still have to Start for now. This causes a bunch of churn
 	// every time the CLI touches anything.
 	//
 	// TODO(danderson): redo the frontend/backend API to assume

cmd/tailscale/cli/web.go

@@ -259,30 +259,30 @@ func tailscaleUp(ctx context.Context) (authURL string, retErr error) {
 	c, bc, ctx, cancel := connect(ctx)
 	defer cancel()
 
+	bc.SetNotifyCallback(func(n ipn.Notify) {
+		if n.ErrMessage != nil {
+			msg := *n.ErrMessage
+			if msg == ipn.ErrMsgPermissionDenied {
+				switch runtime.GOOS {
+				case "windows":
+					msg += " (Tailscale service in use by other user?)"
+				default:
+					msg += " (try 'sudo tailscale up [...]')"
+				}
+			}
+			retErr = fmt.Errorf("backend error: %v", msg)
+			cancel()
+		} else if url := n.BrowseToURL; url != nil {
+			authURL = *url
+			cancel()
+		}
+	})
+
 	bc.SetPrefs(prefs)
 
-	opts := ipn.Options{
+	bc.Start(ipn.Options{
 		StateKey: ipn.GlobalDaemonStateKey,
-		Notify: func(n ipn.Notify) {
-			if n.ErrMessage != nil {
-				msg := *n.ErrMessage
-				if msg == ipn.ErrMsgPermissionDenied {
-					switch runtime.GOOS {
-					case "windows":
-						msg += " (Tailscale service in use by other user?)"
-					default:
-						msg += " (try 'sudo tailscale up [...]')"
-					}
-				}
-				retErr = fmt.Errorf("backend error: %v", msg)
-				cancel()
-			} else if url := n.BrowseToURL; url != nil {
-				authURL = *url
-				cancel()
-			}
-		},
-	}
-	bc.Start(opts)
+	})
 	bc.StartLoginInteractive()
 	pump(ctx, bc, c)
 

ipn/backend.go

@@ -122,8 +122,6 @@ type Options struct {
 	// TODO(danderson): remove some time after the transition to
 	// tailscaled is done.
 	LegacyConfigPath string
-	// Notify is called when backend events happen.
-	Notify func(Notify) `json:"-"`
 	// HTTPTestClient is an optional HTTP client to pass to controlclient
 	// (for tests only).
 	HTTPTestClient *http.Client
@@ -135,6 +133,9 @@ type Options struct {
 // (It has nothing to do with the interface between the backends
 // and the cloud control plane.)
 type Backend interface {
+	// SetNotifyCallback sets the callback to be called on updates
+	// from the backend to the client.
+	SetNotifyCallback(func(Notify))
 	// Start starts or restarts the backend, typically when a
 	// frontend client connects.
 	Start(Options) error

ipn/fake_test.go

@@ -5,7 +5,6 @@
 package ipn
 
 import (
-	"log"
 	"time"
 
 	"tailscale.com/ipn/ipnstate"
@@ -21,18 +20,28 @@ type FakeBackend struct {
 
 func (b *FakeBackend) Start(opts Options) error {
 	b.serverURL = opts.Prefs.ControlURL
-	if opts.Notify == nil {
-		log.Fatalf("FakeBackend.Start: opts.Notify is nil\n")
+	if b.notify == nil {
+		panic("FakeBackend.Start: SetNotifyCallback not called")
 	}
-	b.notify = opts.Notify
-	b.notify(Notify{Prefs: opts.Prefs})
 	nl := NeedsLogin
-	b.notify(Notify{State: &nl})
+	if b.notify != nil {
+		b.notify(Notify{Prefs: opts.Prefs})
+		b.notify(Notify{State: &nl})
+	}
 	return nil
 }
 
+func (b *FakeBackend) SetNotifyCallback(notify func(Notify)) {
+	if notify == nil {
+		panic("FakeBackend.SetNotifyCallback: notify is nil")
+	}
+	b.notify = notify
+}
+
 func (b *FakeBackend) newState(s State) {
-	b.notify(Notify{State: &s})
+	if b.notify != nil {
+		b.notify(Notify{State: &s})
+	}
 	if s == Running {
 		b.live = true
 	} else {
@@ -42,7 +51,9 @@ func (b *FakeBackend) newState(s State) {
 
 func (b *FakeBackend) StartLoginInteractive() {
 	u := b.serverURL + "/this/is/fake"
-	b.notify(Notify{BrowseToURL: &u})
+	if b.notify != nil {
+		b.notify(Notify{BrowseToURL: &u})
+	}
 	b.login()
 }
 
@@ -54,10 +65,14 @@ func (b *FakeBackend) login() {
 	b.newState(NeedsMachineAuth)
 	b.newState(Stopped)
 	// TODO(apenwarr): Fill in a more interesting netmap here.
-	b.notify(Notify{NetMap: &netmap.NetworkMap{}})
+	if b.notify != nil {
+		b.notify(Notify{NetMap: &netmap.NetworkMap{}})
+	}
 	b.newState(Starting)
 	// TODO(apenwarr): Fill in a more interesting status.
-	b.notify(Notify{Engine: &EngineStatus{}})
+	if b.notify != nil {
+		b.notify(Notify{Engine: &EngineStatus{}})
+	}
 	b.newState(Running)
 }
 
@@ -70,7 +85,9 @@ func (b *FakeBackend) SetPrefs(new *Prefs) {
 		panic("FakeBackend.SetPrefs got nil prefs")
 	}
 
-	b.notify(Notify{Prefs: new.Clone()})
+	if b.notify != nil {
+		b.notify(Notify{Prefs: new.Clone()})
+	}
 	if new.WantRunning && !b.live {
 		b.newState(Starting)
 		b.newState(Running)
@@ -87,13 +104,19 @@ func (b *FakeBackend) EditPrefs(mp *MaskedPrefs) {
 }
 
 func (b *FakeBackend) RequestEngineStatus() {
-	b.notify(Notify{Engine: &EngineStatus{}})
+	if b.notify != nil {
+		b.notify(Notify{Engine: &EngineStatus{}})
+	}
 }
 
 func (b *FakeBackend) FakeExpireAfter(x time.Duration) {
-	b.notify(Notify{NetMap: &netmap.NetworkMap{}})
+	if b.notify != nil {
+		b.notify(Notify{NetMap: &netmap.NetworkMap{}})
+	}
 }
 
 func (b *FakeBackend) Ping(ip string, useTSMP bool) {
-	b.notify(Notify{PingResult: &ipnstate.PingResult{}})
+	if b.notify != nil {
+		b.notify(Notify{PingResult: &ipnstate.PingResult{}})
+	}
 }

ipn/handle.go

@@ -15,25 +15,26 @@ import (
 )
 
 type Handle struct {
-	frontendLogID string
-	b             Backend
-	xnotify       func(Notify)
-	logf          logger.Logf
+	b    Backend
+	logf logger.Logf
 
 	// Mutex protects everything below
 	mu                sync.Mutex
+	xnotify           func(Notify)
+	frontendLogID     string
 	netmapCache       *netmap.NetworkMap
 	engineStatusCache EngineStatus
 	stateCache        State
 	prefsCache        *Prefs
 }
 
-func NewHandle(b Backend, logf logger.Logf, opts Options) (*Handle, error) {
+func NewHandle(b Backend, logf logger.Logf, notify func(Notify), opts Options) (*Handle, error) {
 	h := &Handle{
 		b:    b,
 		logf: logf,
 	}
 
+	h.SetNotifyCallback(notify)
 	err := h.Start(opts)
 	if err != nil {
 		return nil, err
@@ -42,18 +43,25 @@ func NewHandle(b Backend, logf logger.Logf, opts Options) (*Handle, error) {
 	return h, nil
 }
 
+func (h *Handle) SetNotifyCallback(notify func(Notify)) {
+	h.mu.Lock()
+	h.xnotify = notify
+	h.mu.Unlock()
+
+	h.b.SetNotifyCallback(h.notify)
+}
+
 func (h *Handle) Start(opts Options) error {
+	h.mu.Lock()
 	h.frontendLogID = opts.FrontendLogID
-	h.xnotify = opts.Notify
 	h.netmapCache = nil
 	h.engineStatusCache = EngineStatus{}
 	h.stateCache = NoState
 	if opts.Prefs != nil {
 		h.prefsCache = opts.Prefs.Clone()
 	}
-	xopts := opts
-	xopts.Notify = h.notify
-	return h.b.Start(xopts)
+	h.mu.Unlock()
+	return h.b.Start(opts)
 }
 
 func (h *Handle) Reset() {

ipn/ipnlocal/local.go

@@ -519,6 +519,12 @@ func (b *LocalBackend) setWgengineStatus(s *wgengine.Status, err error) {
 	b.send(ipn.Notify{Engine: &es})
 }
 
+func (b *LocalBackend) SetNotifyCallback(notify func(ipn.Notify)) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	b.notify = notify
+}
+
 // Start applies the configuration specified in opts, and starts the
 // state machine.
 //
@@ -585,7 +591,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 	}
 	applyPrefsToHostinfo(hostinfo, b.prefs)
 
-	b.notify = opts.Notify
 	b.setNetMapLocked(nil)
 	persistv := b.prefs.Persist
 	b.mu.Unlock()

ipn/message.go

@@ -189,8 +189,8 @@ func (bs *BackendServer) GotCommand(ctx context.Context, cmd *Command) error {
 		bs.GotQuit = true
 		return errors.New("Quit command received")
 	} else if c := cmd.Start; c != nil {
+		bs.b.SetNotifyCallback(bs.send)
 		opts := c.Opts
-		opts.Notify = bs.send
 		return bs.b.Start(opts)
 	} else if c := cmd.StartLoginInteractive; c != nil {
 		bs.b.StartLoginInteractive()
@@ -287,8 +287,6 @@ func (bc *BackendClient) Quit() error {
 }
 
 func (bc *BackendClient) Start(opts Options) error {
-	bc.notify = opts.Notify
-	opts.Notify = nil // server can't call our function pointer
 	bc.send(Command{Start: &StartArgs{Opts: opts}})
 	return nil // remote Start() errors must be handled remotely
 }

ipn/message_test.go

@@ -90,13 +90,11 @@ func TestClientServer(t *testing.T) {
 	bc = NewBackendClient(clogf, clientToServer)
 
 	ch := make(chan Notify, 256)
-	h, err := NewHandle(bc, clogf, Options{
+	notify := func(n Notify) { ch <- n }
+	h, err := NewHandle(bc, clogf, notify, Options{
 		Prefs: &Prefs{
 			ControlURL: "http://example.com/fake",
 		},
-		Notify: func(n Notify) {
-			ch <- n
-		},
 	})
 	if err != nil {
 		t.Fatalf("NewHandle error: %v\n", err)