The --pcap or -p option can be used with or without an argument.  If given,
the argument gives the name of the file to save a packet trace to.  If
omitted, we generate a default name in /tmp.

Generating the default name isn't particularly useful though, since making
a suitable name can easily be done by the caller.  Remove this feature.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c  | 18 +++---------------
 passt.1 | 10 ----------
 pcap.c  | 28 ----------------------------
 3 files changed, 3 insertions(+), 53 deletions(-)

diff --git a/conf.c b/conf.c
index d936157..4eb9e3d 100644
--- a/conf.c
+++ b/conf.c
@@ -737,14 +737,7 @@ static void usage(const char *name)
 		     UNIX_SOCK_PATH, 1);
 	}
 
-	info(   "  -p, --pcap [FILE]	Log tap-facing traffic to pcap file");
-	info(   "    if FILE is not given, log to:");
-
-	if (strstr(name, "pasta"))
-		info("      /tmp/pasta_ISO8601-TIMESTAMP_PID.pcap");
-	else
-		info("      /tmp/passt_ISO8601-TIMESTAMP_PID.pcap");
-
+	info(   "  -p, --pcap FILE	Log tap-facing traffic to pcap file");
 	info(   "  -P, --pid FILE	Write own PID to the given file");
 	info(   "  -m, --mtu MTU	Assign MTU via DHCP/NDP");
 	info(   "    a zero value disables assignment");
@@ -1021,7 +1014,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{"help",	no_argument,		NULL,		'h' },
 		{"socket",	required_argument,	NULL,		's' },
 		{"ns-ifname",	required_argument,	NULL,		'I' },
-		{"pcap",	optional_argument,	NULL,		'p' },
+		{"pcap",	required_argument,	NULL,		'p' },
 		{"pid",		required_argument,	NULL,		'P' },
 		{"mtu",		required_argument,	NULL,		'm' },
 		{"address",	required_argument,	NULL,		'a' },
@@ -1084,7 +1077,7 @@ void conf(struct ctx *c, int argc, char **argv)
 
 		name = getopt_long(argc, argv, optstring, options, NULL);
 
-		if ((name == 'p' || name == 'D' || name == 'S')
&& !optarg &&
+		if ((name == 'D' || name == 'S') && !optarg &&
 		    optind < argc && *argv[optind] && *argv[optind] !=
'-') {
 			if (c->mode == MODE_PASTA) {
 				if (conf_ns_opt(c, nsdir, userns, argv[optind]))
@@ -1289,11 +1282,6 @@ void conf(struct ctx *c, int argc, char **argv)
 				usage(argv[0]);
 			}
 
-			if (!optarg) {
-				*c->pcap = 1;
-				break;
-			}
-
 			ret = snprintf(c->pcap, sizeof(c->pcap), "%s", optarg);
 			if (ret <= 0 || ret >= (int)sizeof(c->pcap)) {
 				err("Invalid pcap path: %s", optarg);
diff --git a/passt.1 b/passt.1
index 378778c..9bed946 100644
--- a/passt.1
+++ b/passt.1
@@ -111,16 +111,6 @@ Display a help message and exit.
 Capture tap-facing (that is, guest-side or namespace-side) network packets to
 \fIfile\fR in \fBpcap\fR format.
 
-If \fIfile\fR is not given, capture packets to
-
-	\fB/tmp/passt_\fIISO8601-timestamp\fR_\fIPID\fB.pcap\fR
-
-in \fBpasst\fR mode and to
-
-	\fB/tmp/pasta_\fIISO8601-timestamp\fR_\fIPID\fB.pcap\fR
-
-in \fBpasta\fR mode, where \fIPID\fR is the ID of the running process.
-
 .TP
 .BR \-P ", " \-\-pid " " \fIfile
 Write own PID to \fIfile\fR once initialisation is done, before forking to
diff --git a/pcap.c b/pcap.c
index 64beb34..7a48baf 100644
--- a/pcap.c
+++ b/pcap.c
@@ -31,11 +31,6 @@
 #include "util.h"
 #include "passt.h"
 
-#define PCAP_PREFIX		"/tmp/passt_"
-#define PCAP_PREFIX_PASTA	"/tmp/pasta_"
-#define PCAP_ISO8601_FORMAT	"%FT%H:%M:%SZ"
-#define PCAP_ISO8601_STR	"YYYY-MM-ddTHH:mm:ssZ"
-
 #define PCAP_VERSION_MINOR 4
 
 static int pcap_fd = -1;
@@ -171,7 +166,6 @@ fail:
 void pcap_init(struct ctx *c)
 {
 	int flags = O_WRONLY | O_CREAT | O_TRUNC;
-	struct timeval tv;
 
 	if (pcap_fd != -1)
 		return;
@@ -179,28 +173,6 @@ void pcap_init(struct ctx *c)
 	if (!*c->pcap)
 		return;
 
-	if (*c->pcap == 1) {
-		char name[] = PCAP_PREFIX PCAP_ISO8601_STR STR(UINT_MAX)
-			      ".pcap";
-		struct tm *tm;
-
-		if (c->mode == MODE_PASTA)
-			memcpy(name, PCAP_PREFIX_PASTA,
-			       sizeof(PCAP_PREFIX_PASTA));
-
-		gettimeofday(&tv, NULL);
-		tm = localtime(&tv.tv_sec);
-		strftime(name + strlen(PCAP_PREFIX),
-			 sizeof(PCAP_ISO8601_STR) - 1, PCAP_ISO8601_FORMAT, tm);
-
-		snprintf(name + strlen(PCAP_PREFIX) + strlen(PCAP_ISO8601_STR),
-			 sizeof(name) - strlen(PCAP_PREFIX) -
-					strlen(PCAP_ISO8601_STR),
-			 "_%i.pcap", getpid());
-
-		strncpy(c->pcap, name, PATH_MAX);
-	}
-
 	flags |= c->foreground ? O_CLOEXEC : 0;
 	pcap_fd = open(c->pcap, flags, S_IRUSR | S_IWUSR);
 	if (pcap_fd == -1) {
-- 
2.37.2

pasta can identify a netns as a "name", which is to say a path relative to
(usually) /run/netns, which is the place that ip(8) creates persistent
network namespaces.  Alternatively a full path to a netns can be given.

The --nsrun-dir option allows the user to change the standard path where
netns names are resolved.  However, there's no real point to this, if the
user wants to override the location of the netns, they can just as easily
use the full path to specify the netns.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c  | 24 ++++--------------------
 passt.1 |  6 ------
 2 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/conf.c b/conf.c
index 6770be9..e76181c 100644
--- a/conf.c
+++ b/conf.c
@@ -510,14 +510,13 @@ static int conf_ns_check(void *arg)
 /**
  * conf_ns_opt() - Open network, user namespaces descriptors from configuration
  * @c:			Execution context
- * @nsdir:		--nsrun-dir argument, can be an empty string
  * @conf_userns:	--userns argument, can be an empty string
  * @optarg:		PID, path or name of namespace
  *
  * Return: 0 on success, negative error code otherwise
  */
 static int conf_ns_opt(struct ctx *c,
-		       char *nsdir, const char *conf_userns, const char *optarg)
+		       const char *conf_userns, const char *optarg)
 {
 	int ufd = -1, nfd = -1, try, ret, netns_only_reset = c->netns_only;
 	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX];
@@ -557,7 +556,7 @@ static int conf_ns_opt(struct ctx *c,
 				continue;
 		} else if (try == 2) {
 			ret = snprintf(netns, PATH_MAX, "%s/%s",
-				 *nsdir ? nsdir : NETNS_RUN_DIR, optarg);
+				       NETNS_RUN_DIR, optarg);
 			if (ret <= 0 || ret > (int)sizeof(netns))
 				continue;
 		}
@@ -859,8 +858,6 @@ pasta_opts:
 	info(   "  --userns NSPATH 	Target user namespace to join");
 	info(   "  --netns-only		Don't join existing user namespace");
 	info(   "    implied if PATH or NAME are given without --userns");
-	info(   "  --nsrun-dir		Directory for nsfs mountpoints");
-	info(   "    default: " NETNS_RUN_DIR);
 	info(   "  --config-net		Configure tap interface in namespace");
 	info(   "  --ns-mac-addr ADDR	Set MAC address on tap interface");
 
@@ -1040,7 +1037,6 @@ void conf(struct ctx *c, int argc, char **argv)
 		{"udp-ns",	required_argument,	NULL,		'U' },
 		{"userns",	required_argument,	NULL,		2 },
 		{"netns-only",	no_argument,		&c->netns_only,	1 },
-		{"nsrun-dir",	required_argument,	NULL,		3 },
 		{"config-net",	no_argument,		&c->pasta_conf_ns, 1 },
 		{"ns-mac-addr",	required_argument,	NULL,		4 },
 		{"dhcp-dns",	no_argument,		NULL,		5 },
@@ -1054,7 +1050,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{ 0 },
 	};
 	struct get_bound_ports_ns_arg ns_ports_arg = { .c = c };
-	char nsdir[PATH_MAX] = { 0 }, userns[PATH_MAX] = { 0 };
+	char userns[PATH_MAX] = { 0 };
 	enum conf_port_type tcp_tap = 0, tcp_init = 0;
 	enum conf_port_type udp_tap = 0, udp_init = 0;
 	bool v4_only = false, v6_only = false;
@@ -1093,18 +1089,6 @@ void conf(struct ctx *c, int argc, char **argv)
 				usage(argv[0]);
 			}
 			break;
-		case 3:
-			if (c->mode != MODE_PASTA) {
-				err("--nsrun-dir is for pasta mode only");
-				usage(argv[0]);
-			}
-
-			ret = snprintf(nsdir, sizeof(nsdir), "%s", optarg);
-			if (ret <= 0 || ret >= (int)sizeof(nsdir)) {
-				err("Invalid nsrun-dir: %s", optarg);
-				usage(argv[0]);
-			}
-			break;
 		case 4:
 			if (c->mode != MODE_PASTA) {
 				err("--ns-mac-addr is for pasta mode only");
@@ -1461,7 +1445,7 @@ void conf(struct ctx *c, int argc, char **argv)
 	check_root(c);
 
 	if (c->mode == MODE_PASTA && optind + 1 == argc) {
-		ret = conf_ns_opt(c, nsdir, userns, argv[optind]);
+		ret = conf_ns_opt(c, userns, argv[optind]);
 		if (ret == -ENOENT)
 			err("Namespace %s not found", argv[optind]);
 		if (ret < 0)
diff --git a/passt.1 b/passt.1
index 78b10b8..bbdadc1 100644
--- a/passt.1
+++ b/passt.1
@@ -458,12 +458,6 @@ without \-\-userns.
 If the target network namespace is bound to the filesystem (that is, if PATH or
 NAME are given as target), do not exit once the network namespace is deleted.
 
-.TP
-.BR \-\-nsrun-dir " " \fIpath
-Directory for nsfs mountpoints, used as path prefix for names of namespaces.
-
-The default path is shown with --help.
-
 .TP
 .BR \-\-config-net
 Configure networking in the namespace: set up addresses and routes as configured
-- 
2.37.2

pasta takes as its only non-option argument either a PID to attach to the
namespaces of, a PATH to a network namespace or a NAME of a network
namespace (relative to /run/netns).  Currently to determine which it is
we try all 3 in that order, and if anything goes wrong we move onto the
next.

This has the potential to cause very confusing failure modes.  e.g. if the
argument is intended to be a network namespace name, but a (non-namespace)
file of the same name exists in the current directory.

Make behaviour more predictable by choosing how to treat the argument based
only on the argument's contents, not anything else on the system:
  - If it's a decimal integer treat it as a PID
  - Otherwise, if it has no '/' characters, treat it as a netns name
    (ip-netns doesn't allow '/' in netns names)
  - Otherwise, treat it as a netns path

If you want to open a persistent netns in the current directory, you can
use './netns'.

This also allows us to split the parsing of the PID|PATH|NAME option from
the actual opening of the namespaces.  In turn that allows us to put the
opening of existing namespaces next to the opening of new namespaces in
pasta_start_ns.  That makes the logical flow easier to follow and will
enable later cleanups.

Caveats:
 - The separation of functions mean we will always generate the basename
   and dirname for the netns_quit system, even when using PID namespaces.
   This is pointless, since the netns_quit system doesn't work for non
   persistent namespaces, but is harmless.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 172 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 90 insertions(+), 82 deletions(-)

diff --git a/conf.c b/conf.c
index 47f3e41..10b7b9f 100644
--- a/conf.c
+++ b/conf.c
@@ -489,6 +489,51 @@ out:
 		warn("Couldn't get any nameserver address");
 }
 
+/**
+ * conf_ns_opt() - Parse non-option argument to namespace paths
+ * @userns:	buffer of size PATH_MAX, initially contains --userns
+ *		argument (may be empty), updated with userns path
+ * @netns:	buffer of size PATH_MAX, updated with netns path
+ * @arg:	PID, path or name of network namespace
+ *
+ * Return: 0 on success, negative error code otherwise
+ */
+static int conf_ns_opt(char *userns, char *netns, const char *arg)
+{
+	char *endptr;
+	long pidval;
+	int ret;
+
+	pidval = strtol(arg, &endptr, 10);
+	if (!*endptr) {
+		/* Looks like a pid */
+		if (pidval < 0 || pidval > INT_MAX) {
+			err("Invalid PID %s", arg);
+			return -EINVAL;
+		}
+
+		snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
+		if (!*userns)
+			snprintf(userns, PATH_MAX, "/proc/%ld/ns/user", pidval);
+		return 0;
+	}
+
+	if (!strchr(arg, '/')) {
+		/* looks like a netns name */
+		ret = snprintf(netns, PATH_MAX, "%s/%s", NETNS_RUN_DIR, arg);
+	} else {
+		/* otherwise assume it's a netns path */
+		ret = snprintf(netns, PATH_MAX, "%s", arg);
+	}
+
+	if (ret <= 0 || ret > PATH_MAX) {
+		err("Network namespace name/path %s too long");
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
 /**
  * conf_ns_check() - Check if we can enter configured namespaces
  * @arg:	Execution context
@@ -508,102 +553,58 @@ static int conf_ns_check(void *arg)
 }
 
 /**
- * conf_ns_opt() - Open network, user namespaces descriptors from configuration
- * @c:			Execution context
- * @conf_userns:	--userns argument, can be an empty string
- * @optarg:		PID, path or name of namespace
+ * conf_ns_open() - Open network, user namespaces descriptors from configuration
+ * @c:		Execution context
+ * @userns:	--userns argument, can be an empty string
+ * @netns:	network namespace path
  *
  * Return: 0 on success, negative error code otherwise
  */
-static int conf_ns_opt(struct ctx *c,
-		       const char *conf_userns, const char *optarg)
+static int conf_ns_open(struct ctx *c, const char *userns, const char *netns)
 {
-	int ufd = -1, nfd = -1, try, ret, netns_only_reset = c->netns_only;
-	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX];
-	char *endptr;
-	long pid_arg;
-	pid_t pid;
+	int ufd = -1, nfd = -1;
 
-	if (c->netns_only && *conf_userns) {
+	if (c->netns_only && *userns) {
 		err("Both --userns and --netns-only given");
 		return -EINVAL;
 	}
 
-	/* It might be a PID, a netns path, or a netns name */
-	for (try = 0; try < 3; try++) {
-		if (try == 0) {
-			pid_arg = strtol(optarg, &endptr, 10);
-			if (*endptr || pid_arg < 0 || pid_arg > INT_MAX)
-				continue;
-
-			pid = pid_arg;
-
-			if (!*conf_userns && !c->netns_only) {
-				ret = snprintf(userns, PATH_MAX,
-					       "/proc/%i/ns/user", pid);
-				if (ret <= 0 || ret > (int)sizeof(userns))
-					continue;
-			}
-			ret = snprintf(netns, PATH_MAX, "/proc/%i/ns/net", pid);
-			if (ret <= 0 || ret > (int)sizeof(netns))
-				continue;
-		} else if (try == 1) {
-			if (!*conf_userns)
-				c->netns_only = 1;
-
-			ret = snprintf(netns, PATH_MAX, "%s", optarg);
-			if (ret <= 0 || ret > (int)sizeof(userns))
-				continue;
-		} else if (try == 2) {
-			ret = snprintf(netns, PATH_MAX, "%s/%s",
-				       NETNS_RUN_DIR, optarg);
-			if (ret <= 0 || ret > (int)sizeof(netns))
-				continue;
-		}
-
-		if (!c->netns_only) {
-			if (*conf_userns)
-				ufd = open(conf_userns, O_RDONLY | O_CLOEXEC);
-			else if (*userns)
-				ufd = open(userns, O_RDONLY | O_CLOEXEC);
-		}
-
-		nfd = open(netns, O_RDONLY | O_CLOEXEC);
-
-		if (nfd == -1 || (ufd == -1 && !c->netns_only)) {
-			if (nfd >= 0)
-				close(nfd);
-
-			if (ufd >= 0)
-				close(ufd);
+	nfd = open(netns, O_RDONLY | O_CLOEXEC);
+	if (nfd < 0) {
+		err("Couldn't open network namespace %s", netns);
+		return -ENOENT;
+	}
 
-			continue;
+	if (!c->netns_only && *userns) {
+		ufd = open(userns, O_RDONLY | O_CLOEXEC);
+		if (ufd < 0) {
+			close(nfd);
+			err("Couldn't open user namespace %s", userns);
+			return -ENOENT;
 		}
+	}
 
-		c->pasta_netns_fd = nfd;
-		c->pasta_userns_fd = ufd;
-
-		NS_CALL(conf_ns_check, c);
+	c->pasta_netns_fd = nfd;
+	c->pasta_userns_fd = ufd;
+	c->netns_only = !*userns;
 
-		if (c->pasta_netns_fd >= 0) {
-			char buf[PATH_MAX];
+	NS_CALL(conf_ns_check, c);
 
-			if (try == 0 || c->no_netns_quit)
-				return 0;
+	if (c->pasta_netns_fd < 0) {
+		err("Couldn't switch to pasta namespaces");
+		return -ENOENT;
+	}
 
-			strncpy(buf, netns, PATH_MAX);
-			strncpy(c->netns_base, basename(buf), PATH_MAX - 1);
-			strncpy(buf, netns, PATH_MAX);
-			strncpy(c->netns_dir, dirname(buf), PATH_MAX - 1);
+	if (!c->no_netns_quit) {
+		char buf[PATH_MAX];
 
-			return 0;
-		}
+		strncpy(buf, netns, PATH_MAX);
+		strncpy(c->netns_base, basename(buf), PATH_MAX - 1);
+		strncpy(buf, netns, PATH_MAX);
+		strncpy(c->netns_dir, dirname(buf), PATH_MAX - 1);
 	}
 
-	c->netns_only = netns_only_reset;
-
-	err("Namespace %s not found", optarg);
-	return -ENOENT;
+	return 0;
 }
 
 /**
@@ -1051,7 +1052,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{ 0 },
 	};
 	struct get_bound_ports_ns_arg ns_ports_arg = { .c = c };
-	char userns[PATH_MAX] = { 0 };
+	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
 	enum conf_port_type tcp_tap = 0, tcp_init = 0;
 	enum conf_port_type udp_tap = 0, udp_init = 0;
 	bool v4_only = false, v6_only = false;
@@ -1446,7 +1447,7 @@ void conf(struct ctx *c, int argc, char **argv)
 	check_root(c);
 
 	if (c->mode == MODE_PASTA && optind + 1 == argc) {
-		ret = conf_ns_opt(c, userns, argv[optind]);
+		ret = conf_ns_opt(userns, netns, argv[optind]);
 		if (ret < 0)
 			usage(argv[0]);
 	} else if (c->mode == MODE_PASTA && *userns && optind == argc) {
@@ -1459,8 +1460,15 @@ void conf(struct ctx *c, int argc, char **argv)
 	if (c->pasta_conf_ns)
 		c->no_ra = 1;
 
-	if (c->mode == MODE_PASTA && c->pasta_netns_fd == -1)
-		pasta_start_ns(c);
+	if (c->mode == MODE_PASTA) {
+		if (*netns) {
+			ret = conf_ns_open(c, userns, netns);
+			if (ret < 0)
+				usage(argv[0]);
+		} else {
+			pasta_start_ns(c);
+		}
+	}
 
 	if (nl_sock_init(c)) {
 		err("Failed to get netlink socket");
-- 
2.37.2

On Fri, 26 Aug 2022 14:58:31 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  When not attaching to an existing network namespace,
pasta always
 spawns an interactive shell in a new namespace to attach to.  Most
 commands which can issue a shell in a modified environment can also
 issue other commands as well (e.g. env, strace).  We want to allow
 pasta to do the same.
 
 Because of the way the non-option argument to pasta is currently
 overloaded, allowing this requires some other changes to the way we
 parse the command line.
 
 David Gibson (8):
   conf: Make the argument to --pcap option mandatory
   conf: Use "-D none" and "-S none" instead of missing empty option
     arguments
   Correct manpage for --userns
   Remove --nsrun-dir option
   Move ENOENT error message into conf_ns_opt()
   More deterministic detection of whether argument is a PID, PATH or
     NAME
   Use explicit --netns option rather than multiplexing with PID
   Allow pasta to take a command to execute 
Applied now with those small changes, thanks a lot.

-- 
Stefano