commit 15445de248c08d10c285aba654d601a88d741c2a
author Bert Muthalaly <stijlist@google.com> Mon Nov 19 19:00:13 2018 -0500
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Fri Dec 14 23:57:59 2018 +0000
tree eb0a52f9ec4cd8900bc25d8db2a4f3b043b2d623
parent cbd70ffa79bf47db98893f6529b040e26d490553

[netstack] Avoid configuring ethernet clients after they've been started

...fixing the race condition causing Chromium test crashes outlined in
NET-2026.
This logic will be moved into netcfg in the work for NET-1837.

Tested:
  - fx run-test netstack_test
  - fx run-test netstack_integration_test
  - manual test on Toulouse: primary NIC gets a DHCP address from
  edgerouter on boot

Bug: NET-2026 #done
Change-Id: I362f2415abc75c781d48fab0adf035fb3240aa52

go/src/netstack/link/eth/client.go

diff --git a/go/src/netstack/link/eth/client.go b/go/src/netstack/link/eth/client.go
index 451f746..e44034e 100644
--- a/go/src/netstack/link/eth/client.go
+++ b/go/src/netstack/link/eth/client.go
@@ -138,17 +138,11 @@
 		if err := c.rxCompleteLocked(); err != nil {
 			return fmt.Errorf("eth: failed to load rx fifo: %v", err)
 		}
-		if status, err := device.Start(); err != nil {
-			return err
-		} else if err := checkStatus(status, "Start"); err != nil {
-			return err
-		}
 		return nil
 	}(); err != nil {
 		c.closeLocked()
 		return nil, err
 	}
-	c.changeStateLocked(StateStarted)
 
 	return c, nil
 }

go/src/netstack/netstack.go

diff --git a/go/src/netstack/netstack.go b/go/src/netstack/netstack.go
index c47c47e..c35008b 100644
--- a/go/src/netstack/netstack.go
+++ b/go/src/netstack/netstack.go
@@ -233,7 +233,11 @@
 	}
 	if enabled {
 		d.ctx, d.cancel = context.WithCancel(ifs.ctx)
-		d.client.Run(d.ctx)
+		if c := d.client; c != nil {
+			c.Run(d.ctx)
+		} else {
+			panic(fmt.Sprintf("nil DHCP client on interface %s", ifs.nic.Name))
+		}
 	} else if d.cancel != nil {
 		d.cancel()
 	}
@@ -486,16 +490,9 @@
 			return nil
 		}
 
-		status, err := client.GetStatus()
-		if err != nil {
-			return fmt.Errorf("NIC %s: failed to get device status for MAC=%x: %v", ifs.nic.Name, client.Info.Mac, err)
-		}
-
 		switch config.IpAddressConfig.Which() {
 		case netstack.IpAddressConfigDhcp:
-			if status == eth.LinkUp {
-				ifs.setDHCPStatus(true)
-			}
+			ifs.setDHCPStatus(true)
 		case netstack.IpAddressConfigStaticIp:
 			subnet := config.IpAddressConfig.StaticIp
 			protocol, tcpipAddr, retval := ns.validateInterfaceAddress(subnet.Addr, subnet.PrefixLen)
@@ -504,7 +501,7 @@
 			}
 			ns.setInterfaceAddress(ifs.nic.ID, protocol, tcpipAddr, subnet.PrefixLen)
 		}
-		return nil
+		return ifs.eth.Up()
 	})
 }
 

go/src/netstack/netstack_test.go

diff --git a/go/src/netstack/netstack_test.go b/go/src/netstack/netstack_test.go
index c2b42c4..3c2f786 100644
--- a/go/src/netstack/netstack_test.go
+++ b/go/src/netstack/netstack_test.go
@@ -5,8 +5,8 @@
 package main
 
 import (
-	"testing"
 	"syscall/zx"
+	"testing"
 
 	"fidl/fuchsia/netstack"
 	"fidl/zircon/ethernet"
@@ -57,7 +57,27 @@
 	}
 }
 
-func TestInitialDhcpConfiguration(t *testing.T) {
+func TestNicStartedByDefault(t *testing.T) {
+	ns := newNetstack(t)
+
+	startCalled := false
+	eth := deviceForAddEth(ethernet.Info{}, t)
+	eth.StartImpl = func() (int32, error) {
+		startCalled = true
+		return int32(zx.ErrOk), nil
+	}
+
+	_, err := ns.addEth(testTopoPath, netstack.InterfaceConfig{Name: testDeviceName}, &eth)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if !startCalled {
+		t.Errorf("expected ethernet.Device.Start to be called from addEth")
+	}
+}
+
+func TestDhcpConfiguration(t *testing.T) {
 	ns := newNetstack(t)
 
 	ipAddressConfig := netstack.IpAddressConfig{}