[PATCH 0/6] Fix bug 215 and some related issues with fd handling

[PATCH 0/6] Fix bug 215 and some related issues with fd handling

[David Gibson]

Stefano, you called it correctly.  While working on bug 215, as usual,
I found a bunch of adjacent things to clean up.

I did start by attempting the appoach you suggested for bug 215 -
remembering which of the low fds were standard streams and avoiding
closing them in __daemon().  It is indeed shorter, but only by 1-2
lines.  Looking at possible interactions with other things, I became
more and more convinced that leaving anything other than the standard
streams in fds 0-2 was an accident waiting to happen.  Much of the
rest of the series is, for example, dealing with the possibility of
--fd [012].

David Gibson (6):
  isolation, util: Fold close_open_files() into isolate_initial()
  isolation, conf: Set c->fd_tap from eary parse of --fd
  conf: Make conf_tap_fd() operate more like conf_mode()
  isolation: Move --fd descriptor to a number of our choosing
  main: Ensure fds 0-2 are populated
  passt: *Always* close pidfile_fd, not just when daemonizing

 conf.c      | 31 +++++++++++++++++++++++--------
 conf.h      |  2 +-
 isolation.c | 32 ++++++++++++++++++++++++++++++--
 isolation.h |  2 +-
 passt.c     | 27 +++++++++++++++++----------
 util.c      | 52 +---------------------------------------------------
 util.h      |  1 -
 7 files changed, 73 insertions(+), 74 deletions(-)

-- 
2.55.0

[PATCH 1/6] isolation, util: Fold close_open_files() into isolate_initial()

[David Gibson]

Most functions in util.c are, well, utilities, that are useful in a bunch
of places.  close_open_files(), however, is very specific, it's only called
from isolate_initial(), and really only can be called from there - it's
logic encodes a very specific action we want to take at that point.

So, remove it from util.c and fold into isolate_initial().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 isolation.c | 37 ++++++++++++++++++++++++++++++++++++-
 util.c      | 49 -------------------------------------------------
 util.h      |  1 -
 3 files changed, 36 insertions(+), 51 deletions(-)

diff --git a/isolation.c b/isolation.c
index c868e668..07b281bb 100644
--- a/isolation.c
+++ b/isolation.c
@@ -58,6 +58,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <getopt.h>
 #include <grp.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -88,6 +89,7 @@
 #include "passt.h"
 #include "log.h"
 #include "isolation.h"
+#include "conf.h"
 
 #define CAP_VERSION	_LINUX_CAPABILITY_VERSION_3
 #define CAP_WORDS	_LINUX_CAPABILITY_U32S_3
@@ -204,6 +206,9 @@ static int move_root(void)
  */
 void isolate_initial(int argc, char **argv)
 {
+	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
+					{ 0 }, };
+	int name, rc, fd_tap = -1;
 	uint64_t keep;
 
 	/* We want to keep CAP_NET_BIND_SERVICE in the initial
@@ -244,7 +249,37 @@ void isolate_initial(int argc, char **argv)
 
 	drop_caps_ep_except(keep);
 
-	close_open_files(argc, argv);
+	do {
+		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
+
+		if (name == 'F')
+			fd_tap = conf_tap_fd(optarg);
+	} while (name != -1);
+
+	if (fd_tap == -1) {
+		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	} else if (fd_tap == STDERR_FILENO + 1) { /* Still a single range */
+		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
+	} else {
+		rc = close_range(STDERR_FILENO + 1, fd_tap - 1,
+				 CLOSE_RANGE_UNSHARE);
+		if (!rc)
+			rc = close_range(fd_tap + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	}
+
+	if (rc) {
+		if (errno == ENOSYS || errno == EINVAL) {
+			/* This probably means close_range() or the
+			 * CLOSE_RANGE_UNSHARE flag is not supported by the
+			 * kernel.  Not much we can do here except carry on and
+			 * hope for the best.
+			 */
+			warn(
+"Can't use close_range() to ensure no files leaked by parent");
+		} else {
+			die_perror("Failed to close files leaked by parent");
+		}
+	}
 }
 
 /**
diff --git a/util.c b/util.c
index 4bc5d6f8..ce5021a9 100644
--- a/util.c
+++ b/util.c
@@ -26,7 +26,6 @@
 #include <stdbool.h>
 #include <linux/errqueue.h>
 #include <linux/in6.h>
-#include <getopt.h>
 
 #include "linux_dep.h"
 #include "util.h"
@@ -38,7 +37,6 @@
 #include "epoll_ctl.h"
 #include "pasta.h"
 #include "serialise.h"
-#include "conf.h"
 #ifdef HAS_GETRANDOM
 #include <sys/random.h>
 #endif
@@ -924,53 +922,6 @@ const char *str_ee_origin(const struct sock_extended_err *ee)
 	return "<invalid>";
 }
 
-/**
- * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr
- * @argc:	Argument count
- * @argv:	Command line options, as we need to skip any file given via --fd
- */
-void close_open_files(int argc, char **argv)
-{
-	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
-					{ 0 },
-				      };
-	long fd = -1;
-	int name, rc;
-
-	do {
-		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
-
-		if (name == 'F')
-			fd = conf_tap_fd(optarg);
-	} while (name != -1);
-
-	if (fd == -1) {
-		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	} else if (fd == STDERR_FILENO + 1) { /* Still a single range */
-		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
-	} else {
-		rc = close_range(STDERR_FILENO + 1, fd - 1,
-				 CLOSE_RANGE_UNSHARE);
-		if (!rc)
-			rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	}
-
-	if (rc) {
-		if (errno == ENOSYS || errno == EINVAL) {
-			/* This probably means close_range() or the
-			 * CLOSE_RANGE_UNSHARE flag is not supported by the
-			 * kernel.  Not much we can do here except carry on and
-			 * hope for the best.
-			 */
-			warn(
-"Can't use close_range() to ensure no files leaked by parent");
-		} else {
-			die_perror("Failed to close files leaked by parent");
-		}
-	}
-
-}
-
 /**
  * snprintf_check() - snprintf() wrapper, checking for truncation and errors
  * @str:	Output buffer
diff --git a/util.h b/util.h
index 90e8a20d..2435f536 100644
--- a/util.h
+++ b/util.h
@@ -168,7 +168,6 @@ intmax_t read_file_integer(const char *path, intmax_t fallback);
 int write_remainder(int fd, const struct iovec *iov, size_t iovcnt,
 		    size_t skip, size_t length);
 int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip);
-void close_open_files(int argc, char **argv);
 bool snprintf_check(char *str, size_t size, const char *format, ...);
 long clamped_scale(long x, long y, long lo, long hi, long f);
 
-- 
2.55.0

[PATCH 2/6] isolation, conf: Set c->fd_tap from eary parse of --fd

[David Gibson]

We parse --fd twice: once in isolate_initial() just to avoid clobbering
the passed in fd.  Then we parse it "for real" in conf(), to set c->fd_tap
and other configuration variables.

Change this, so that we return the value parsed early from
isolate_initial() and set c->fd_tap from that.  This doesn't accomplish
much immediately, but will make some further cleanups possible.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c      | 2 +-
 isolation.c | 6 +++++-
 isolation.h | 2 +-
 passt.c     | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/conf.c b/conf.c
index 6d83daef..41c9d557 100644
--- a/conf.c
+++ b/conf.c
@@ -1593,7 +1593,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->fd_control_listen = c->fd_control = -1;
 			break;
 		case 'F':
-			c->fd_tap = conf_tap_fd(optarg);
+			/* --fd was parsed early and c->fd_tap set in main() */
 			c->one_off = true;
 			*c->sock_path = 0;
 			break;
diff --git a/isolation.c b/isolation.c
index 07b281bb..4bb79e34 100644
--- a/isolation.c
+++ b/isolation.c
@@ -203,8 +203,10 @@ static int move_root(void)
  *  - close all open files except for standard streams and the one from --fd
  * Mustn't:
  *  - remove filesystem access (we need to access files during setup)
+ *
+ * Return: fd number from --fd, or -1 if not specified
  */
-void isolate_initial(int argc, char **argv)
+int isolate_initial(int argc, char **argv)
 {
 	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
 					{ 0 }, };
@@ -280,6 +282,8 @@ void isolate_initial(int argc, char **argv)
 			die_perror("Failed to close files leaked by parent");
 		}
 	}
+
+	return fd_tap;
 }
 
 /**
diff --git a/isolation.h b/isolation.h
index 66b6968d..ef27c7d5 100644
--- a/isolation.h
+++ b/isolation.h
@@ -10,7 +10,7 @@
 #include <stdbool.h>
 #include <unistd.h>
 
-void isolate_initial(int argc, char **argv);
+int isolate_initial(int argc, char **argv);
 void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool use_userns,
 		  const char *userns);
 int isolate_prefork(const struct ctx *c);
diff --git a/passt.c b/passt.c
index 65a07d72..b99e5998 100644
--- a/passt.c
+++ b/passt.c
@@ -341,7 +341,7 @@ int main(int argc, char **argv)
 
 	arch_avx2_exec(argv);
 
-	isolate_initial(argc, argv);
+	c->fd_tap = isolate_initial(argc, argv);
 
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
-- 
2.55.0

[PATCH 3/6] conf: Make conf_tap_fd() operate more like conf_mode()

[David Gibson]

We have two cases where we need to parse specific options early:
conf_tap_fd() and conf_mode().  conf_tap_fd() has a slightly odd interface,
requiring the caller to use getopt_long() to find the right option, then
pass it in.  Alter it to work like conf_mode() instead, where all the
command line parsing logic is contained within the conf.c function.

This is slightly more lines, but has a clearer division of responsibility.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c      | 25 +++++++++++++++++++++----
 conf.h      |  2 +-
 isolation.c | 12 ++----------
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/conf.c b/conf.c
index 41c9d557..f0bce41b 100644
--- a/conf.c
+++ b/conf.c
@@ -1153,17 +1153,34 @@ static void conf_sock_listen(const struct ctx *c)
 
 /**
  * conf_tap_fd() - Read tap fd as supplied by -F command line option
- * @arg:	Argument to -F command line option
+ * @argc:	Argument count
+ * @argv:	Command line options
+ *
+ * Return: fd number from --fd option, or -1 if not supplied
  */
-int conf_tap_fd(const char *arg)
+int conf_tap_fd(int argc, char **argv)
 {
-	const char *p = arg;
+	const struct option optfd[] = {	{ "fd", required_argument, NULL, 'F' },
+					{ 0 }, };
+	const char *fdarg = NULL, *p;
 	unsigned long val;
+	int name;
+
+	optind = 0;
+	do {
+		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
+		if (name == 'F')
+			fdarg = optarg;
+	} while (name != -1);
+
+	if (!fdarg)
+		return -1;
 
+	p = fdarg;
 	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p)	||
 	    val > INT_MAX					||
 	    (val != STDIN_FILENO && val <= STDERR_FILENO))
-		die("Invalid --fd: %s", arg);
+		die("Invalid --fd: %s", fdarg);
 
 	return val;
 }
diff --git a/conf.h b/conf.h
index 1fa1280e..19bf9bc5 100644
--- a/conf.h
+++ b/conf.h
@@ -7,7 +7,7 @@
 #define CONF_H
 
 enum passt_modes conf_mode(int argc, char *argv[]);
-int conf_tap_fd(const char *arg);
+int conf_tap_fd(int argc, char **argv);
 void conf(struct ctx *c, int argc, char **argv);
 void conf_listen_handler(struct ctx *c, uint32_t events);
 void conf_handler(struct ctx *c, uint32_t events);
diff --git a/isolation.c b/isolation.c
index 4bb79e34..c4d476d6 100644
--- a/isolation.c
+++ b/isolation.c
@@ -58,7 +58,6 @@
 
 #include <errno.h>
 #include <fcntl.h>
-#include <getopt.h>
 #include <grp.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -208,9 +207,7 @@ static int move_root(void)
  */
 int isolate_initial(int argc, char **argv)
 {
-	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
-					{ 0 }, };
-	int name, rc, fd_tap = -1;
+	int rc, fd_tap = -1;
 	uint64_t keep;
 
 	/* We want to keep CAP_NET_BIND_SERVICE in the initial
@@ -251,12 +248,7 @@ int isolate_initial(int argc, char **argv)
 
 	drop_caps_ep_except(keep);
 
-	do {
-		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
-
-		if (name == 'F')
-			fd_tap = conf_tap_fd(optarg);
-	} while (name != -1);
+	fd_tap = conf_tap_fd(argc, argv);
 
 	if (fd_tap == -1) {
 		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-- 
2.55.0

[PATCH 4/6] isolation: Move --fd descriptor to a number of our choosing

[David Gibson]

Some users of passt pass an fd for the tap interface in, with the --fd
parameter, rather than having passt open it itself.  This requires some
slightly fiddly logic in isolate_initial() so we don't close() it along
with any other file descriptors leaked into us by hte parent.

More importantly, however, this is broken if the passed fd is 0, 1 or 2,
since in that case we will assume it's a standard stream and close it in
__daemon().  We explicitly disallow 1 or 2 in conf_tap_fd(), but 0 has been
permitted since aa1cc8922 ("conf: allow --fd 0").  It looks like the use
case of the contributor of that patch didn't involve daemonizing passt.

To fix this, use dup2() to move to the passed fd to 3.  This removes the
possibility of mixing it up with a standard stream, and as a bonus makes
the close_range() logic much simpler.  With isolate_initial() made safe
for --fd 1 and --fd 2, we can remove the logic excluding those from
conf_fd_tap().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c      |  4 +---
 isolation.c | 21 +++++++++------------
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/conf.c b/conf.c
index f0bce41b..67f0db6e 100644
--- a/conf.c
+++ b/conf.c
@@ -1177,9 +1177,7 @@ int conf_tap_fd(int argc, char **argv)
 		return -1;
 
 	p = fdarg;
-	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p)	||
-	    val > INT_MAX					||
-	    (val != STDIN_FILENO && val <= STDERR_FILENO))
+	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p) || val > INT_MAX)
 		die("Invalid --fd: %s", fdarg);
 
 	return val;
diff --git a/isolation.c b/isolation.c
index c4d476d6..1a7b8527 100644
--- a/isolation.c
+++ b/isolation.c
@@ -207,7 +207,7 @@ static int move_root(void)
  */
 int isolate_initial(int argc, char **argv)
 {
-	int rc, fd_tap = -1;
+	int fd_tap = -1, close_from = STDERR_FILENO + 1;
 	uint64_t keep;
 
 	/* We want to keep CAP_NET_BIND_SERVICE in the initial
@@ -249,19 +249,16 @@ int isolate_initial(int argc, char **argv)
 	drop_caps_ep_except(keep);
 
 	fd_tap = conf_tap_fd(argc, argv);
-
-	if (fd_tap == -1) {
-		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	} else if (fd_tap == STDERR_FILENO + 1) { /* Still a single range */
-		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
-	} else {
-		rc = close_range(STDERR_FILENO + 1, fd_tap - 1,
-				 CLOSE_RANGE_UNSHARE);
-		if (!rc)
-			rc = close_range(fd_tap + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	if (fd_tap >= 0) {
+		/* Move the passed fd to a more convenient location */
+		if (fd_tap != close_from			&&
+		    (dup2(fd_tap, close_from) != close_from	||
+		     close(fd_tap)))
+			die_perror("Could not move --fd descriptor");
+		fd_tap = close_from++;
 	}
 
-	if (rc) {
+	if (close_range(close_from, ~0U, CLOSE_RANGE_UNSHARE)) {
 		if (errno == ENOSYS || errno == EINVAL) {
 			/* This probably means close_range() or the
 			 * CLOSE_RANGE_UNSHARE flag is not supported by the
-- 
2.55.0

[PATCH 5/6] main: Ensure fds 0-2 are populated

[David Gibson]

Usually fds 0-2 are stdin, stdout and stderr.  However, certain things
which might invoke passt can close some of those standard fds.  If so,
anything we open might be placed in one of the standard slots.

For the handful of things we open early enough, this can be a problem
because we close fds 0-2 in __daemon(), replacing them with dupes of
/dev/null.

We could avoid closing those fds in __daemon() if they're not standard
streams.  However, leaving things other than the standard streams in fds
0-2 is a footgun: a stray printf() that occurs in a circumstance it
shouldn't could send harmful garbage to a device or socket.  It's also
likely to be confusing if debugging with strace or similar.

To avoid this, fill any missing standard streams with a dupe of /dev/null,
right after isolate_initial().  Since open()ing /dev/null itself could
land in one of those fd 0-2 slots, we need to be careful when we close it
not to leave a new gap.

Link: https://bugs.passt.top/show_bug.cgi?id=215
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt.c | 19 +++++++++++++------
 util.c  |  3 +--
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/passt.c b/passt.c
index b99e5998..663a1005 100644
--- a/passt.c
+++ b/passt.c
@@ -330,8 +330,8 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
 int main(int argc, char **argv)
 {
 	struct epoll_event events[NUM_EPOLL_EVENTS];
+	int nfds, devnull_fd = -1, fd;
 	struct ctx *c = &passt_ctx;
-	int nfds, devnull_fd = -1;
 	struct rlimit limit;
 	struct timespec now;
 	struct sigaction sa;
@@ -343,6 +343,15 @@ int main(int argc, char **argv)
 
 	c->fd_tap = isolate_initial(argc, argv);
 
+	if ((devnull_fd = open("/dev/null", O_RDWR | O_CLOEXEC)) < 0)
+		die_perror("Failed to open /dev/null");
+	/* Ensure fds 0-2 are populated */
+	for (fd = 0; fd <= STDERR_FILENO; fd++) {
+		if (fcntl(fd, F_GETFD) < 0 &&
+		    dup2(devnull_fd, fd) < 0)
+			die_perror("Failed to populate fd %d", fd);
+	}
+
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
 	sa.sa_handler = exit_handler;
@@ -410,11 +419,6 @@ int main(int argc, char **argv)
 	fwd_neigh_table_init(c);
 	nl_neigh_notify_init(c);
 
-	if (!c->foreground) {
-		if ((devnull_fd = open("/dev/null", O_RDWR | O_CLOEXEC)) < 0)
-			die_perror("Failed to open /dev/null");
-	}
-
 	if (isolate_prefork(c))
 		die("Failed to sandbox process, exiting");
 
@@ -428,6 +432,9 @@ int main(int argc, char **argv)
 		c->pidfile_fd = -1;
 	}
 
+	if (devnull_fd > STDERR_FILENO)
+		close(devnull_fd);
+
 	if (pasta_child_pid) {
 		kill(pasta_child_pid, SIGUSR1);
 		log_stderr = false;
diff --git a/util.c b/util.c
index ce5021a9..bd4c6caa 100644
--- a/util.c
+++ b/util.c
@@ -522,8 +522,7 @@ int __daemon(int pidfile_fd, int devnull_fd)
 	if (setsid()				< 0 ||
 	    dup2(devnull_fd, STDIN_FILENO)	< 0 ||
 	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
-	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
-	    close(devnull_fd))
+	    dup2(devnull_fd, STDERR_FILENO)	< 0)
 		passt_exit(EXIT_FAILURE);
 
 	return 0;
-- 
2.55.0

[PATCH 6/6] passt: *Always* close pidfile_fd, not just when daemonizing

[David Gibson]

In order to satisfy static checkers that we don't have an fd leak,
a9c61ffaf153 added a close() of c->pidfile_fd, amongst others.  However,
it only close()s it in the case where we daemonize into the background.
While less universally useful in the foreground / non-daemon case, it's
perfectly reasonable to still have a pidfile.  We'll still write it, and
we should still close it.

Fixes: a9c61ffaf153 ("util, passt: Close daemon-lifetime fds on exit to avoid Coverity warning")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/passt.c b/passt.c
index 663a1005..4b7c8434 100644
--- a/passt.c
+++ b/passt.c
@@ -424,14 +424,14 @@ int main(int argc, char **argv)
 
 	if (!c->foreground) {
 		__daemon(c->pidfile_fd, devnull_fd);
-		close(c->pidfile_fd);
-		c->pidfile_fd = -1;
 		log_stderr = false;
 	} else {
 		pidfile_write(c->pidfile_fd, getpid());
-		c->pidfile_fd = -1;
 	}
 
+	close(c->pidfile_fd);
+	c->pidfile_fd = -1;
+
 	if (devnull_fd > STDERR_FILENO)
 		close(devnull_fd);
 
-- 
2.55.0

Re: [PATCH 1/6] isolation, util: Fold close_open_files() into isolate_initial()

[Stefano Brivio]

On Tue, 14 Jul 2026 19:29:21 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Most functions in util.c are, well, utilities, that are useful in a bunch
> of places.  close_open_files(), however, is very specific, it's only called
> from isolate_initial(), and really only can be called from there - it's

Nit: its

> logic encodes a very specific action we want to take at that point.
> 
> So, remove it from util.c and fold into isolate_initial().

To me this doesn't actually look like an improvement (well, of course,
otherwise I would have structured commit 09603cab28f9 ("passt, util:
Close any open file that the parent might have leaked") differently)
exactly because close_open_files() is very specific and does exactly
that one thing, contributing to a shorter version of isolate_initial().

Now, I realise that we're pretty far from "fixing"
https://bugs.passt.top/show_bug.cgi?id=31, but still,
isolate_initial() in its current form looks much more digestible than
a version with thirty-three lines added on top. It currently has a
specific role and that's conceptually a separated operation,
regardless of the fact it has a single caller.

On top of that, the function comment to close_open_files() disappears
this way, because it has no natural place anymore. But I think that
was rather useful.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

By the way, should you respin, you could probably Cc: Ammar as he
volunteered to review this kind of series. I would also suggest Cc'ing
reporters (and marking them as such when doable).

-- 
Stefano

Re: [PATCH 2/6] isolation, conf: Set c->fd_tap from eary parse of --fd

[Stefano Brivio]

On Tue, 14 Jul 2026 19:29:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> We parse --fd twice: once in isolate_initial() just to avoid clobbering
> the passed in fd.  Then we parse it "for real" in conf(), to set c->fd_tap
> and other configuration variables.
> 
> Change this, so that we return the value parsed early from
> isolate_initial() and set c->fd_tap from that.  This doesn't accomplish
> much immediately, but will make some further cleanups possible.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c      | 2 +-
>  isolation.c | 6 +++++-
>  isolation.h | 2 +-
>  passt.c     | 2 +-
>  4 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 6d83daef..41c9d557 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1593,7 +1593,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  			c->fd_control_listen = c->fd_control = -1;
>  			break;
>  		case 'F':
> -			c->fd_tap = conf_tap_fd(optarg);
> +			/* --fd was parsed early and c->fd_tap set in main() */
>  			c->one_off = true;
>  			*c->sock_path = 0;
>  			break;
> diff --git a/isolation.c b/isolation.c
> index 07b281bb..4bb79e34 100644
> --- a/isolation.c
> +++ b/isolation.c
> @@ -203,8 +203,10 @@ static int move_root(void)
>   *  - close all open files except for standard streams and the one from --fd
>   * Mustn't:
>   *  - remove filesystem access (we need to access files during setup)
> + *
> + * Return: fd number from --fd, or -1 if not specified

I think this is a rather weird value to return for a function that's
called isolate_initial(). Are patches 1/2 and 2/2 really needed for the
rest of the series? I don't quite see the connection.

I'm still reviewing the rest of the series.

-- 
Stefano