virtcontainers: always pass sandbox as a pointer

Currently we sometimes pass it as a pointer and other times not. As a result, the view of sandbox across virtcontainers may not be the same and it costs extra memory copy each time we pass it by value. Fix it by ensuring sandbox is always passed by pointers. Fixes: kata-containers#262 Signed-off-by: Peng Tao <[email protected]>
bergwolf · Apr 26, 2018 · a8caa41 · a8caa41
1 parent 45e3f85
commit a8caa41
Show file tree

Hide file tree

Showing 34 changed files with 130 additions and 130 deletions.
diff --git a/virtcontainers/agent.go b/virtcontainers/agent.go
@@ -143,27 +143,27 @@ type agent interface {
 	exec(sandbox *Sandbox, c Container, cmd Cmd) (*Process, error)
 
 	// startSandbox will tell the agent to start all containers related to the Sandbox.
-	startSandbox(sandbox Sandbox) error
+	startSandbox(sandbox *Sandbox) error
 
 	// stopSandbox will tell the agent to stop all containers related to the Sandbox.
-	stopSandbox(sandbox Sandbox) error
+	stopSandbox(sandbox *Sandbox) error
 
 	// createContainer will tell the agent to create a container related to a Sandbox.
 	createContainer(sandbox *Sandbox, c *Container) (*Process, error)
 
 	// startContainer will tell the agent to start a container related to a Sandbox.
-	startContainer(sandbox Sandbox, c *Container) error
+	startContainer(sandbox *Sandbox, c *Container) error
 
 	// stopContainer will tell the agent to stop a container related to a Sandbox.
-	stopContainer(sandbox Sandbox, c Container) error
+	stopContainer(sandbox *Sandbox, c Container) error
 
 	// killContainer will tell the agent to send a signal to a
 	// container related to a Sandbox. If all is true, all processes in
 	// the container will be sent the signal.
-	killContainer(sandbox Sandbox, c Container, signal syscall.Signal, all bool) error
+	killContainer(sandbox *Sandbox, c Container, signal syscall.Signal, all bool) error
 
 	// processListContainer will list the processes running inside the container
-	processListContainer(sandbox Sandbox, c Container, options ProcessListOptions) (ProcessList, error)
+	processListContainer(sandbox *Sandbox, c Container, options ProcessListOptions) (ProcessList, error)
 
 	// onlineCPUMem will online CPUs and Memory inside the Sandbox.
 	// This function should be called after hot adding vCPUs or Memory.

diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go
@@ -528,7 +528,7 @@ func TestStartSandboxHyperstartAgentSuccessful(t *testing.T) {
 	pImpl, ok := p.(*Sandbox)
 	assert.True(t, ok)
 
-	bindUnmountAllRootfs(defaultSharedDir, *pImpl)
+	bindUnmountAllRootfs(defaultSharedDir, pImpl)
 }
 
 func TestStartSandboxKataAgentSuccessful(t *testing.T) {
@@ -568,7 +568,7 @@ func TestStartSandboxKataAgentSuccessful(t *testing.T) {
 	pImpl, ok := p.(*Sandbox)
 	assert.True(t, ok)
 
-	bindUnmountAllRootfs(defaultSharedDir, *pImpl)
+	bindUnmountAllRootfs(defaultSharedDir, pImpl)
 }
 
 func TestStartSandboxFailing(t *testing.T) {
@@ -800,7 +800,7 @@ func TestRunSandboxHyperstartAgentSuccessful(t *testing.T) {
 	pImpl, ok := p.(*Sandbox)
 	assert.True(t, ok)
 
-	bindUnmountAllRootfs(defaultSharedDir, *pImpl)
+	bindUnmountAllRootfs(defaultSharedDir, pImpl)
 }
 
 func TestRunSandboxKataAgentSuccessful(t *testing.T) {
@@ -846,7 +846,7 @@ func TestRunSandboxKataAgentSuccessful(t *testing.T) {
 	pImpl, ok := p.(*Sandbox)
 	assert.True(t, ok)
 
-	bindUnmountAllRootfs(defaultSharedDir, *pImpl)
+	bindUnmountAllRootfs(defaultSharedDir, pImpl)
 }
 
 func TestRunSandboxFailing(t *testing.T) {
@@ -1392,7 +1392,7 @@ func TestStartStopContainerHyperstartAgentSuccessful(t *testing.T) {
 	pImpl, ok := p.(*Sandbox)
 	assert.True(t, ok)
 
-	bindUnmountAllRootfs(defaultSharedDir, *pImpl)
+	bindUnmountAllRootfs(defaultSharedDir, pImpl)
 }
 
 func TestStartStopSandboxHyperstartAgentSuccessfulWithCNINetwork(t *testing.T) {
@@ -1649,7 +1649,7 @@ func TestEnterContainerHyperstartAgentSuccessful(t *testing.T) {
 	pImpl, ok := p.(*Sandbox)
 	assert.True(t, ok)
 
-	bindUnmountAllRootfs(defaultSharedDir, *pImpl)
+	bindUnmountAllRootfs(defaultSharedDir, pImpl)
 }
 
 func TestEnterContainerFailingNoSandbox(t *testing.T) {

diff --git a/virtcontainers/cc_proxy.go b/virtcontainers/cc_proxy.go
@@ -13,7 +13,7 @@ type ccProxy struct {
 }
 
 // start is the proxy start implementation for ccProxy.
-func (p *ccProxy) start(sandbox Sandbox, params proxyParams) (int, string, error) {
+func (p *ccProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) {
 	config, err := newProxyConfig(sandbox.config)
 	if err != nil {
 		return -1, "", err
@@ -38,6 +38,6 @@ func (p *ccProxy) start(sandbox Sandbox, params proxyParams) (int, string, error
 	return cmd.Process.Pid, proxyURL, nil
 }
 
-func (p *ccProxy) stop(sandbox Sandbox, pid int) error {
+func (p *ccProxy) stop(sandbox *Sandbox, pid int) error {
 	return nil
 }
diff --git a/virtcontainers/cc_proxy_test.go b/virtcontainers/cc_proxy_test.go
@@ -25,7 +25,7 @@ func TestCCProxyStart(t *testing.T) {
 	proxy := &ccProxy{}
 
 	type testData struct {
-		sandbox     Sandbox
+		sandbox     *Sandbox
 		expectedURI string
 		expectError bool
 	}
@@ -35,16 +35,16 @@ func TestCCProxyStart(t *testing.T) {
 	expectedURI := fmt.Sprintf("unix://%s", expectedSocketPath)
 
 	data := []testData{
-		{Sandbox{}, "", true},
+		{&Sandbox{}, "", true},
 		{
-			Sandbox{
+			&Sandbox{
 				config: &SandboxConfig{
 					ProxyType: "invalid",
 				},
 			}, "", true,
 		},
 		{
-			Sandbox{
+			&Sandbox{
 				config: &SandboxConfig{
 					ProxyType:   CCProxyType,
 					ProxyConfig: ProxyConfig{
@@ -54,7 +54,7 @@ func TestCCProxyStart(t *testing.T) {
 			}, "", true,
 		},
 		{
-			Sandbox{
+			&Sandbox{
 				config: &SandboxConfig{
 					ProxyType: CCProxyType,
 					ProxyConfig: ProxyConfig{
@@ -64,7 +64,7 @@ func TestCCProxyStart(t *testing.T) {
 			}, "", true,
 		},
 		{
-			Sandbox{
+			&Sandbox{
 				id: testSandboxID,
 				config: &SandboxConfig{
 					ProxyType: CCProxyType,

diff --git a/virtcontainers/cc_shim.go b/virtcontainers/cc_shim.go
@@ -14,7 +14,7 @@ type ccShim struct{}
 // start is the ccShim start implementation.
 // It starts the cc-shim binary with URL and token flags provided by
 // the proxy.
-func (s *ccShim) start(sandbox Sandbox, params ShimParams) (int, error) {
+func (s *ccShim) start(sandbox *Sandbox, params ShimParams) (int, error) {
 	if sandbox.config == nil {
 		return -1, fmt.Errorf("Sandbox config cannot be nil")
 	}

diff --git a/virtcontainers/cc_shim_test.go b/virtcontainers/cc_shim_test.go
@@ -35,7 +35,7 @@ func getMockCCShimBinPath() string {
 	return DefaultMockCCShimBinPath
 }
 
-func testCCShimStart(t *testing.T, sandbox Sandbox, params ShimParams, expectFail bool) {
+func testCCShimStart(t *testing.T, sandbox *Sandbox, params ShimParams, expectFail bool) {
 	s := &ccShim{}
 
 	pid, err := s.start(sandbox, params)
@@ -58,19 +58,19 @@ func testCCShimStart(t *testing.T, sandbox Sandbox, params ShimParams, expectFai
 }
 
 func TestCCShimStartNilSandboxConfigFailure(t *testing.T) {
-	testCCShimStart(t, Sandbox{}, ShimParams{}, true)
+	testCCShimStart(t, &Sandbox{}, ShimParams{}, true)
 }
 
 func TestCCShimStartNilShimConfigFailure(t *testing.T) {
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{},
 	}
 
 	testCCShimStart(t, sandbox, ShimParams{}, true)
 }
 
 func TestCCShimStartShimPathEmptyFailure(t *testing.T) {
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType:   CCShimType,
 			ShimConfig: ShimConfig{},
@@ -81,7 +81,7 @@ func TestCCShimStartShimPathEmptyFailure(t *testing.T) {
 }
 
 func TestCCShimStartShimTypeInvalid(t *testing.T) {
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType:   "foo",
 			ShimConfig: ShimConfig{},
@@ -92,7 +92,7 @@ func TestCCShimStartShimTypeInvalid(t *testing.T) {
 }
 
 func TestCCShimStartParamsTokenEmptyFailure(t *testing.T) {
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType: CCShimType,
 			ShimConfig: ShimConfig{
@@ -105,7 +105,7 @@ func TestCCShimStartParamsTokenEmptyFailure(t *testing.T) {
 }
 
 func TestCCShimStartParamsURLEmptyFailure(t *testing.T) {
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType: CCShimType,
 			ShimConfig: ShimConfig{
@@ -122,7 +122,7 @@ func TestCCShimStartParamsURLEmptyFailure(t *testing.T) {
 }
 
 func TestCCShimStartParamsContainerEmptyFailure(t *testing.T) {
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType: CCShimType,
 			ShimConfig: ShimConfig{
@@ -148,7 +148,7 @@ func TestCCShimStartParamsInvalidCommand(t *testing.T) {
 
 	cmd := filepath.Join(dir, "does-not-exist")
 
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType: CCShimType,
 			ShimConfig: ShimConfig{
@@ -165,16 +165,16 @@ func TestCCShimStartParamsInvalidCommand(t *testing.T) {
 	testCCShimStart(t, sandbox, params, true)
 }
 
-func startCCShimStartWithoutConsoleSuccessful(t *testing.T, detach bool) (*os.File, *os.File, *os.File, Sandbox, ShimParams, error) {
+func startCCShimStartWithoutConsoleSuccessful(t *testing.T, detach bool) (*os.File, *os.File, *os.File, *Sandbox, ShimParams, error) {
 	saveStdout := os.Stdout
 	rStdout, wStdout, err := os.Pipe()
 	if err != nil {
-		return nil, nil, nil, Sandbox{}, ShimParams{}, err
+		return nil, nil, nil, &Sandbox{}, ShimParams{}, err
 	}
 
 	os.Stdout = wStdout
 
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType: CCShimType,
 			ShimConfig: ShimConfig{
@@ -260,7 +260,7 @@ func TestCCShimStartDetachSuccessful(t *testing.T) {
 }
 
 func TestCCShimStartWithConsoleNonExistingFailure(t *testing.T) {
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType: CCShimType,
 			ShimConfig: ShimConfig{
@@ -336,7 +336,7 @@ func TestCCShimStartWithConsoleSuccessful(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	sandbox := Sandbox{
+	sandbox := &Sandbox{
 		config: &SandboxConfig{
 			ShimType: CCShimType,
 			ShimConfig: ShimConfig{

diff --git a/virtcontainers/cni.go b/virtcontainers/cni.go
@@ -59,7 +59,7 @@ func convertCNIResult(cniResult cniTypes.Result) (NetworkInfo, error) {
 	}
 }
 
-func (n *cni) invokePluginsAdd(sandbox Sandbox, networkNS *NetworkNamespace) (*NetworkInfo, error) {
+func (n *cni) invokePluginsAdd(sandbox *Sandbox, networkNS *NetworkNamespace) (*NetworkInfo, error) {
 	netPlugin, err := cniPlugin.NewNetworkPlugin()
 	if err != nil {
 		return nil, err
@@ -86,7 +86,7 @@ func (n *cni) invokePluginsAdd(sandbox Sandbox, networkNS *NetworkNamespace) (*N
 	return &netInfo, nil
 }
 
-func (n *cni) invokePluginsDelete(sandbox Sandbox, networkNS NetworkNamespace) error {
+func (n *cni) invokePluginsDelete(sandbox *Sandbox, networkNS NetworkNamespace) error {
 	netPlugin, err := cniPlugin.NewNetworkPlugin()
 	if err != nil {
 		return err
@@ -130,7 +130,7 @@ func (n *cni) run(networkNSPath string, cb func() error) error {
 }
 
 // add adds all needed interfaces inside the network namespace for the CNI network.
-func (n *cni) add(sandbox Sandbox, config NetworkConfig, netNsPath string, netNsCreated bool) (NetworkNamespace, error) {
+func (n *cni) add(sandbox *Sandbox, config NetworkConfig, netNsPath string, netNsCreated bool) (NetworkNamespace, error) {
 
 	networkNS := NetworkNamespace{
 		NetNsPath:    netNsPath,
@@ -155,7 +155,7 @@ func (n *cni) add(sandbox Sandbox, config NetworkConfig, netNsPath string, netNs
 
 // remove unbridges and deletes TAP interfaces. It also removes virtual network
 // interfaces and deletes the network namespace for the CNI network.
-func (n *cni) remove(sandbox Sandbox, networkNS NetworkNamespace) error {
+func (n *cni) remove(sandbox *Sandbox, networkNS NetworkNamespace) error {
 	if err := removeNetworkCommon(networkNS); err != nil {
 		return err
 	}

diff --git a/virtcontainers/cnm.go b/virtcontainers/cnm.go
@@ -28,7 +28,7 @@ func (n *cnm) run(networkNSPath string, cb func() error) error {
 }
 
 // add adds all needed interfaces inside the network namespace for the CNM network.
-func (n *cnm) add(sandbox Sandbox, config NetworkConfig, netNsPath string, netNsCreated bool) (NetworkNamespace, error) {
+func (n *cnm) add(sandbox *Sandbox, config NetworkConfig, netNsPath string, netNsCreated bool) (NetworkNamespace, error) {
 	endpoints, err := createEndpointsFromScan(netNsPath, config)
 	if err != nil {
 		return NetworkNamespace{}, err
@@ -49,7 +49,7 @@ func (n *cnm) add(sandbox Sandbox, config NetworkConfig, netNsPath string, netNs
 
 // remove unbridges and deletes TAP interfaces. It also removes virtual network
 // interfaces and deletes the network namespace for the CNM network.
-func (n *cnm) remove(sandbox Sandbox, networkNS NetworkNamespace) error {
+func (n *cnm) remove(sandbox *Sandbox, networkNS NetworkNamespace) error {
 	if err := removeNetworkCommon(networkNS); err != nil {
 		return err
 	}

diff --git a/virtcontainers/container.go b/virtcontainers/container.go
@@ -598,7 +598,7 @@ func (c *Container) start() error {
 		return err
 	}
 
-	if err := c.sandbox.agent.startContainer(*(c.sandbox), c); err != nil {
+	if err := c.sandbox.agent.startContainer(c.sandbox, c); err != nil {
 		c.Logger().WithError(err).Error("Failed to start container")
 
 		if err := c.stop(); err != nil {
@@ -652,7 +652,7 @@ func (c *Container) stop() error {
 	// return an error, but instead try to kill it forcefully.
 	if err := waitForShim(c.process.Pid); err != nil {
 		// Force the container to be killed.
-		if err := c.sandbox.agent.killContainer(*(c.sandbox), *c, syscall.SIGKILL, true); err != nil {
+		if err := c.sandbox.agent.killContainer(c.sandbox, *c, syscall.SIGKILL, true); err != nil {
 			return err
 		}
 
@@ -665,7 +665,7 @@ func (c *Container) stop() error {
 		}
 	}
 
-	if err := c.sandbox.agent.stopContainer(*(c.sandbox), *c); err != nil {
+	if err := c.sandbox.agent.stopContainer(c.sandbox, *c); err != nil {
 		return err
 	}
 
@@ -712,7 +712,7 @@ func (c *Container) kill(signal syscall.Signal, all bool) error {
 		return fmt.Errorf("Container not ready or running, impossible to signal the container")
 	}
 
-	return c.sandbox.agent.killContainer(*(c.sandbox), *c, signal, all)
+	return c.sandbox.agent.killContainer(c.sandbox, *c, signal, all)
 }
 
 func (c *Container) processList(options ProcessListOptions) (ProcessList, error) {
@@ -724,7 +724,7 @@ func (c *Container) processList(options ProcessListOptions) (ProcessList, error)
 		return nil, fmt.Errorf("Container not running, impossible to list processes")
 	}
 
-	return c.sandbox.agent.processListContainer(*(c.sandbox), *c, options)
+	return c.sandbox.agent.processListContainer(c.sandbox, *c, options)
 }
 
 func (c *Container) hotplugDrive() error {

diff --git a/virtcontainers/filesystem.go b/virtcontainers/filesystem.go
@@ -95,7 +95,7 @@ var runStoragePath = filepath.Join("/run", storagePathSuffix)
 // The default resource storage implementation is filesystem.
 type resourceStorage interface {
 	// Create all resources for a sandbox
-	createAllResources(sandbox Sandbox) error
+	createAllResources(sandbox *Sandbox) error
 
 	// Resources URIs functions return both the URI
 	// for the actual resource and the URI base.
@@ -140,7 +140,7 @@ func (fs *filesystem) Logger() *logrus.Entry {
 	return virtLog.WithField("subsystem", "filesystem")
 }
 
-func (fs *filesystem) createAllResources(sandbox Sandbox) (err error) {
+func (fs *filesystem) createAllResources(sandbox *Sandbox) (err error) {
 	for _, resource := range []sandboxResource{stateFileType, configFileType} {
 		_, path, _ := fs.sandboxURI(sandbox.id, resource)
 		err = os.MkdirAll(path, dirMode)