From 5fb9e00ecfd0bb76b05fa20d80af899d34b149ce Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 31 Mar 2021 22:32:07 -0700 Subject: [PATCH] net/dns/resolver: remove Start method, fully spin up in New instead. Signed-off-by: David Anderson --- net/dns/resolver/tsdns.go | 14 ++----- net/dns/resolver/tsdns_test.go | 76 +++++++++++++++++----------------- wgengine/userspace.go | 10 +++-- 3 files changed, 47 insertions(+), 53 deletions(-) diff --git a/net/dns/resolver/tsdns.go b/net/dns/resolver/tsdns.go index 3d04777ce..09214b43b 100644 --- a/net/dns/resolver/tsdns.go +++ b/net/dns/resolver/tsdns.go @@ -84,7 +84,7 @@ type Resolver struct { // New returns a new resolver. // linkMon optionally specifies a link monitor to use for socket rebinding. -func New(logf logger.Logf, linkMon *monitor.Mon) *Resolver { +func New(logf logger.Logf, linkMon *monitor.Mon) (*Resolver, error) { r := &Resolver{ logf: logger.WithPrefix(logf, "dns: "), linkMon: linkMon, @@ -98,20 +98,14 @@ func New(logf logger.Logf, linkMon *monitor.Mon) *Resolver { r.unregLinkMon = r.linkMon.RegisterChangeCallback(r.onLinkMonitorChange) } - return r -} - -func (r *Resolver) Start() error { - if r.forwarder != nil { - if err := r.forwarder.Start(); err != nil { - return err - } + if err := r.forwarder.Start(); err != nil { + return nil, err } r.wg.Add(1) go r.poll() - return nil + return r, nil } // Close shuts down the resolver and ensures poll goroutines have exited. diff --git a/net/dns/resolver/tsdns_test.go b/net/dns/resolver/tsdns_test.go index fc1491b51..9ee7d11ac 100644 --- a/net/dns/resolver/tsdns_test.go +++ b/net/dns/resolver/tsdns_test.go @@ -194,14 +194,14 @@ func TestRDNSNameToIPv6(t *testing.T) { } func TestResolve(t *testing.T) { - r := New(t.Logf, nil) - r.SetMap(dnsMap) - - if err := r.Start(); err != nil { + r, err := New(t.Logf, nil) + if err != nil { t.Fatalf("start: %v", err) } defer r.Close() + r.SetMap(dnsMap) + tests := []struct { name string qname string @@ -240,14 +240,14 @@ func TestResolve(t *testing.T) { } func TestResolveReverse(t *testing.T) { - r := New(t.Logf, nil) - r.SetMap(dnsMap) - - if err := r.Start(); err != nil { + r, err := New(t.Logf, nil) + if err != nil { t.Fatalf("start: %v", err) } defer r.Close() + r.SetMap(dnsMap) + tests := []struct { name string ip netaddr.IP @@ -318,18 +318,18 @@ func TestDelegate(t *testing.T) { return } - r := New(t.Logf, nil) + r, err := New(t.Logf, nil) + if err != nil { + t.Fatalf("start: %v", err) + } + defer r.Close() + r.SetMap(dnsMap) r.SetUpstreams([]net.Addr{ v4server.PacketConn.LocalAddr(), v6server.PacketConn.LocalAddr(), }) - if err := r.Start(); err != nil { - t.Fatalf("start: %v", err) - } - defer r.Close() - tests := []struct { title string query []byte @@ -397,15 +397,15 @@ func TestDelegateCollision(t *testing.T) { } defer server.Shutdown() - r := New(t.Logf, nil) - r.SetMap(dnsMap) - r.SetUpstreams([]net.Addr{server.PacketConn.LocalAddr()}) - - if err := r.Start(); err != nil { + r, err := New(t.Logf, nil) + if err != nil { t.Fatalf("start: %v", err) } defer r.Close() + r.SetMap(dnsMap) + r.SetUpstreams([]net.Addr{server.PacketConn.LocalAddr()}) + packets := []struct { qname string qtype dns.Type @@ -463,9 +463,8 @@ func TestDelegateCollision(t *testing.T) { } func TestConcurrentSetMap(t *testing.T) { - r := New(t.Logf, nil) - - if err := r.Start(); err != nil { + r, err := New(t.Logf, nil) + if err != nil { t.Fatalf("start: %v", err) } defer r.Close() @@ -499,14 +498,14 @@ func TestConcurrentSetUpstreams(t *testing.T) { } defer server.Shutdown() - r := New(t.Logf, nil) - r.SetMap(dnsMap) - - if err := r.Start(); err != nil { + r, err := New(t.Logf, nil) + if err != nil { t.Fatalf("start: %v", err) } defer r.Close() + r.SetMap(dnsMap) + packet := dnspacket("test.site.", dns.TypeA) // This is purely to ensure that delegation does not race with SetUpstreams. var wg sync.WaitGroup @@ -670,14 +669,14 @@ var emptyResponse = []byte{ } func TestFull(t *testing.T) { - r := New(t.Logf, nil) - r.SetMap(dnsMap) - - if err := r.Start(); err != nil { + r, err := New(t.Logf, nil) + if err != nil { t.Fatalf("start: %v", err) } defer r.Close() + r.SetMap(dnsMap) + // One full packet and one error packet tests := []struct { name string @@ -709,13 +708,12 @@ func TestFull(t *testing.T) { } func TestAllocs(t *testing.T) { - r := New(t.Logf, nil) - r.SetMap(dnsMap) - - if err := r.Start(); err != nil { + r, err := New(t.Logf, nil) + if err != nil { t.Fatalf("start: %v", err) } defer r.Close() + r.SetMap(dnsMap) // It is seemingly pointless to test allocs in the delegate path, // as dialer.Dial -> Read -> Write alone comprise 12 allocs. @@ -778,15 +776,15 @@ func BenchmarkFull(b *testing.B) { } defer server.Shutdown() - r := New(b.Logf, nil) - r.SetMap(dnsMap) - r.SetUpstreams([]net.Addr{server.PacketConn.LocalAddr()}) - - if err := r.Start(); err != nil { + r, err := New(b.Logf, nil) + if err != nil { b.Fatalf("start: %v", err) } defer r.Close() + r.SetMap(dnsMap) + r.SetUpstreams([]net.Addr{server.PacketConn.LocalAddr()}) + tests := []struct { name string request []byte diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 35195dd5b..28f79501b 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -219,7 +219,11 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) e.linkMonOwned = true } - e.resolver = resolver.New(logf, e.linkMon) + var err error + e.resolver, err = resolver.New(logf, e.linkMon) + if err != nil { + return nil, err + } logf("link state: %+v", e.linkMon.InterfaceState()) @@ -246,7 +250,7 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) NoteRecvActivity: e.noteReceiveActivity, LinkMonitor: e.linkMon, } - var err error + e.magicConn, err = magicsock.NewConn(magicsockOpts) if err != nil { return nil, fmt.Errorf("wgengine: %v", err) @@ -374,8 +378,6 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) e.logf("Starting magicsock...") e.magicConn.Start() - e.logf("Starting resolver...") - e.resolver.Start() go e.pollResolver() e.logf("Engine created.")