[PATCH v4 0/9] error logging fixes

newer
[PATCH v2 0/4] Some cleanups to...

Laine Stump

15 Feb 2023 15 Feb '23

9:24 a.m.

There are two topics covered here: 1) If a logFile is set, passt's behavior has been to send all error messages there, and *not* to stderr. This makes it difficult for another program that is exec'ing passt (and setting it to log to a file) to report useful error messages when passt fails - everything after the point that the logfile is opened is sent only to the logfile. The first patch makes a simple change to the logging functions that uses the value of the system logmask to decide if it should force writing messages to stderr even when a logfile has been specified. Change from "v2": I'm using Stefano's suggestion of "abusing" logmask, rather than adding a static bool to keep track. Change from "v3": tweak a commend to Stefano's liking. 2) All the rest of the patches eliminate use of the blanket usage() function when a commandline error is encountered (and add in specific/details error messages when previously usage() was all that was logged), and replace calls to err() followed by exit() with a single call to the new function die(). Change from "v2": I changed the name of the "log and exit" function from "errexit()" to "die()" at the suggestion of Dave Gibson (Stefano concurred). Although it seems a bit more violent, it does make moot many/most of Stefano's nits about needing to split lines to eliminate

...

80 characters (I did address all the rest of the things he pointed out, though)

NB: Yes, this says it is v3, and the previous version I sent was v2, and there *was no v1* - this is because I didn't realize that git-publish is automatically incrementing the version number every time I run it, and I had done a test-drive sending the patches to my personal address prior to sending them to the list - *that* was v1. Laine Stump (9): log to stderr until process is daemonized, even if a log file is set add die() to log an error message and exit with a single call eliminate most calls to usage() in conf() make conf_ports() exit immediately after logging error make conf_pasta_ns() exit immediately after logging error make conf_ugid() exit immediately after logging error make conf_netns_opt() exit immediately after logging error log a detailed error (not usage()) when there are extra non-option arguments convert all remaining err() followed by exit() to die() conf.c | 468 ++++++++++++++++++++-------------------------------- isolation.c | 67 +++----- log.c | 24 +-- log.h | 1 + netlink.c | 3 +- passt.c | 29 ++-- pasta.c | 20 +-- tap.c | 30 ++-- 8 files changed, 244 insertions(+), 398 deletions(-) -- 2.39.1

Show replies by date

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set

Once a log file (specified on the commandline) is opened, the logging functions will stop sending error logs to stderr, which is annoying if passt has been started by another process that wants to collect error messages from stderr so it can report why passt failed to start. It would be much nicer if passt continued sending all log messages to stderr until it daemonizes itself (at which point the process that started passt can assume that it was started successfully). The system log mask is set to LOG_EMERG when the process starts, and we're already using that to do "special" logging during the period from process start until the log level requested on the commandline is processed (setting the log mask to something else). This period *almost* matches with "the time before the process is daemonized"; if we just delay setting the log mask a tiny bit, then it will match exactly, and we can use it to determine if we need to send log messages to stderr even when a log file has been specified and opened. This patch delays the setting of the log mask until immediately before the call to __daemon(). It also modifies logfn() slightly, so that it will log to stderr any time log mask is LOG_EMERG, even if a log file has been opened. Signed-off-by: Laine Stump --- log.c | 4 ++-- passt.c | 17 ++++++++++------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/log.c b/log.c index 468c730..6dc6673 100644 --- a/log.c +++ b/log.c @@ -66,8 +66,8 @@ void name(const char *format, ...) { \ va_end(args); \ } \ \ - if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \ - setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \ + if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) || \ + setlogmask(0) == LOG_MASK(LOG_EMERG)) { \ va_start(args, format); \ (void)vfprintf(stderr, format, args); \ va_end(args); \ diff --git a/passt.c b/passt.c index d957e14..c48c2d5 100644 --- a/passt.c +++ b/passt.c @@ -246,13 +246,6 @@ int main(int argc, char **argv) if (c.stderr || isatty(fileno(stdout))) __openlog(log_name, LOG_PERROR, LOG_DAEMON); - if (c.debug) - __setlogmask(LOG_UPTO(LOG_DEBUG)); - else if (c.quiet) - __setlogmask(LOG_UPTO(LOG_ERR)); - else - __setlogmask(LOG_UPTO(LOG_INFO)); - quit_fd = pasta_netns_quit_init(&c); tap_sock_init(&c); @@ -296,6 +289,16 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } + /* Once the log mask is not LOG_EMERG, we will no longer + * log to stderr if there was a log file specified. + */ + if (c.debug) + __setlogmask(LOG_UPTO(LOG_DEBUG)); + else if (c.quiet) + __setlogmask(LOG_UPTO(LOG_ERR)); + else + __setlogmask(LOG_UPTO(LOG_INFO)); + if (!c.foreground) __daemon(pidfile_fd, devnull_fd); else -- 2.39.1

Stefano Brivio

12:54 p.m.

New subject: [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set

On Wed, 15 Feb 2023 03:24:29 -0500 Laine Stump wrote:

...

Once a log file (specified on the commandline) is opened, the logging functions will stop sending error logs to stderr, which is annoying if passt has been started by another process that wants to collect error messages from stderr so it can report why passt failed to start. It would be much nicer if passt continued sending all log messages to stderr until it daemonizes itself (at which point the process that started passt can assume that it was started successfully).

The system log mask is set to LOG_EMERG when the process starts, and we're already using that to do "special" logging during the period from process start until the log level requested on the commandline is processed (setting the log mask to something else). This period *almost* matches with "the time before the process is daemonized"; if we just delay setting the log mask a tiny bit, then it will match exactly, and we can use it to determine if we need to send log messages to stderr even when a log file has been specified and opened.

This patch delays the setting of the log mask until immediately before the call to __daemon(). It also modifies logfn() slightly, so that it will log to stderr any time log mask is LOG_EMERG, even if a log file has been opened.

Signed-off-by: Laine Stump --- log.c | 4 ++-- passt.c | 17 ++++++++++------- 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/log.c b/log.c index 468c730..6dc6673 100644 --- a/log.c +++ b/log.c @@ -66,8 +66,8 @@ void name(const char *format, ...) { \ va_end(args); \ } \ \ - if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \ - setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \ + if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) || \ + setlogmask(0) == LOG_MASK(LOG_EMERG)) { \ va_start(args, format); \ (void)vfprintf(stderr, format, args); \ va_end(args); \

Strictly speaking, I think this is correct, but it causes duplicate messages to be printed on interactive terminals, or with -e/--stderr, because in that case we already set LOG_PERROR in our __openlog() wrapper. I had a quick attempt at reworking this whole mess, with a table clearly gathering conditions and logging outcomes, but it's actually not that straightforward. So I'd rather just post a follow-up patch for this, at least for the moment. -- Stefano

David Gibson

16 Feb 16 Feb

6:30 a.m.

New subject: [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set

On Wed, Feb 15, 2023 at 03:24:29AM -0500, Laine Stump wrote:

...

Once a log file (specified on the commandline) is opened, the logging functions will stop sending error logs to stderr, which is annoying if passt has been started by another process that wants to collect error messages from stderr so it can report why passt failed to start. It would be much nicer if passt continued sending all log messages to stderr until it daemonizes itself (at which point the process that started passt can assume that it was started successfully).

The system log mask is set to LOG_EMERG when the process starts, and we're already using that to do "special" logging during the period from process start until the log level requested on the commandline is processed (setting the log mask to something else). This period *almost* matches with "the time before the process is daemonized"; if we just delay setting the log mask a tiny bit, then it will match exactly, and we can use it to determine if we need to send log messages to stderr even when a log file has been specified and opened.

This patch delays the setting of the log mask until immediately before the call to __daemon(). It also modifies logfn() slightly, so that it will log to stderr any time log mask is LOG_EMERG, even if a log file has been opened.

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

...

--- log.c | 4 ++-- passt.c | 17 ++++++++++------- 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/log.c b/log.c index 468c730..6dc6673 100644 --- a/log.c +++ b/log.c @@ -66,8 +66,8 @@ void name(const char *format, ...) { \ va_end(args); \ } \ \ - if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \ - setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \ + if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) || \ + setlogmask(0) == LOG_MASK(LOG_EMERG)) { \ va_start(args, format); \ (void)vfprintf(stderr, format, args); \ va_end(args); \ diff --git a/passt.c b/passt.c index d957e14..c48c2d5 100644 --- a/passt.c +++ b/passt.c @@ -246,13 +246,6 @@ int main(int argc, char **argv) if (c.stderr || isatty(fileno(stdout))) __openlog(log_name, LOG_PERROR, LOG_DAEMON);

- if (c.debug) - __setlogmask(LOG_UPTO(LOG_DEBUG)); - else if (c.quiet) - __setlogmask(LOG_UPTO(LOG_ERR)); - else - __setlogmask(LOG_UPTO(LOG_INFO)); - quit_fd = pasta_netns_quit_init(&c);

tap_sock_init(&c); @@ -296,6 +289,16 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); }

+ /* Once the log mask is not LOG_EMERG, we will no longer + * log to stderr if there was a log file specified. + */ + if (c.debug) + __setlogmask(LOG_UPTO(LOG_DEBUG)); + else if (c.quiet) + __setlogmask(LOG_UPTO(LOG_ERR)); + else + __setlogmask(LOG_UPTO(LOG_INFO)); + if (!c.foreground) __daemon(pidfile_fd, devnull_fd); else

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Stefano Brivio

6:50 p.m.

New subject: [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set

On Thu, 16 Feb 2023 16:30:53 +1100 David Gibson wrote:

...

On Wed, Feb 15, 2023 at 03:24:29AM -0500, Laine Stump wrote:

...
Once a log file (specified on the commandline) is opened, the logging functions will stop sending error logs to stderr, which is annoying if passt has been started by another process that wants to collect error messages from stderr so it can report why passt failed to start. It would be much nicer if passt continued sending all log messages to stderr until it daemonizes itself (at which point the process that started passt can assume that it was started successfully).

The system log mask is set to LOG_EMERG when the process starts, and we're already using that to do "special" logging during the period from process start until the log level requested on the commandline is processed (setting the log mask to something else). This period *almost* matches with "the time before the process is daemonized"; if we just delay setting the log mask a tiny bit, then it will match exactly, and we can use it to determine if we need to send log messages to stderr even when a log file has been specified and opened.

This patch delays the setting of the log mask until immediately before the call to __daemon(). It also modifies logfn() slightly, so that it will log to stderr any time log mask is LOG_EMERG, even if a log file has been opened.

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

Sorry David, I missed to add *all* your Reviewed-by tags on this series. :/ Thanks a lot for reviewing all this. -- Stefano

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 2/9] add die() to log an error message and exit with a single call

Almost all occurences of err() are either immediately followed by exit(EXIT_FAILURE), usage(argv[0]) (which itself then calls exit(EXIT_FAILURE), or that is what's done immediately after returning from the function that calls err(). Modify the errfn macro so that its instantiations can include exit(EXIT_FAILURE) at the end, and use that to create a new function die() that will log an error and then exit. Signed-off-by: Laine Stump --- log.c | 14 +++++++++----- log.h | 1 + 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/log.c b/log.c index 6dc6673..2920aba 100644 --- a/log.c +++ b/log.c @@ -44,7 +44,7 @@ static char log_header[BUFSIZ]; /* File header, written back on cuts */ static time_t log_start; /* Start timestamp */ int log_trace; /* --trace mode enabled */ -#define logfn(name, level) \ +#define logfn(name, level, doexit) \ void name(const char *format, ...) { \ struct timespec tp; \ va_list args; \ @@ -74,6 +74,9 @@ void name(const char *format, ...) { \ if (format[strlen(format)] != '\n') \ fprintf(stderr, "\n"); \ } \ + \ + if (doexit) \ + exit(EXIT_FAILURE); \ } /* Prefixes for log file messages, indexed by priority */ @@ -86,10 +89,11 @@ const char *logfile_prefix[] = { " ", /* LOG_DEBUG */ }; -logfn(err, LOG_ERR) -logfn(warn, LOG_WARNING) -logfn(info, LOG_INFO) -logfn(debug, LOG_DEBUG) +logfn(die, LOG_ERR, 1) +logfn(err, LOG_ERR, 0) +logfn(warn, LOG_WARNING, 0) +logfn(info, LOG_INFO, 0) +logfn(debug,LOG_DEBUG, 0) /** * trace_init() - Set log_trace depending on trace (debug) mode diff --git a/log.h b/log.h index 987dc17..d4e9d85 100644 --- a/log.h +++ b/log.h @@ -10,6 +10,7 @@ #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */ #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE)) +void die(const char *format, ...); void err(const char *format, ...); void warn(const char *format, ...); void info(const char *format, ...); -- 2.39.1

David Gibson

16 Feb 16 Feb

6:31 a.m.

New subject: [PATCH v4 2/9] add die() to log an error message and exit with a single call

On Wed, Feb 15, 2023 at 03:24:30AM -0500, Laine Stump wrote:

...

Almost all occurences of err() are either immediately followed by exit(EXIT_FAILURE), usage(argv[0]) (which itself then calls exit(EXIT_FAILURE), or that is what's done immediately after returning from the function that calls err(). Modify the errfn macro so that its instantiations can include exit(EXIT_FAILURE) at the end, and use that to create a new function die() that will log an error and then exit.

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

...

--- log.c | 14 +++++++++----- log.h | 1 + 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/log.c b/log.c index 6dc6673..2920aba 100644 --- a/log.c +++ b/log.c @@ -44,7 +44,7 @@ static char log_header[BUFSIZ]; /* File header, written back on cuts */ static time_t log_start; /* Start timestamp */ int log_trace; /* --trace mode enabled */

-#define logfn(name, level) \ +#define logfn(name, level, doexit) \ void name(const char *format, ...) { \ struct timespec tp; \ va_list args; \ @@ -74,6 +74,9 @@ void name(const char *format, ...) { \ if (format[strlen(format)] != '\n') \ fprintf(stderr, "\n"); \ } \ + \ + if (doexit) \ + exit(EXIT_FAILURE); \ }

/* Prefixes for log file messages, indexed by priority */ @@ -86,10 +89,11 @@ const char *logfile_prefix[] = { " ", /* LOG_DEBUG */ };

-logfn(err, LOG_ERR) -logfn(warn, LOG_WARNING) -logfn(info, LOG_INFO) -logfn(debug, LOG_DEBUG) +logfn(die, LOG_ERR, 1) +logfn(err, LOG_ERR, 0) +logfn(warn, LOG_WARNING, 0) +logfn(info, LOG_INFO, 0) +logfn(debug,LOG_DEBUG, 0)

/** * trace_init() - Set log_trace depending on trace (debug) mode diff --git a/log.h b/log.h index 987dc17..d4e9d85 100644 --- a/log.h +++ b/log.h @@ -10,6 +10,7 @@ #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */ #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE))

+void die(const char *format, ...); void err(const char *format, ...); void warn(const char *format, ...); void info(const char *format, ...);

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 3/9] eliminate most calls to usage() in conf()

Nearly all of the calls to usage() in conf() occur immediately after logging a more detailed error message, and the fact that these errors are occuring indicates that the user has already seen the passt usage message (or read the manpage). Spamming the logfile with the complete contents of the usage message serves only to obscure the more detailed error message. The only time when the full usage message should be output is if the user explicitly asks for it with -h (or its synonyms) As a start to eliminating the excessive calls to usage(), this patch replaces most calls to err() followed by usage() with a call to die() instead. A few other usage() calls remain, but their removal involves bit more nuance that should be properly explained in separate commit messages. Signed-off-by: Laine Stump --- conf.c | 336 +++++++++++++++++++++------------------------------------ 1 file changed, 122 insertions(+), 214 deletions(-) diff --git a/conf.c b/conf.c index c37552d..ad8c249 100644 --- a/conf.c +++ b/conf.c @@ -1156,69 +1156,57 @@ void conf(struct ctx *c, int argc, char **argv) case 0: break; case 2: - if (c->mode != MODE_PASTA) { - err("--userns is for pasta mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASTA) + die("--userns is for pasta mode only"); ret = snprintf(userns, sizeof(userns), "%s", optarg); - if (ret <= 0 || ret >= (int)sizeof(userns)) { - err("Invalid userns: %s", optarg); - usage(argv[0]); - } + if (ret <= 0 || ret >= (int)sizeof(userns)) + die("Invalid userns: %s", optarg); + break; case 3: - if (c->mode != MODE_PASTA) { - err("--netns is for pasta mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASTA) + die("--netns is for pasta mode only"); ret = conf_netns_opt(netns, optarg); if (ret < 0) usage(argv[0]); break; case 4: - if (c->mode != MODE_PASTA) { - err("--ns-mac-addr is for pasta mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASTA) + die("--ns-mac-addr is for pasta mode only"); for (i = 0; i < ETH_ALEN; i++) { errno = 0; b = strtol(optarg + (intptr_t)i * 3, NULL, 16); - if (b < 0 || b > UCHAR_MAX || errno) { - err("Invalid MAC address: %s", optarg); - usage(argv[0]); - } + if (b < 0 || b > UCHAR_MAX || errno) + die("Invalid MAC address: %s", optarg); + c->mac_guest[i] = b; } break; case 5: - if (c->mode != MODE_PASTA) { - err("--dhcp-dns is for pasta mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASTA) + die("--dhcp-dns is for pasta mode only"); + c->no_dhcp_dns = 0; break; case 6: - if (c->mode != MODE_PASST) { - err("--no-dhcp-dns is for passt mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASST) + die("--no-dhcp-dns is for passt mode only"); + c->no_dhcp_dns = 1; break; case 7: - if (c->mode != MODE_PASTA) { - err("--dhcp-search is for pasta mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASTA) + die("--dhcp-search is for pasta mode only"); + c->no_dhcp_dns_search = 0; break; case 8: - if (c->mode != MODE_PASST) { - err("--no-dhcp-search is for passt mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASST) + die("--no-dhcp-search is for passt mode only"); + c->no_dhcp_dns_search = 1; break; case 9: @@ -1235,50 +1223,39 @@ void conf(struct ctx *c, int argc, char **argv) !IN4_IS_ADDR_LOOPBACK(&c->ip4.dns_match)) break; - err("Invalid DNS forwarding address: %s", optarg); - usage(argv[0]); + die("Invalid DNS forwarding address: %s", optarg); break; case 10: - if (c->mode != MODE_PASTA) { - err("--no-netns-quit is for pasta mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASTA) + die("--no-netns-quit is for pasta mode only"); + c->no_netns_quit = 1; break; case 11: - if (c->trace) { - err("Multiple --trace options given"); - usage(argv[0]); - } + if (c->trace) + die("Multiple --trace options given"); - if (c->quiet) { - err("Either --trace or --quiet"); - usage(argv[0]); - } + if (c->quiet) + die("Either --trace or --quiet"); c->trace = c->debug = 1; break; case 12: - if (runas) { - err("Multiple --runas options given"); - usage(argv[0]); - } + if (runas) + die("Multiple --runas options given"); runas = optarg; break; case 13: - if (logsize) { - err("Multiple --log-size options given"); - usage(argv[0]); - } + if (logsize) + die("Multiple --log-size options given"); errno = 0; logsize = strtol(optarg, NULL, 0); - if (logsize < LOGFILE_SIZE_MIN || errno) { - err("Invalid --log-size: %s", optarg); - usage(argv[0]); - } + if (logsize < LOGFILE_SIZE_MIN || errno) + die("Invalid --log-size: %s", optarg); + break; case 14: fprintf(stdout, @@ -1286,138 +1263,102 @@ void conf(struct ctx *c, int argc, char **argv) fprintf(stdout, VERSION_BLOB); exit(EXIT_SUCCESS); case 'd': - if (c->debug) { - err("Multiple --debug options given"); - usage(argv[0]); - } + if (c->debug) + die("Multiple --debug options given"); - if (c->quiet) { - err("Either --debug or --quiet"); - usage(argv[0]); - } + if (c->quiet) + die("Either --debug or --quiet"); c->debug = 1; break; case 'e': - if (logfile) { - err("Can't log to both file and stderr"); - usage(argv[0]); - } + if (logfile) + die("Can't log to both file and stderr"); - if (c->stderr) { - err("Multiple --stderr options given"); - usage(argv[0]); - } + if (c->stderr) + die("Multiple --stderr options given"); c->stderr = 1; break; case 'l': - if (c->stderr) { - err("Can't log to both stderr and file"); - usage(argv[0]); - } + if (c->stderr) + die("Can't log to both stderr and file"); - if (logfile) { - err("Multiple --log-file options given"); - usage(argv[0]); - } + if (logfile) + die("Multiple --log-file options given"); logfile = optarg; break; case 'q': - if (c->quiet) { - err("Multiple --quiet options given"); - usage(argv[0]); - } + if (c->quiet) + die("Multiple --quiet options given"); - if (c->debug) { - err("Either --debug or --quiet"); - usage(argv[0]); - } + if (c->debug) + die("Either --debug or --quiet"); c->quiet = 1; break; case 'f': - if (c->foreground) { - err("Multiple --foreground options given"); - usage(argv[0]); - } + if (c->foreground) + die("Multiple --foreground options given"); c->foreground = 1; break; case 's': - if (*c->sock_path) { - err("Multiple --socket options given"); - usage(argv[0]); - } + if (*c->sock_path) + die("Multiple --socket options given"); ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s", optarg); - if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) { - err("Invalid socket path: %s", optarg); - usage(argv[0]); - } + if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) + die("Invalid socket path: %s", optarg); + break; case 'F': - if (c->fd_tap >= 0) { - err("Multiple --fd options given"); - usage(argv[0]); - } + if (c->fd_tap >= 0) + die("Multiple --fd options given"); errno = 0; c->fd_tap = strtol(optarg, NULL, 0); - if (c->fd_tap < 0 || errno) { - err("Invalid --fd: %s", optarg); - usage(argv[0]); - } + if (c->fd_tap < 0 || errno) + die("Invalid --fd: %s", optarg); c->one_off = true; break; case 'I': - if (*c->pasta_ifn) { - err("Multiple --ns-ifname options given"); - usage(argv[0]); - } + if (*c->pasta_ifn) + die("Multiple --ns-ifname options given"); ret = snprintf(c->pasta_ifn, IFNAMSIZ - 1, "%s", optarg); - if (ret <= 0 || ret >= IFNAMSIZ - 1) { - err("Invalid interface name: %s", optarg); - usage(argv[0]); - } + if (ret <= 0 || ret >= IFNAMSIZ - 1) + die("Invalid interface name: %s", optarg); + break; case 'p': - if (*c->pcap) { - err("Multiple --pcap options given"); - usage(argv[0]); - } + if (*c->pcap) + die("Multiple --pcap options given"); ret = snprintf(c->pcap, sizeof(c->pcap), "%s", optarg); - if (ret <= 0 || ret >= (int)sizeof(c->pcap)) { - err("Invalid pcap path: %s", optarg); - usage(argv[0]); - } + if (ret <= 0 || ret >= (int)sizeof(c->pcap)) + die("Invalid pcap path: %s", optarg); + break; case 'P': - if (*c->pid_file) { - err("Multiple --pid options given"); - usage(argv[0]); - } + if (*c->pid_file) + die("Multiple --pid options given"); ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s", optarg); - if (ret <= 0 || ret >= (int)sizeof(c->pid_file)) { - err("Invalid PID file: %s", optarg); - usage(argv[0]); - } + if (ret <= 0 || ret >= (int)sizeof(c->pid_file)) + die("Invalid PID file: %s", optarg); + break; case 'm': - if (c->mtu) { - err("Multiple --mtu options given"); - usage(argv[0]); - } + if (c->mtu) + die("Multiple --mtu options given"); errno = 0; c->mtu = strtol(optarg, NULL, 0); @@ -1428,10 +1369,9 @@ void conf(struct ctx *c, int argc, char **argv) } if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || - errno) { - err("Invalid MTU: %s", optarg); - usage(argv[0]); - } + errno) + die("Invalid MTU: %s", optarg); + break; case 'a': if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr) && @@ -1451,24 +1391,21 @@ void conf(struct ctx *c, int argc, char **argv) !IN4_IS_ADDR_MULTICAST(&c->ip4.addr)) break; - err("Invalid address: %s", optarg); - usage(argv[0]); + die("Invalid address: %s", optarg); break; case 'n': c->ip4.prefix_len = conf_ip4_prefix(optarg); - if (c->ip4.prefix_len < 0) { - err("Invalid netmask: %s", optarg); - usage(argv[0]); - } + if (c->ip4.prefix_len < 0) + die("Invalid netmask: %s", optarg); + break; case 'M': for (i = 0; i < ETH_ALEN; i++) { errno = 0; b = strtol(optarg + (intptr_t)i * 3, NULL, 16); - if (b < 0 || b > UCHAR_MAX || errno) { - err("Invalid MAC address: %s", optarg); - usage(argv[0]); - } + if (b < 0 || b > UCHAR_MAX || errno) + die("Invalid MAC address: %s", optarg); + c->mac[i] = b; } break; @@ -1486,41 +1423,30 @@ void conf(struct ctx *c, int argc, char **argv) !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw)) break; - err("Invalid gateway address: %s", optarg); - usage(argv[0]); + die("Invalid gateway address: %s", optarg); break; case 'i': - if (ifi) { - err("Redundant interface: %s", optarg); - usage(argv[0]); - } + if (ifi) + die("Redundant interface: %s", optarg); - if (!(ifi = if_nametoindex(optarg))) { - err("Invalid interface name %s: %s", optarg, + if (!(ifi = if_nametoindex(optarg))) + die("Invalid interface name %s: %s", optarg, strerror(errno)); - usage(argv[0]); - } break; case 'D': if (!strcmp(optarg, "none")) { - if (c->no_dns) { - err("Redundant DNS options"); - usage(argv[0]); - } + if (c->no_dns) + die("Redundant DNS options"); - if (dns4 - c->ip4.dns || dns6 - c->ip6.dns) { - err("Conflicting DNS options"); - usage(argv[0]); - } + if (dns4 - c->ip4.dns || dns6 - c->ip6.dns) + die("Conflicting DNS options"); c->no_dns = 1; break; } - if (c->no_dns) { - err("Conflicting DNS options"); - usage(argv[0]); - } + if (c->no_dns) + die("Conflicting DNS options"); if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) && inet_pton(AF_INET, optarg, dns4)) { @@ -1534,29 +1460,22 @@ void conf(struct ctx *c, int argc, char **argv) break; } - err("Cannot use DNS address %s", optarg); - usage(argv[0]); + die("Cannot use DNS address %s", optarg); break; case 'S': if (!strcmp(optarg, "none")) { - if (c->no_dns_search) { - err("Redundant DNS search options"); - usage(argv[0]); - } + if (c->no_dns_search) + die("Redundant DNS search options"); - if (dnss != c->dns_search) { - err("Conflicting DNS search options"); - usage(argv[0]); - } + if (dnss != c->dns_search) + die("Conflicting DNS search options"); c->no_dns_search = 1; break; } - if (c->no_dns_search) { - err("Conflicting DNS search options"); - usage(argv[0]); - } + if (c->no_dns_search) + die("Conflicting DNS search options"); if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) { ret = snprintf(dnss->n, sizeof(*c->dns_search), @@ -1568,8 +1487,7 @@ void conf(struct ctx *c, int argc, char **argv) break; } - err("Cannot use DNS search domain %s", optarg); - usage(argv[0]); + die("Cannot use DNS search domain %s", optarg); break; case '4': v4_only = true; @@ -1578,15 +1496,11 @@ void conf(struct ctx *c, int argc, char **argv) v6_only = true; break; case '1': - if (c->mode != MODE_PASST) { - err("--one-off is for passt mode only"); - usage(argv[0]); - } + if (c->mode != MODE_PASST) + die("--one-off is for passt mode only"); - if (c->one_off) { - err("Redundant --one-off option"); - usage(argv[0]); - } + if (c->one_off) + die("Redundant --one-off option"); c->one_off = true; break; @@ -1604,15 +1518,11 @@ void conf(struct ctx *c, int argc, char **argv) } } while (name != -1); - if (v4_only && v6_only) { - err("Options ipv4-only and ipv6-only are mutually exclusive"); - usage(argv[0]); - } + if (v4_only && v6_only) + die("Options ipv4-only and ipv6-only are mutually exclusive"); - if (*c->sock_path && c->fd_tap >= 0) { - err("Options --socket and --fd are mutually exclusive"); - usage(argv[0]); - } + if (*c->sock_path && c->fd_tap >= 0) + die("Options --socket and --fd are mutually exclusive"); ret = conf_ugid(runas, &uid, &gid); if (ret) @@ -1628,10 +1538,8 @@ void conf(struct ctx *c, int argc, char **argv) c->ifi4 = conf_ip4(ifi, &c->ip4, c->mac); if (!v4_only) c->ifi6 = conf_ip6(ifi, &c->ip6, c->mac); - if (!c->ifi4 && !c->ifi6) { - err("External interface not usable"); - exit(EXIT_FAILURE); - } + if (!c->ifi4 && !c->ifi6) + die("External interface not usable"); /* Inbound port options can be parsed now (after IPv4/IPv6 settings) */ optind = 1; -- 2.39.1

David Gibson

16 Feb 16 Feb

6:34 a.m.

New subject: [PATCH v4 3/9] eliminate most calls to usage() in conf()

On Wed, Feb 15, 2023 at 03:24:31AM -0500, Laine Stump wrote:

...

Nearly all of the calls to usage() in conf() occur immediately after logging a more detailed error message, and the fact that these errors are occuring indicates that the user has already seen the passt usage message (or read the manpage). Spamming the logfile with the complete contents of the usage message serves only to obscure the more detailed error message. The only time when the full usage message should be output is if the user explicitly asks for it with -h (or its synonyms)

As a start to eliminating the excessive calls to usage(), this patch replaces most calls to err() followed by usage() with a call to die() instead. A few other usage() calls remain, but their removal involves bit more nuance that should be properly explained in separate commit messages.

Signed-off-by: Laine Stump

Reviewed-by: David Gibson -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 4/9] make conf_ports() exit immediately after logging error

Rather than having conf_ports() (possibly) log an error, and then letting the caller log the entire usage() message and exit, just log the errors and exit immediately (using die()). For some errors, conf_ports would previously not log any specific message, leaving it up to the user to determine the problem by guessing. We replace all of those silent returns with die() (logging a specific error), thus permitting us to make conf_ports() return void, which simplifies the caller. While modifying the two callers to conf_ports() to not check for a return value, we can further simplify the code by removing the check for a non-null optarg, as that is guaranteed to never happen (due to prior calls to getopt_long() with "argument required" for all relevant options - getopt_long() would have already caught this error). Signed-off-by: Laine Stump --- conf.c | 52 ++++++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/conf.c b/conf.c index ad8c249..0d4ff79 100644 --- a/conf.c +++ b/conf.c @@ -173,11 +173,9 @@ static int parse_port_range(const char *s, char **endptr, * @optname: Short option name, t, T, u, or U * @optarg: Option argument (port specification) * @fwd: Pointer to @port_fwd to be updated - * - * Return: -EINVAL on parsing error, 0 otherwise */ -static int conf_ports(const struct ctx *c, char optname, const char *optarg, - struct port_fwd *fwd) +static void conf_ports(const struct ctx *c, char optname, const char *optarg, + struct port_fwd *fwd) { char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf; char buf[BUFSIZ], *spec, *ifname = NULL, *p; @@ -187,23 +185,32 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, if (!strcmp(optarg, "none")) { if (fwd->mode) - return -EINVAL; + goto mode_conflict; + fwd->mode = FWD_NONE; - return 0; + return; } if (!strcmp(optarg, "auto")) { - if (fwd->mode || c->mode != MODE_PASTA) - return -EINVAL; + if (fwd->mode) + goto mode_conflict; + + if (c->mode != MODE_PASTA) + die("'auto' port forwarding is only allowed for pasta"); + fwd->mode = FWD_AUTO; - return 0; + return; } if (!strcmp(optarg, "all")) { unsigned i; - if (fwd->mode || c->mode != MODE_PASST) - return -EINVAL; + if (fwd->mode) + goto mode_conflict; + + if (c->mode != MODE_PASST) + die("'all' port forwarding is only allowed for passt"); + fwd->mode = FWD_ALL; memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8); @@ -214,11 +221,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i); } - return 0; + return; } if (fwd->mode > FWD_SPEC) - return -EINVAL; + die("Specific ports cannot be specified together with all/none/auto"); fwd->mode = FWD_SPEC; @@ -292,7 +299,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, udp_sock_init(c, 0, af, addr, ifname, i); } - return 0; + return; } /* Now process base ranges, skipping exclusions */ @@ -339,14 +346,13 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, } } while ((p = next_chunk(p, ','))); - return 0; + return; bad: - err("Invalid port specifier %s", optarg); - return -EINVAL; - + die("Invalid port specifier %s", optarg); overlap: - err("Overlapping port specifier %s", optarg); - return -EINVAL; + die("Overlapping port specifier %s", optarg); +mode_conflict: + die("Port forwarding mode '%s' conflicts with previous mode", optarg); } /** @@ -1550,8 +1556,7 @@ void conf(struct ctx *c, int argc, char **argv) if ((name == 't' && (fwd = &c->tcp.fwd_in)) || (name == 'u' && (fwd = &c->udp.fwd_in.f))) { - if (!optarg || conf_ports(c, name, optarg, fwd)) - usage(argv[0]); + conf_ports(c, name, optarg, fwd); } } while (name != -1); @@ -1589,8 +1594,7 @@ void conf(struct ctx *c, int argc, char **argv) if ((name == 'T' && (fwd = &c->tcp.fwd_out)) || (name == 'U' && (fwd = &c->udp.fwd_out.f))) { - if (!optarg || conf_ports(c, name, optarg, fwd)) - usage(argv[0]); + conf_ports(c, name, optarg, fwd); } } while (name != -1); -- 2.39.1

David Gibson

16 Feb 16 Feb

6:36 a.m.

New subject: [PATCH v4 4/9] make conf_ports() exit immediately after logging error

On Wed, Feb 15, 2023 at 03:24:32AM -0500, Laine Stump wrote:

...

Rather than having conf_ports() (possibly) log an error, and then letting the caller log the entire usage() message and exit, just log the errors and exit immediately (using die()).

For some errors, conf_ports would previously not log any specific message, leaving it up to the user to determine the problem by guessing. We replace all of those silent returns with die() (logging a specific error), thus permitting us to make conf_ports() return void, which simplifies the caller.

While modifying the two callers to conf_ports() to not check for a return value, we can further simplify the code by removing the check for a non-null optarg, as that is guaranteed to never happen (due to prior calls to getopt_long() with "argument required" for all relevant options - getopt_long() would have already caught this error).

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

...

--- conf.c | 52 ++++++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/conf.c b/conf.c index ad8c249..0d4ff79 100644 --- a/conf.c +++ b/conf.c @@ -173,11 +173,9 @@ static int parse_port_range(const char *s, char **endptr, * @optname: Short option name, t, T, u, or U * @optarg: Option argument (port specification) * @fwd: Pointer to @port_fwd to be updated - * - * Return: -EINVAL on parsing error, 0 otherwise */ -static int conf_ports(const struct ctx *c, char optname, const char *optarg, - struct port_fwd *fwd) +static void conf_ports(const struct ctx *c, char optname, const char *optarg, + struct port_fwd *fwd) { char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf; char buf[BUFSIZ], *spec, *ifname = NULL, *p; @@ -187,23 +185,32 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,

if (!strcmp(optarg, "none")) { if (fwd->mode) - return -EINVAL; + goto mode_conflict; + fwd->mode = FWD_NONE; - return 0; + return; }

if (!strcmp(optarg, "auto")) { - if (fwd->mode || c->mode != MODE_PASTA) - return -EINVAL; + if (fwd->mode) + goto mode_conflict; + + if (c->mode != MODE_PASTA) + die("'auto' port forwarding is only allowed for pasta"); + fwd->mode = FWD_AUTO; - return 0; + return; }

if (!strcmp(optarg, "all")) { unsigned i;

- if (fwd->mode || c->mode != MODE_PASST) - return -EINVAL; + if (fwd->mode) + goto mode_conflict; + + if (c->mode != MODE_PASST) + die("'all' port forwarding is only allowed for passt"); + fwd->mode = FWD_ALL; memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);

@@ -214,11 +221,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i); }

- return 0; + return; }

if (fwd->mode > FWD_SPEC) - return -EINVAL; + die("Specific ports cannot be specified together with all/none/auto");

fwd->mode = FWD_SPEC;

@@ -292,7 +299,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, udp_sock_init(c, 0, af, addr, ifname, i); }

- return 0; + return; }

/* Now process base ranges, skipping exclusions */ @@ -339,14 +346,13 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, } } while ((p = next_chunk(p, ',')));

- return 0; + return; bad: - err("Invalid port specifier %s", optarg); - return -EINVAL; - + die("Invalid port specifier %s", optarg); overlap: - err("Overlapping port specifier %s", optarg); - return -EINVAL; + die("Overlapping port specifier %s", optarg); +mode_conflict: + die("Port forwarding mode '%s' conflicts with previous mode", optarg); }

/** @@ -1550,8 +1556,7 @@ void conf(struct ctx *c, int argc, char **argv)

if ((name == 't' && (fwd = &c->tcp.fwd_in)) || (name == 'u' && (fwd = &c->udp.fwd_in.f))) { - if (!optarg || conf_ports(c, name, optarg, fwd)) - usage(argv[0]); + conf_ports(c, name, optarg, fwd); } } while (name != -1);

@@ -1589,8 +1594,7 @@ void conf(struct ctx *c, int argc, char **argv)

if ((name == 'T' && (fwd = &c->tcp.fwd_out)) || (name == 'U' && (fwd = &c->udp.fwd_out.f))) { - if (!optarg || conf_ports(c, name, optarg, fwd)) - usage(argv[0]); + conf_ports(c, name, optarg, fwd); } } while (name != -1);

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 5/9] make conf_pasta_ns() exit immediately after logging error

As with conf_ports, this allows us to make the function return void. Signed-off-by: Laine Stump --- conf.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/conf.c b/conf.c index 0d4ff79..c7ed64c 100644 --- a/conf.c +++ b/conf.c @@ -497,21 +497,15 @@ static int conf_netns_opt(char *netns, const char *arg) * @optind: Index of first non-option argument * @argc: Number of arguments * @argv: Command line arguments - * - * Return: 0 on success, negative error code otherwise */ -static int conf_pasta_ns(int *netns_only, char *userns, char *netns, - int optind, int argc, char *argv[]) +static void conf_pasta_ns(int *netns_only, char *userns, char *netns, + int optind, int argc, char *argv[]) { - if (*netns_only && *userns) { - err("Both --userns and --netns-only given"); - return -EINVAL; - } + if (*netns_only && *userns) + die("Both --userns and --netns-only given"); - if (*netns && optind != argc) { - err("Both --netns and PID or command given"); - return -EINVAL; - } + if (*netns && optind != argc) + die("Both --netns and PID or command given"); if (optind + 1 == argc) { char *endptr; @@ -520,10 +514,8 @@ static int conf_pasta_ns(int *netns_only, char *userns, char *netns, pidval = strtol(argv[optind], &endptr, 10); if (!*endptr) { /* Looks like a pid */ - if (pidval < 0 || pidval > INT_MAX) { - err("Invalid PID %s", argv[optind]); - return -EINVAL; - } + if (pidval < 0 || pidval > INT_MAX) + die("Invalid PID %s", argv[optind]); snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval); if (!*userns) @@ -535,8 +527,6 @@ static int conf_pasta_ns(int *netns_only, char *userns, char *netns, /* Attaching to a netns/PID, with no userns given */ if (*netns && !*userns) *netns_only = 1; - - return 0; } /** conf_ip4_prefix() - Parse an IPv4 prefix length or netmask @@ -1560,13 +1550,10 @@ void conf(struct ctx *c, int argc, char **argv) } } while (name != -1); - if (c->mode == MODE_PASTA) { - if (conf_pasta_ns(&netns_only, userns, netns, - optind, argc, argv) < 0) - usage(argv[0]); - } else if (optind != argc) { + if (c->mode == MODE_PASTA) + conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv); + else if (optind != argc) usage(argv[0]); - } isolate_user(uid, gid, !netns_only, userns, c->mode); -- 2.39.1

David Gibson

16 Feb 16 Feb

6:37 a.m.

New subject: [PATCH v4 5/9] make conf_pasta_ns() exit immediately after logging error

On Wed, Feb 15, 2023 at 03:24:33AM -0500, Laine Stump wrote:

...

As with conf_ports, this allows us to make the function return void.

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

...

--- conf.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/conf.c b/conf.c index 0d4ff79..c7ed64c 100644 --- a/conf.c +++ b/conf.c @@ -497,21 +497,15 @@ static int conf_netns_opt(char *netns, const char *arg) * @optind: Index of first non-option argument * @argc: Number of arguments * @argv: Command line arguments - * - * Return: 0 on success, negative error code otherwise */ -static int conf_pasta_ns(int *netns_only, char *userns, char *netns, - int optind, int argc, char *argv[]) +static void conf_pasta_ns(int *netns_only, char *userns, char *netns, + int optind, int argc, char *argv[]) { - if (*netns_only && *userns) { - err("Both --userns and --netns-only given"); - return -EINVAL; - } + if (*netns_only && *userns) + die("Both --userns and --netns-only given");

- if (*netns && optind != argc) { - err("Both --netns and PID or command given"); - return -EINVAL; - } + if (*netns && optind != argc) + die("Both --netns and PID or command given");

if (optind + 1 == argc) { char *endptr; @@ -520,10 +514,8 @@ static int conf_pasta_ns(int *netns_only, char *userns, char *netns, pidval = strtol(argv[optind], &endptr, 10); if (!*endptr) { /* Looks like a pid */ - if (pidval < 0 || pidval > INT_MAX) { - err("Invalid PID %s", argv[optind]); - return -EINVAL; - } + if (pidval < 0 || pidval > INT_MAX) + die("Invalid PID %s", argv[optind]);

snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval); if (!*userns) @@ -535,8 +527,6 @@ static int conf_pasta_ns(int *netns_only, char *userns, char *netns, /* Attaching to a netns/PID, with no userns given */ if (*netns && !*userns) *netns_only = 1; - - return 0; }

/** conf_ip4_prefix() - Parse an IPv4 prefix length or netmask @@ -1560,13 +1550,10 @@ void conf(struct ctx *c, int argc, char **argv) } } while (name != -1);

- if (c->mode == MODE_PASTA) { - if (conf_pasta_ns(&netns_only, userns, netns, - optind, argc, argv) < 0) - usage(argv[0]); - } else if (optind != argc) { + if (c->mode == MODE_PASTA) + conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv); + else if (optind != argc) usage(argv[0]); - }

isolate_user(uid, gid, !netns_only, userns, c->mode);

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 6/9] make conf_ugid() exit immediately after logging error

Again, it can then be made to return void, simplifying the caller. Signed-off-by: Laine Stump --- conf.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/conf.c b/conf.c index c7ed64c..19020f9 100644 --- a/conf.c +++ b/conf.c @@ -995,22 +995,18 @@ static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid) * @runas: --runas option, may be NULL * @uid: User ID, set on success * @gid: Group ID, set on success - * - * Return: 0 on success, negative error code on failure */ -static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) +static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) { const char root_uid_map[] = " 0 0 4294967295"; char buf[BUFSIZ]; - int ret; int fd; /* If user has specified --runas, that takes precedence... */ if (runas) { - ret = conf_runas(runas, uid, gid); - if (ret) - err("Invalid --runas option: %s", runas); - return ret; + if (conf_runas(runas, uid, gid)) + die("Invalid --runas option: %s", runas); + return; } /* ...otherwise default to current user and group... */ @@ -1019,20 +1015,18 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) /* ...as long as it's not root... */ if (*uid) - return 0; + return; /* ...or at least not root in the init namespace... */ if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) { - ret = -errno; - err("Can't determine if we're in init namespace: %s", - strerror(-ret)); - return ret; + die("Can't determine if we're in init namespace: %s", + strerror(errno)); } if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) || strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) { close(fd); - return 0; + return; } close(fd); @@ -1056,7 +1050,6 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) *uid = *gid = 65534; #endif } - return 0; } /** @@ -1520,9 +1513,7 @@ void conf(struct ctx *c, int argc, char **argv) if (*c->sock_path && c->fd_tap >= 0) die("Options --socket and --fd are mutually exclusive"); - ret = conf_ugid(runas, &uid, &gid); - if (ret) - usage(argv[0]); + conf_ugid(runas, &uid, &gid); if (logfile) { logfile_init(c->mode == MODE_PASST ? "passt" : "pasta", -- 2.39.1

David Gibson

16 Feb 16 Feb

6:38 a.m.

New subject: [PATCH v4 6/9] make conf_ugid() exit immediately after logging error

On Wed, Feb 15, 2023 at 03:24:34AM -0500, Laine Stump wrote:

...

Again, it can then be made to return void, simplifying the caller.

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

...

--- conf.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/conf.c b/conf.c index c7ed64c..19020f9 100644 --- a/conf.c +++ b/conf.c @@ -995,22 +995,18 @@ static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid) * @runas: --runas option, may be NULL * @uid: User ID, set on success * @gid: Group ID, set on success - * - * Return: 0 on success, negative error code on failure */ -static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) +static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) { const char root_uid_map[] = " 0 0 4294967295"; char buf[BUFSIZ]; - int ret; int fd;

/* If user has specified --runas, that takes precedence... */ if (runas) { - ret = conf_runas(runas, uid, gid); - if (ret) - err("Invalid --runas option: %s", runas); - return ret; + if (conf_runas(runas, uid, gid)) + die("Invalid --runas option: %s", runas); + return; }

/* ...otherwise default to current user and group... */ @@ -1019,20 +1015,18 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)

/* ...as long as it's not root... */ if (*uid) - return 0; + return;

/* ...or at least not root in the init namespace... */ if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) { - ret = -errno; - err("Can't determine if we're in init namespace: %s", - strerror(-ret)); - return ret; + die("Can't determine if we're in init namespace: %s", + strerror(errno)); }

if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) || strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) { close(fd); - return 0; + return; }

close(fd); @@ -1056,7 +1050,6 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) *uid = *gid = 65534; #endif } - return 0; }

/** @@ -1520,9 +1513,7 @@ void conf(struct ctx *c, int argc, char **argv) if (*c->sock_path && c->fd_tap >= 0) die("Options --socket and --fd are mutually exclusive");

- ret = conf_ugid(runas, &uid, &gid); - if (ret) - usage(argv[0]); + conf_ugid(runas, &uid, &gid);

if (logfile) { logfile_init(c->mode == MODE_PASST ? "passt" : "pasta",

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 7/9] make conf_netns_opt() exit immediately after logging error

...and return void to simplify the caller. Signed-off-by: Laine Stump --- conf.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/conf.c b/conf.c index 19020f9..d020b4f 100644 --- a/conf.c +++ b/conf.c @@ -464,10 +464,8 @@ out: * conf_netns_opt() - Parse --netns option * @netns: buffer of size PATH_MAX, updated with netns path * @arg: --netns argument - * - * Return: 0 on success, negative error code otherwise */ -static int conf_netns_opt(char *netns, const char *arg) +static void conf_netns_opt(char *netns, const char *arg) { int ret; @@ -479,12 +477,8 @@ static int conf_netns_opt(char *netns, const char *arg) 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; + if (ret <= 0 || ret > PATH_MAX) + die("Network namespace name/path %s too long"); } /** @@ -1157,9 +1151,7 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode != MODE_PASTA) die("--netns is for pasta mode only"); - ret = conf_netns_opt(netns, optarg); - if (ret < 0) - usage(argv[0]); + conf_netns_opt(netns, optarg); break; case 4: if (c->mode != MODE_PASTA) -- 2.39.1

David Gibson

16 Feb 16 Feb

6:38 a.m.

New subject: [PATCH v4 7/9] make conf_netns_opt() exit immediately after logging error

On Wed, Feb 15, 2023 at 03:24:35AM -0500, Laine Stump wrote:

...

...and return void to simplify the caller.

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

...

--- conf.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/conf.c b/conf.c index 19020f9..d020b4f 100644 --- a/conf.c +++ b/conf.c @@ -464,10 +464,8 @@ out: * conf_netns_opt() - Parse --netns option * @netns: buffer of size PATH_MAX, updated with netns path * @arg: --netns argument - * - * Return: 0 on success, negative error code otherwise */ -static int conf_netns_opt(char *netns, const char *arg) +static void conf_netns_opt(char *netns, const char *arg) { int ret;

@@ -479,12 +477,8 @@ static int conf_netns_opt(char *netns, const char *arg) 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; + if (ret <= 0 || ret > PATH_MAX) + die("Network namespace name/path %s too long"); }

/** @@ -1157,9 +1151,7 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode != MODE_PASTA) die("--netns is for pasta mode only");

- ret = conf_netns_opt(netns, optarg); - if (ret < 0) - usage(argv[0]); + conf_netns_opt(netns, optarg); break; case 4: if (c->mode != MODE_PASTA)

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 8/9] log a detailed error (not usage()) when there are extra non-option arguments

Signed-off-by: Laine Stump --- conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conf.c b/conf.c index d020b4f..f175405 100644 --- a/conf.c +++ b/conf.c @@ -1536,7 +1536,7 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode == MODE_PASTA) conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv); else if (optind != argc) - usage(argv[0]); + die("Extra non-option argument: %s", argv[optind]); isolate_user(uid, gid, !netns_only, userns, c->mode); -- 2.39.1

David Gibson

16 Feb 16 Feb

6:39 a.m.

New subject: [PATCH v4 8/9] log a detailed error (not usage()) when there are extra non-option arguments

On Wed, Feb 15, 2023 at 03:24:36AM -0500, Laine Stump wrote:

...

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

...

--- conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conf.c b/conf.c index d020b4f..f175405 100644 --- a/conf.c +++ b/conf.c @@ -1536,7 +1536,7 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode == MODE_PASTA) conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv); else if (optind != argc) - usage(argv[0]); + die("Extra non-option argument: %s", argv[optind]);

isolate_user(uid, gid, !netns_only, userns, c->mode);

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laine Stump

15 Feb 15 Feb

9:24 a.m.

New subject: [PATCH v4 9/9] convert all remaining err() followed by exit() to die()

This actually leaves us with 0 uses of err(), but someone could want to use it in the future, so we may as well leave it around. Signed-off-by: Laine Stump --- isolation.c | 67 ++++++++++++++++++----------------------------------- log.c | 6 ++--- netlink.c | 3 +-- passt.c | 12 ++++------ pasta.c | 20 ++++++---------- tap.c | 30 ++++++++---------------- 6 files changed, 47 insertions(+), 91 deletions(-) diff --git a/isolation.c b/isolation.c index 4e6637d..6bae4d4 100644 --- a/isolation.c +++ b/isolation.c @@ -103,10 +103,8 @@ static void drop_caps_ep_except(uint64_t keep) struct __user_cap_data_struct data[CAP_WORDS]; int i; - if (syscall(SYS_capget, &hdr, data)) { - err("Couldn't get current capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (syscall(SYS_capget, &hdr, data)) + die("Couldn't get current capabilities: %s", strerror(errno)); for (i = 0; i < CAP_WORDS; i++) { uint32_t mask = keep >> (32 * i); @@ -115,10 +113,8 @@ static void drop_caps_ep_except(uint64_t keep) data[i].permitted &= mask; } - if (syscall(SYS_capset, &hdr, data)) { - err("Couldn't drop capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (syscall(SYS_capset, &hdr, data)) + die("Couldn't drop capabilities: %s", strerror(errno)); } /** @@ -154,26 +150,20 @@ static void clamp_caps(void) * normal operation, so carry on without it. */ if (prctl(PR_CAPBSET_DROP, i, 0, 0, 0) && - errno != EINVAL && errno != EPERM) { - err("Couldn't drop cap %i from bounding set: %s", + errno != EINVAL && errno != EPERM) + die("Couldn't drop cap %i from bounding set: %s", i, strerror(errno)); - exit(EXIT_FAILURE); - } } - if (syscall(SYS_capget, &hdr, data)) { - err("Couldn't get current capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (syscall(SYS_capget, &hdr, data)) + die("Couldn't get current capabilities: %s", strerror(errno)); for (i = 0; i < CAP_WORDS; i++) data[i].inheritable = 0; - if (syscall(SYS_capset, &hdr, data)) { - err("Couldn't drop inheritable capabilities: %s", + if (syscall(SYS_capset, &hdr, data)) + die("Couldn't drop inheritable capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } } /** @@ -229,46 +219,35 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, /* First set our UID & GID in the original namespace */ if (setgroups(0, NULL)) { /* If we don't have CAP_SETGID, this will EPERM */ - if (errno != EPERM) { - err("Can't drop supplementary groups: %s", + if (errno != EPERM) + die("Can't drop supplementary groups: %s", strerror(errno)); - exit(EXIT_FAILURE); - } } - if (setgid(gid) != 0) { - err("Can't set GID to %u: %s", gid, strerror(errno)); - exit(EXIT_FAILURE); - } + if (setgid(gid) != 0) + die("Can't set GID to %u: %s", gid, strerror(errno)); - if (setuid(uid) != 0) { - err("Can't set UID to %u: %s", uid, strerror(errno)); - exit(EXIT_FAILURE); - } + if (setuid(uid) != 0) + die("Can't set UID to %u: %s", uid, strerror(errno)); if (*userns) { /* If given a userns, join it */ int ufd; ufd = open(userns, O_RDONLY | O_CLOEXEC); - if (ufd < 0) { - err("Couldn't open user namespace %s: %s", + if (ufd < 0) + die("Couldn't open user namespace %s: %s", userns, strerror(errno)); - exit(EXIT_FAILURE); - } - if (setns(ufd, CLONE_NEWUSER) != 0) { - err("Couldn't enter user namespace %s: %s", + if (setns(ufd, CLONE_NEWUSER) != 0) + die("Couldn't enter user namespace %s: %s", userns, strerror(errno)); - exit(EXIT_FAILURE); - } close(ufd); } else if (use_userns) { /* Create and join a new userns */ - if (unshare(CLONE_NEWUSER) != 0) { - err("Couldn't create user namespace: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (unshare(CLONE_NEWUSER) != 0) + die("Couldn't create user namespace: %s", + strerror(errno)); } /* Joining a new userns gives us full capabilities; drop the diff --git a/log.c b/log.c index 2920aba..785bc36 100644 --- a/log.c +++ b/log.c @@ -193,10 +193,8 @@ void logfile_init(const char *name, const char *path, size_t size) log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR); - if (log_file == -1) { - err("Couldn't open log file %s: %s", path, strerror(errno)); - exit(EXIT_FAILURE); - } + if (log_file == -1) + die("Couldn't open log file %s: %s", path, strerror(errno)); log_size = size ? size : LOGFILE_SIZE_DEFAULT; diff --git a/netlink.c b/netlink.c index b8fa2a0..8f785ca 100644 --- a/netlink.c +++ b/netlink.c @@ -90,8 +90,7 @@ void nl_sock_init(const struct ctx *c, bool ns) return; fail: - err("Failed to get netlink socket"); - exit(EXIT_FAILURE); + die("Failed to get netlink socket"); } /** diff --git a/passt.c b/passt.c index c48c2d5..5b8146e 100644 --- a/passt.c +++ b/passt.c @@ -202,10 +202,8 @@ int main(int argc, char **argv) name = basename(argv0); if (strstr(name, "pasta")) { sa.sa_handler = pasta_child_handler; - if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN)) { - err("Couldn't install signal handlers"); - exit(EXIT_FAILURE); - } + if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN)) + die("Couldn't install signal handlers"); c.mode = MODE_PASTA; log_name = "pasta"; @@ -284,10 +282,8 @@ int main(int argc, char **argv) } } - if (isolate_prefork(&c)) { - err("Failed to sandbox process, exiting\n"); - exit(EXIT_FAILURE); - } + if (isolate_prefork(&c)) + die("Failed to sandbox process, exiting"); /* Once the log mask is not LOG_EMERG, we will no longer * log to stderr if there was a log file specified. diff --git a/pasta.c b/pasta.c index d4d3dc8..6c9a412 100644 --- a/pasta.c +++ b/pasta.c @@ -131,19 +131,15 @@ void pasta_open_ns(struct ctx *c, const char *netns) int nfd = -1; nfd = open(netns, O_RDONLY | O_CLOEXEC); - if (nfd < 0) { - err("Couldn't open network namespace %s", netns); - exit(EXIT_FAILURE); - } + if (nfd < 0) + die("Couldn't open network namespace %s", netns); c->pasta_netns_fd = nfd; NS_CALL(ns_check, c); - if (c->pasta_netns_fd < 0) { - err("Couldn't switch to pasta namespaces"); - exit(EXIT_FAILURE); - } + if (c->pasta_netns_fd < 0) + die("Couldn't switch to pasta namespaces"); if (!c->no_netns_quit) { char buf[PATH_MAX] = { 0 }; @@ -232,11 +228,9 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, arg.exe = "/bin/sh"; if ((size_t)snprintf(sh_arg0, sizeof(sh_arg0), - "-%s", arg.exe) >= sizeof(sh_arg0)) { - err("$SHELL is too long (%u bytes)", - strlen(arg.exe)); - exit(EXIT_FAILURE); - } + "-%s", arg.exe) >= sizeof(sh_arg0)) + die("$SHELL is too long (%u bytes)", strlen(arg.exe)); + sh_argv[0] = sh_arg0; arg.argv = sh_argv; } diff --git a/tap.c b/tap.c index 716d887..02da84d 100644 --- a/tap.c +++ b/tap.c @@ -1008,10 +1008,8 @@ static void tap_sock_unix_init(struct ctx *c) }; int i; - if (fd < 0) { - err("UNIX socket: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (fd < 0) + die("UNIX socket: %s", strerror(errno)); /* In passt mode, we don't know the guest's MAC until it sends * us packets. Use the broadcast address so our first packets @@ -1029,18 +1027,14 @@ static void tap_sock_unix_init(struct ctx *c) snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i); ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0); - if (ex < 0) { - err("UNIX domain socket check: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (ex < 0) + die("UNIX domain socket check: %s", strerror(errno)); ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); if (!ret || (errno != ENOENT && errno != ECONNREFUSED && errno != EACCES)) { - if (*c->sock_path) { - err("Socket path %s already in use", path); - exit(EXIT_FAILURE); - } + if (*c->sock_path) + die("Socket path %s already in use", path); close(ex); continue; @@ -1053,10 +1047,8 @@ static void tap_sock_unix_init(struct ctx *c) break; } - if (i == UNIX_SOCK_MAX) { - err("UNIX socket bind: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (i == UNIX_SOCK_MAX) + die("UNIX socket bind: %s", strerror(errno)); info("UNIX domain socket bound at %s\n", addr.sun_path); @@ -1159,10 +1151,8 @@ static void tap_sock_tun_init(struct ctx *c) struct epoll_event ev = { 0 }; NS_CALL(tap_ns_tun, c); - if (tun_ns_fd == -1) { - err("Failed to open tun socket in namespace"); - exit(EXIT_FAILURE); - } + if (tun_ns_fd == -1) + die("Failed to open tun socket in namespace"); pasta_ns_conf(c); -- 2.39.1

David Gibson

16 Feb 16 Feb

6:40 a.m.

New subject: [PATCH v4 9/9] convert all remaining err() followed by exit() to die()

On Wed, Feb 15, 2023 at 03:24:37AM -0500, Laine Stump wrote:

...

This actually leaves us with 0 uses of err(), but someone could want to use it in the future, so we may as well leave it around.

Signed-off-by: Laine Stump

Reviewed-by: David Gibson

...

--- isolation.c | 67 ++++++++++++++++++----------------------------------- log.c | 6 ++--- netlink.c | 3 +-- passt.c | 12 ++++------ pasta.c | 20 ++++++---------- tap.c | 30 ++++++++---------------- 6 files changed, 47 insertions(+), 91 deletions(-)

diff --git a/isolation.c b/isolation.c index 4e6637d..6bae4d4 100644 --- a/isolation.c +++ b/isolation.c @@ -103,10 +103,8 @@ static void drop_caps_ep_except(uint64_t keep) struct __user_cap_data_struct data[CAP_WORDS]; int i;

- if (syscall(SYS_capget, &hdr, data)) { - err("Couldn't get current capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (syscall(SYS_capget, &hdr, data)) + die("Couldn't get current capabilities: %s", strerror(errno));

for (i = 0; i < CAP_WORDS; i++) { uint32_t mask = keep >> (32 * i); @@ -115,10 +113,8 @@ static void drop_caps_ep_except(uint64_t keep) data[i].permitted &= mask; }

- if (syscall(SYS_capset, &hdr, data)) { - err("Couldn't drop capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (syscall(SYS_capset, &hdr, data)) + die("Couldn't drop capabilities: %s", strerror(errno)); }

/** @@ -154,26 +150,20 @@ static void clamp_caps(void) * normal operation, so carry on without it. */ if (prctl(PR_CAPBSET_DROP, i, 0, 0, 0) && - errno != EINVAL && errno != EPERM) { - err("Couldn't drop cap %i from bounding set: %s", + errno != EINVAL && errno != EPERM) + die("Couldn't drop cap %i from bounding set: %s", i, strerror(errno)); - exit(EXIT_FAILURE); - } }

- if (syscall(SYS_capget, &hdr, data)) { - err("Couldn't get current capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (syscall(SYS_capget, &hdr, data)) + die("Couldn't get current capabilities: %s", strerror(errno));

for (i = 0; i < CAP_WORDS; i++) data[i].inheritable = 0;

- if (syscall(SYS_capset, &hdr, data)) { - err("Couldn't drop inheritable capabilities: %s", + if (syscall(SYS_capset, &hdr, data)) + die("Couldn't drop inheritable capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } }

/** @@ -229,46 +219,35 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, /* First set our UID & GID in the original namespace */ if (setgroups(0, NULL)) { /* If we don't have CAP_SETGID, this will EPERM */ - if (errno != EPERM) { - err("Can't drop supplementary groups: %s", + if (errno != EPERM) + die("Can't drop supplementary groups: %s", strerror(errno)); - exit(EXIT_FAILURE); - } }

- if (setgid(gid) != 0) { - err("Can't set GID to %u: %s", gid, strerror(errno)); - exit(EXIT_FAILURE); - } + if (setgid(gid) != 0) + die("Can't set GID to %u: %s", gid, strerror(errno));

- if (setuid(uid) != 0) { - err("Can't set UID to %u: %s", uid, strerror(errno)); - exit(EXIT_FAILURE); - } + if (setuid(uid) != 0) + die("Can't set UID to %u: %s", uid, strerror(errno));

if (*userns) { /* If given a userns, join it */ int ufd;

ufd = open(userns, O_RDONLY | O_CLOEXEC); - if (ufd < 0) { - err("Couldn't open user namespace %s: %s", + if (ufd < 0) + die("Couldn't open user namespace %s: %s", userns, strerror(errno)); - exit(EXIT_FAILURE); - }

- if (setns(ufd, CLONE_NEWUSER) != 0) { - err("Couldn't enter user namespace %s: %s", + if (setns(ufd, CLONE_NEWUSER) != 0) + die("Couldn't enter user namespace %s: %s", userns, strerror(errno)); - exit(EXIT_FAILURE); - }

close(ufd);

} else if (use_userns) { /* Create and join a new userns */ - if (unshare(CLONE_NEWUSER) != 0) { - err("Couldn't create user namespace: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (unshare(CLONE_NEWUSER) != 0) + die("Couldn't create user namespace: %s", + strerror(errno)); }

/* Joining a new userns gives us full capabilities; drop the diff --git a/log.c b/log.c index 2920aba..785bc36 100644 --- a/log.c +++ b/log.c @@ -193,10 +193,8 @@ void logfile_init(const char *name, const char *path, size_t size)

log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR); - if (log_file == -1) { - err("Couldn't open log file %s: %s", path, strerror(errno)); - exit(EXIT_FAILURE); - } + if (log_file == -1) + die("Couldn't open log file %s: %s", path, strerror(errno));

log_size = size ? size : LOGFILE_SIZE_DEFAULT;

diff --git a/netlink.c b/netlink.c index b8fa2a0..8f785ca 100644 --- a/netlink.c +++ b/netlink.c @@ -90,8 +90,7 @@ void nl_sock_init(const struct ctx *c, bool ns) return;

fail: - err("Failed to get netlink socket"); - exit(EXIT_FAILURE); + die("Failed to get netlink socket"); }

/** diff --git a/passt.c b/passt.c index c48c2d5..5b8146e 100644 --- a/passt.c +++ b/passt.c @@ -202,10 +202,8 @@ int main(int argc, char **argv) name = basename(argv0); if (strstr(name, "pasta")) { sa.sa_handler = pasta_child_handler; - if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN)) { - err("Couldn't install signal handlers"); - exit(EXIT_FAILURE); - } + if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN)) + die("Couldn't install signal handlers");

c.mode = MODE_PASTA; log_name = "pasta"; @@ -284,10 +282,8 @@ int main(int argc, char **argv) } }

- if (isolate_prefork(&c)) { - err("Failed to sandbox process, exiting\n"); - exit(EXIT_FAILURE); - } + if (isolate_prefork(&c)) + die("Failed to sandbox process, exiting");

/* Once the log mask is not LOG_EMERG, we will no longer * log to stderr if there was a log file specified. diff --git a/pasta.c b/pasta.c index d4d3dc8..6c9a412 100644 --- a/pasta.c +++ b/pasta.c @@ -131,19 +131,15 @@ void pasta_open_ns(struct ctx *c, const char *netns) int nfd = -1;

nfd = open(netns, O_RDONLY | O_CLOEXEC); - if (nfd < 0) { - err("Couldn't open network namespace %s", netns); - exit(EXIT_FAILURE); - } + if (nfd < 0) + die("Couldn't open network namespace %s", netns);

c->pasta_netns_fd = nfd;

NS_CALL(ns_check, c);

- if (c->pasta_netns_fd < 0) { - err("Couldn't switch to pasta namespaces"); - exit(EXIT_FAILURE); - } + if (c->pasta_netns_fd < 0) + die("Couldn't switch to pasta namespaces");

if (!c->no_netns_quit) { char buf[PATH_MAX] = { 0 }; @@ -232,11 +228,9 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, arg.exe = "/bin/sh";

if ((size_t)snprintf(sh_arg0, sizeof(sh_arg0), - "-%s", arg.exe) >= sizeof(sh_arg0)) { - err("$SHELL is too long (%u bytes)", - strlen(arg.exe)); - exit(EXIT_FAILURE); - } + "-%s", arg.exe) >= sizeof(sh_arg0)) + die("$SHELL is too long (%u bytes)", strlen(arg.exe)); + sh_argv[0] = sh_arg0; arg.argv = sh_argv; } diff --git a/tap.c b/tap.c index 716d887..02da84d 100644 --- a/tap.c +++ b/tap.c @@ -1008,10 +1008,8 @@ static void tap_sock_unix_init(struct ctx *c) }; int i;

- if (fd < 0) { - err("UNIX socket: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (fd < 0) + die("UNIX socket: %s", strerror(errno));

/* In passt mode, we don't know the guest's MAC until it sends * us packets. Use the broadcast address so our first packets @@ -1029,18 +1027,14 @@ static void tap_sock_unix_init(struct ctx *c) snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);

ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0); - if (ex < 0) { - err("UNIX domain socket check: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (ex < 0) + die("UNIX domain socket check: %s", strerror(errno));

ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); if (!ret || (errno != ENOENT && errno != ECONNREFUSED && errno != EACCES)) { - if (*c->sock_path) { - err("Socket path %s already in use", path); - exit(EXIT_FAILURE); - } + if (*c->sock_path) + die("Socket path %s already in use", path);

close(ex); continue; @@ -1053,10 +1047,8 @@ static void tap_sock_unix_init(struct ctx *c) break; }

- if (i == UNIX_SOCK_MAX) { - err("UNIX socket bind: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (i == UNIX_SOCK_MAX) + die("UNIX socket bind: %s", strerror(errno));

info("UNIX domain socket bound at %s\n", addr.sun_path);

@@ -1159,10 +1151,8 @@ static void tap_sock_tun_init(struct ctx *c) struct epoll_event ev = { 0 };

NS_CALL(tap_ns_tun, c); - if (tun_ns_fd == -1) { - err("Failed to open tun socket in namespace"); - exit(EXIT_FAILURE); - } + if (tun_ns_fd == -1) + die("Failed to open tun socket in namespace");

pasta_ns_conf(c);

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Stefano Brivio

15 Feb 15 Feb

12:56 p.m.

On Wed, 15 Feb 2023 03:24:28 -0500 Laine Stump wrote:

...

There are two topics covered here:

1) If a logFile is set, passt's behavior has been to send all error messages there, and *not* to stderr. This makes it difficult for another program that is exec'ing passt (and setting it to log to a file) to report useful error messages when passt fails - everything after the point that the logfile is opened is sent only to the logfile. The first patch makes a simple change to the logging functions that uses the value of the system logmask to decide if it should force writing messages to stderr even when a logfile has been specified.

Change from "v2": I'm using Stefano's suggestion of "abusing" logmask, rather than adding a static bool to keep track.

Change from "v3": tweak a commend to Stefano's liking.

2) All the rest of the patches eliminate use of the blanket usage() function when a commandline error is encountered (and add in specific/details error messages when previously usage() was all that was logged), and replace calls to err() followed by exit() with a single call to the new function die().

Change from "v2": I changed the name of the "log and exit" function from "errexit()" to "die()" at the suggestion of Dave Gibson (Stefano concurred). Although it seems a bit more violent, it does make moot many/most of Stefano's nits about needing to split lines to eliminate

...
80 characters (I did address all the rest of the things he pointed out, though)

NB: Yes, this says it is v3, and the previous version I sent was v2, and there *was no v1* - this is because I didn't realize that git-publish is automatically incrementing the version number every time I run it, and I had done a test-drive sending the patches to my personal address prior to sending them to the list - *that* was v1.

Laine Stump (9): log to stderr until process is daemonized, even if a log file is set add die() to log an error message and exit with a single call eliminate most calls to usage() in conf() make conf_ports() exit immediately after logging error make conf_pasta_ns() exit immediately after logging error make conf_ugid() exit immediately after logging error make conf_netns_opt() exit immediately after logging error log a detailed error (not usage()) when there are extra non-option arguments convert all remaining err() followed by exit() to die()

This looks good to me now. I'll wait a bit longer for reviews before applying. -- Stefano

Stefano Brivio

16 Feb 16 Feb

11:21 p.m.

On Wed, 15 Feb 2023 03:24:28 -0500 Laine Stump wrote:

...

There are two topics covered here:

1) If a logFile is set, passt's behavior has been to send all error messages there, and *not* to stderr. This makes it difficult for another program that is exec'ing passt (and setting it to log to a file) to report useful error messages when passt fails - everything after the point that the logfile is opened is sent only to the logfile. The first patch makes a simple change to the logging functions that uses the value of the system logmask to decide if it should force writing messages to stderr even when a logfile has been specified.

Change from "v2": I'm using Stefano's suggestion of "abusing" logmask, rather than adding a static bool to keep track.

Change from "v3": tweak a commend to Stefano's liking.

2) All the rest of the patches eliminate use of the blanket usage() function when a commandline error is encountered (and add in specific/details error messages when previously usage() was all that was logged), and replace calls to err() followed by exit() with a single call to the new function die().

Change from "v2": I changed the name of the "log and exit" function from "errexit()" to "die()" at the suggestion of Dave Gibson (Stefano concurred). Although it seems a bit more violent, it does make moot many/most of Stefano's nits about needing to split lines to eliminate

...
80 characters (I did address all the rest of the things he pointed out, though)

NB: Yes, this says it is v3, and the previous version I sent was v2, and there *was no v1* - this is because I didn't realize that git-publish is automatically incrementing the version number every time I run it, and I had done a test-drive sending the patches to my personal address prior to sending them to the list - *that* was v1.

Laine Stump (9): log to stderr until process is daemonized, even if a log file is set add die() to log an error message and exit with a single call eliminate most calls to usage() in conf() make conf_ports() exit immediately after logging error make conf_pasta_ns() exit immediately after logging error make conf_ugid() exit immediately after logging error make conf_netns_opt() exit immediately after logging error log a detailed error (not usage()) when there are extra non-option arguments convert all remaining err() followed by exit() to die()

Applied. -- Stefano

839

Age (days ago)

840

Last active (days ago)

List overview

Download

22 comments

3 participants

participants (3)

David Gibson
Laine Stump
Stefano Brivio