[lxc-devel] [lxd/master] Move forkproxy to using subprocess

stgraber on Github lxc-bot at linuxcontainers.org
Thu Aug 13 22:46:01 UTC 2020


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 301 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200813/2972ba45/attachment.bin>
-------------- next part --------------
From eb97c1ef8f5f001d6eb4e9227eb554f9750ed04b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Thu, 13 Aug 2020 18:27:57 -0400
Subject: [PATCH 1/2] shared/subprocess: Add StartWithFiles
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: St├ęphane Graber <stgraber at ubuntu.com>
---
 shared/subprocess/proc.go | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/shared/subprocess/proc.go b/shared/subprocess/proc.go
index 060637e681..f78b1a1f70 100644
--- a/shared/subprocess/proc.go
+++ b/shared/subprocess/proc.go
@@ -100,6 +100,15 @@ func (p *Process) Stop() error {
 
 // Start will start the given process object
 func (p *Process) Start() error {
+	return p.start(nil)
+}
+
+// StartWithFiles will start the given process object with extra file descriptors
+func (p *Process) StartWithFiles(fds []*os.File) error {
+	return p.start(fds)
+}
+
+func (p *Process) start(fds []*os.File) error {
 	var cmd *exec.Cmd
 
 	if p.Apparmor != "" && p.hasApparmor() {
@@ -117,6 +126,10 @@ func (p *Process) Start() error {
 		cmd.SysProcAttr.Credential.Gid = p.GID
 	}
 
+	if fds != nil {
+		cmd.ExtraFiles = fds
+	}
+
 	// Setup output capture.
 	if p.Stdout != "" {
 		out, err := os.Create(p.Stdout)

From 8e461e015c24fc4b3b02302fdf31fb2f55cddbca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Thu, 13 Aug 2020 18:28:13 -0400
Subject: [PATCH 2/2] lxd/forkproxy: Switch to using subprocess
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: St├ęphane Graber <stgraber at ubuntu.com>
---
 lxd/device/proxy.go   | 75 +++++++++++++++---------------------
 lxd/main_forkproxy.go | 90 ++++++++-----------------------------------
 2 files changed, 47 insertions(+), 118 deletions(-)

diff --git a/lxd/device/proxy.go b/lxd/device/proxy.go
index c3ef87019c..9d3420622e 100644
--- a/lxd/device/proxy.go
+++ b/lxd/device/proxy.go
@@ -2,9 +2,7 @@ package device
 
 import (
 	"bufio"
-	"bytes"
 	"fmt"
-	"io/ioutil"
 	"net"
 	"os"
 	"path/filepath"
@@ -13,7 +11,6 @@ import (
 	"time"
 
 	"github.com/pkg/errors"
-	"golang.org/x/sys/unix"
 	liblxc "gopkg.in/lxc/go-lxc.v2"
 
 	deviceConfig "github.com/lxc/lxd/lxd/device/config"
@@ -24,6 +21,7 @@ import (
 	"github.com/lxc/lxd/lxd/util"
 	"github.com/lxc/lxd/shared"
 	"github.com/lxc/lxd/shared/logger"
+	"github.com/lxc/lxd/shared/subprocess"
 	"github.com/lxc/lxd/shared/validate"
 )
 
@@ -195,10 +193,9 @@ func (d *proxy) Start() (*deviceConfig.RunConfig, error) {
 			logFileName := fmt.Sprintf("proxy.%s.log", d.name)
 			logPath := filepath.Join(d.inst.LogPath(), logFileName)
 
-			_, err = shared.RunCommandInheritFds(
-				proxyValues.inheritFds,
-				d.state.OS.ExecPath,
-				"forkproxy",
+			// Spawn the daemon using subprocess
+			command := d.state.OS.ExecPath
+			forkproxyargs := []string{"forkproxy",
 				"--",
 				proxyValues.listenPid,
 				proxyValues.listenPidFd,
@@ -206,28 +203,43 @@ func (d *proxy) Start() (*deviceConfig.RunConfig, error) {
 				proxyValues.connectPid,
 				proxyValues.connectPidFd,
 				proxyValues.connectAddr,
-				logPath,
-				pidPath,
 				proxyValues.listenAddrGID,
 				proxyValues.listenAddrUID,
 				proxyValues.listenAddrMode,
 				proxyValues.securityGID,
 				proxyValues.securityUID,
 				proxyValues.proxyProtocol,
-			)
+			}
+
+			p, err := subprocess.NewProcess(command, forkproxyargs, logPath, logPath)
+			if err != nil {
+				return fmt.Errorf("Failed to create subprocess: %s", err)
+			}
+
+			err = p.StartWithFiles(proxyValues.inheritFds)
+			if err != nil {
+				return fmt.Errorf("Failed to run: %s %s: %v", command, strings.Join(forkproxyargs, " "), err)
+			}
 			for _, file := range proxyValues.inheritFds {
 				file.Close()
 			}
 
+			err = p.Save(pidPath)
 			if err != nil {
-				return fmt.Errorf("Error occurred when starting proxy device: %s", err)
+				// Kill Process if started, but could not save the file
+				err2 := p.Stop()
+				if err != nil {
+					return fmt.Errorf("Could not kill subprocess while handling saving error: %s: %s", err, err2)
+				}
+
+				return fmt.Errorf("Failed to save subprocess details: %s", err)
 			}
 
 			// Poll log file a few times until we see "Started" to indicate successful start.
 			for i := 0; i < 10; i++ {
 				started, err := d.checkProcStarted(logPath)
-
 				if err != nil {
+					p.Stop()
 					return fmt.Errorf("Error occurred when starting proxy device: %s", err)
 				}
 
@@ -238,6 +250,7 @@ func (d *proxy) Start() (*deviceConfig.RunConfig, error) {
 				time.Sleep(time.Second)
 			}
 
+			p.Stop()
 			return fmt.Errorf("Error occurred when starting proxy device, please look in %s", logPath)
 		},
 	}
@@ -514,45 +527,21 @@ func (d *proxy) setupProxyProcInfo() (*proxyProcInfo, error) {
 }
 
 func (d *proxy) killProxyProc(pidPath string) error {
-	// Get the contents of the pid file
-	contents, err := ioutil.ReadFile(pidPath)
-	if err != nil {
-		return err
-	}
-	pidString := strings.TrimSpace(string(contents))
-
-	// Check if the process still exists
-	if !shared.PathExists(fmt.Sprintf("/proc/%s", pidString)) {
-		os.Remove(pidPath)
-		return nil
-	}
-
-	// Check if it's forkdns
-	cmdArgs, err := ioutil.ReadFile(fmt.Sprintf("/proc/%s/cmdline", pidString))
-	if err != nil {
-		os.Remove(pidPath)
+	// If the pid file doesn't exist, there is no process to kill.
+	if !shared.PathExists(pidPath) {
 		return nil
 	}
 
-	cmdFields := strings.Split(string(bytes.TrimRight(cmdArgs, string("\x00"))), string(byte(0)))
-	if len(cmdFields) < 5 || cmdFields[1] != "forkproxy" {
-		os.Remove(pidPath)
-		return nil
-	}
-
-	// Parse the pid
-	pidInt, err := strconv.Atoi(pidString)
+	p, err := subprocess.ImportProcess(pidPath)
 	if err != nil {
-		return err
+		return fmt.Errorf("Could not read pid file: %s", err)
 	}
 
-	// Actually kill the process
-	err = unix.Kill(pidInt, unix.SIGKILL)
-	if err != nil {
-		return err
+	err = p.Stop()
+	if err != nil && err != subprocess.ErrNotRunning {
+		return fmt.Errorf("Unable to kill forkproxy: %s", err)
 	}
 
-	// Cleanup
 	os.Remove(pidPath)
 	return nil
 }
diff --git a/lxd/main_forkproxy.go b/lxd/main_forkproxy.go
index 196c7c567c..49d810cb55 100644
--- a/lxd/main_forkproxy.go
+++ b/lxd/main_forkproxy.go
@@ -85,13 +85,12 @@ again:
 void forkproxy(void)
 {
 	unsigned int needs_mntns = 0;
-	int connect_pid, connect_pidfd, listen_pid, listen_pidfd, log_fd;
+	int connect_pid, connect_pidfd, listen_pid, listen_pidfd;
 	size_t unix_prefix_len = sizeof("unix:") - 1;
 	ssize_t ret;
 	pid_t pid;
-	char *connect_addr, *cur, *listen_addr, *log_path, *pid_path;
+	char *connect_addr, *cur, *listen_addr;
 	int sk_fds[2] = {-EBADF, -EBADF};
-	FILE *pid_file;
 
 	// Get the pid
 	cur = advance_arg(false);
@@ -106,29 +105,6 @@ void forkproxy(void)
 	connect_pid = atoi(advance_arg(true));
 	connect_pidfd = atoi(advance_arg(true));
 	connect_addr = advance_arg(true);
-	log_path = advance_arg(true);
-	pid_path = advance_arg(true);
-
-	close(STDIN_FILENO);
-	log_fd = open(log_path, O_WRONLY | O_CREAT | O_CLOEXEC | O_TRUNC, 0600);
-	if (log_fd < 0)
-		_exit(EXIT_FAILURE);
-
-	ret = dup3(log_fd, STDOUT_FILENO, O_CLOEXEC);
-	if (ret < 0)
-		_exit(EXIT_FAILURE);
-
-	ret = dup3(log_fd, STDERR_FILENO, O_CLOEXEC);
-	if (ret < 0)
-		_exit(EXIT_FAILURE);
-
-	pid_file = fopen(pid_path, "we+");
-	if (!pid_file) {
-		fprintf(stderr,
-			"%s - Failed to create pid file for proxy daemon\n",
-			strerror(errno));
-		_exit(EXIT_FAILURE);
-	}
 
 	if (strncmp(listen_addr, "udp:", sizeof("udp:") - 1) == 0 &&
 	    strncmp(connect_addr, "udp:", sizeof("udp:") - 1) != 0) {
@@ -166,7 +142,6 @@ void forkproxy(void)
 
 		whoami = FORKPROXY_CHILD;
 
-		fclose(pid_file);
 		ret = close(sk_fds[0]);
 		if (ret < 0)
 			fprintf(stderr, "%s - Failed to close fd %d\n",
@@ -269,41 +244,6 @@ void forkproxy(void)
 		// can just rely on init doing it's job and reaping the zombie
 		// process. So, technically unsatisfying but pragmatically
 		// correct.
-
-		// daemonize
-		pid = fork();
-		if (pid < 0)
-			_exit(EXIT_FAILURE);
-
-		if (pid != 0) {
-			ret = wait_for_pid(pid);
-			if (ret < 0)
-				_exit(EXIT_FAILURE);
-
-			_exit(EXIT_SUCCESS);
-		}
-
-		pid = fork();
-		if (pid < 0)
-			_exit(EXIT_FAILURE);
-
-		if (pid != 0) {
-			ret = fprintf(pid_file, "%d", pid);
-			fclose(pid_file);
-			if (ret < 0) {
-				fprintf(stderr, "Failed to write proxy daemon pid %d to \"%s\"\n",
-					pid, pid_path);
-				ret = EXIT_FAILURE;
-			}
-			close(STDOUT_FILENO);
-			close(STDERR_FILENO);
-			_exit(EXIT_SUCCESS);
-		}
-
-		ret = setsid();
-		if (ret < 0)
-			fprintf(stderr, "%s - Failed to setsid in proxy daemon\n",
-				strerror(errno));
 	}
 }
 */
@@ -338,7 +278,7 @@ func (c *cmdForkproxy) Command() *cobra.Command {
   container, connecting one side to the host and the other to the
   container.
 `
-	cmd.Args = cobra.ExactArgs(14)
+	cmd.Args = cobra.ExactArgs(12)
 	cmd.RunE = c.Run
 	cmd.Hidden = true
 
@@ -459,7 +399,7 @@ func (c *cmdForkproxy) Run(cmd *cobra.Command, args []string) error {
 	}
 
 	// Sanity checks
-	if len(args) != 14 {
+	if len(args) != 12 {
 		cmd.Help()
 
 		if len(args) == 0 {
@@ -529,16 +469,16 @@ func (c *cmdForkproxy) Run(cmd *cobra.Command, args []string) error {
 			var err error
 
 			listenAddrGID := -1
-			if args[8] != "" {
-				listenAddrGID, err = strconv.Atoi(args[8])
+			if args[6] != "" {
+				listenAddrGID, err = strconv.Atoi(args[6])
 				if err != nil {
 					return err
 				}
 			}
 
 			listenAddrUID := -1
-			if args[9] != "" {
-				listenAddrUID, err = strconv.Atoi(args[9])
+			if args[7] != "" {
+				listenAddrUID, err = strconv.Atoi(args[7])
 				if err != nil {
 					return err
 				}
@@ -552,8 +492,8 @@ func (c *cmdForkproxy) Run(cmd *cobra.Command, args []string) error {
 			}
 
 			var listenAddrMode os.FileMode
-			if args[10] != "" {
-				tmp, err := strconv.ParseUint(args[10], 8, 0)
+			if args[8] != "" {
+				tmp, err := strconv.ParseUint(args[8], 8, 0)
 				if err != nil {
 					return err
 				}
@@ -621,16 +561,16 @@ func (c *cmdForkproxy) Run(cmd *cobra.Command, args []string) error {
 
 	// Drop privilege if requested
 	gid := uint64(0)
-	if args[11] != "" {
-		gid, err = strconv.ParseUint(args[11], 10, 32)
+	if args[9] != "" {
+		gid, err = strconv.ParseUint(args[9], 10, 32)
 		if err != nil {
 			return err
 		}
 	}
 
 	uid := uint64(0)
-	if args[12] != "" {
-		uid, err = strconv.ParseUint(args[12], 10, 32)
+	if args[10] != "" {
+		uid, err = strconv.ParseUint(args[10], 10, 32)
 		if err != nil {
 			return err
 		}
@@ -710,7 +650,7 @@ func (c *cmdForkproxy) Run(cmd *cobra.Command, args []string) error {
 				continue
 			}
 
-			err := listenerInstance(epFd, lAddr, cAddr, curFd, srcConn, args[13] == "true")
+			err := listenerInstance(epFd, lAddr, cAddr, curFd, srcConn, args[11] == "true")
 			if err != nil {
 				fmt.Printf("Warning: Failed to prepare new listener instance: %s\n", err)
 			}


More information about the lxc-devel mailing list