[PATCH v2 0/9] error logging fixes

newer
[libvirt PATCH] qemu: allow passt...

older
[PATCH] tap: Send frames after the...

Laine Stump

8 Feb 2023 8 Feb '23

6:48 p.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. The first patch makes a simple change to the logging functions that checks a global bool that is set true after all initialization is completed. 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). Laine Stump (9): log to stderr until process is daemonized, even if a logfile is set add errexit() 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 errexit() conf.c | 471 ++++++++++++++++++++-------------------------------- isolation.c | 78 ++++----- log.c | 33 ++-- log.h | 2 + netlink.c | 3 +- passt.c | 18 +- pasta.c | 21 +-- tap.c | 30 ++-- 8 files changed, 258 insertions(+), 398 deletions(-) -- 2.39.1

Show replies by date

Laine Stump

8 Feb 8 Feb

6:48 p.m.

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

Signed-off-by: Laine Stump --- log.c | 14 +++++++++++++- log.h | 1 + passt.c | 6 ++++-- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/log.c b/log.c index 468c730..0ab0adf 100644 --- a/log.c +++ b/log.c @@ -34,6 +34,7 @@ static int log_sock = -1; /* Optional socket to system logger */ static char log_ident[BUFSIZ]; /* Identifier string for openlog() */ static int log_mask; /* Current log priority mask */ static int log_opt; /* Options for openlog() */ +static int log_daemon_mode = false; /* true once process is daemonized */ static int log_file = -1; /* Optional log file descriptor */ static size_t log_size; /* Maximum log file size in bytes */ @@ -67,7 +68,8 @@ void name(const char *format, ...) { \ } \ \ if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \ - setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \ + setlogmask(0) == LOG_MASK(LOG_EMERG)) \ + && (log_file == -1 || !log_daemon_mode)) { \ va_start(args, format); \ (void)vfprintf(stderr, format, args); \ va_end(args); \ @@ -91,6 +93,16 @@ logfn(warn, LOG_WARNING) logfn(info, LOG_INFO) logfn(debug, LOG_DEBUG) +/** + * log_go_daemon() - tell logging subsystem that the process has been + * been daemonized, so it stop logging to stderr + * if appropriate. + */ +void log_go_daemon(void) +{ + log_daemon_mode = true; +} + /** * trace_init() - Set log_trace depending on trace (debug) mode * @enable: Tracing debug mode enabled if non-zero diff --git a/log.h b/log.h index 987dc17..a57c777 100644 --- a/log.h +++ b/log.h @@ -25,6 +25,7 @@ void trace_init(int enable); void __openlog(const char *ident, int option, int facility); void logfile_init(const char *name, const char *path, size_t size); +void log_go_daemon(void); void passt_vsyslog(int pri, const char *format, va_list ap); void logfile_write(int pri, const char *format, va_list ap); void __setlogmask(int mask); diff --git a/passt.c b/passt.c index 8b2c50d..cf010e8 100644 --- a/passt.c +++ b/passt.c @@ -296,10 +296,12 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } - if (!c.foreground) + if (!c.foreground) { __daemon(pidfile_fd, devnull_fd); - else + log_go_daemon(); + } else { write_pidfile(pidfile_fd, getpid()); + } isolate_postfork(&c); -- 2.39.1

Stefano Brivio

9 Feb 9 Feb

6:45 p.m.

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

On Wed, 8 Feb 2023 12:48:30 -0500 Laine Stump wrote:

...

Signed-off-by: Laine Stump --- log.c | 14 +++++++++++++- log.h | 1 + passt.c | 6 ++++-- 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/log.c b/log.c index 468c730..0ab0adf 100644 --- a/log.c +++ b/log.c @@ -34,6 +34,7 @@ static int log_sock = -1; /* Optional socket to system logger */ static char log_ident[BUFSIZ]; /* Identifier string for openlog() */ static int log_mask; /* Current log priority mask */ static int log_opt; /* Options for openlog() */ +static int log_daemon_mode = false; /* true once process is daemonized */

static int log_file = -1; /* Optional log file descriptor */ static size_t log_size; /* Maximum log file size in bytes */ @@ -67,7 +68,8 @@ void name(const char *format, ...) { \ } \ \ if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \ - setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \ + setlogmask(0) == LOG_MASK(LOG_EMERG)) \ + && (log_file == -1 || !log_daemon_mode)) { \

This is getting a bit complicated. At the moment, LOG_EMERG is abused with the meaning "we don't know where/what to log yet", because we didn't process logging options yet. Would it be an option to extend this abuse a bit further, and use LOG_EMERG to indicate that passt didn't daemonise yet, instead? You would just need to move the __setlogmask() calls in main(), and change the comment about the first one. I haven't tested this, and we should be a bit careful with this (check what happens with and without running passt from an interactive terminal, with and without a log file, etc.). A possibly cleaner approach could be to decouple this from the log mask, and have an enum instead, to represent logging "states" or "modes" -- but I'm not sure we're really going to save much complexity with it. By the way, the man page should be updated as well. -- Stefano

Laine Stump

14 Feb 14 Feb

4:41 a.m.

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

On 2/9/23 12:45 PM, Stefano Brivio wrote:

...

On Wed, 8 Feb 2023 12:48:30 -0500 Laine Stump wrote:

...
Signed-off-by: Laine Stump --- log.c | 14 +++++++++++++- log.h | 1 + passt.c | 6 ++++-- 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/log.c b/log.c index 468c730..0ab0adf 100644 --- a/log.c +++ b/log.c @@ -34,6 +34,7 @@ static int log_sock = -1; /* Optional socket to system logger */ static char log_ident[BUFSIZ]; /* Identifier string for openlog() */ static int log_mask; /* Current log priority mask */ static int log_opt; /* Options for openlog() */ +static int log_daemon_mode = false; /* true once process is daemonized */

static int log_file = -1; /* Optional log file descriptor */ static size_t log_size; /* Maximum log file size in bytes */ @@ -67,7 +68,8 @@ void name(const char *format, ...) { \ } \ \ if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \ - setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \ + setlogmask(0) == LOG_MASK(LOG_EMERG)) \ + && (log_file == -1 || !log_daemon_mode)) { \

This is getting a bit complicated.

At the moment, LOG_EMERG is abused with the meaning "we don't know where/what to log yet", because we didn't process logging options yet.

Would it be an option to extend this abuse a bit further, and use LOG_EMERG to indicate that passt didn't daemonise yet, instead? You would just need to move the __setlogmask() calls in main(), and change the comment about the first one.

Well, the fact that I didn't understand this was what was being done with the log mask kind of indicates this abuse will be misunderstood in the long run :-P But the abuse is already there, so it's not like I'm really making it any (much?) worse. It's just increasing the window when the logmask is set to LOG_EMERG, while modifying the log macro to not care if log_file is opened or not. I'll try that and see how it works out.

...

I haven't tested this, and we should be a bit careful with this (check what happens with and without running passt from an interactive terminal, with and without a log file, etc.).

A possibly cleaner approach could be to decouple this from the log mask, and have an enum instead, to represent logging "states" or "modes" -- but I'm not sure we're really going to save much complexity with it.

If I'm understanding you, that's kind of what I was doing, it's just that there are two modes - false == "logging before being daemonized" and true == "logging after being daemonized".

...

By the way, the man page should be updated as well.

Good point.

Laine Stump

8 Feb 8 Feb

6:48 p.m.

New subject: [PATCH v2 2/9] add errexit() 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 errxit() that will log an error and then exit. Signed-off-by: Laine Stump --- log.c | 13 ++++++++----- log.h | 1 + 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/log.c b/log.c index 0ab0adf..4956914 100644 --- a/log.c +++ b/log.c @@ -45,7 +45,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; \ @@ -76,6 +76,8 @@ 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 */ @@ -88,10 +90,11 @@ const char *logfile_prefix[] = { " ", /* LOG_DEBUG */ }; -logfn(err, LOG_ERR) -logfn(warn, LOG_WARNING) -logfn(info, LOG_INFO) -logfn(debug, LOG_DEBUG) +logfn(errexit, LOG_ERR, 1) +logfn(err, LOG_ERR, 0) +logfn(warn, LOG_WARNING, 0) +logfn(info, LOG_INFO, 0) +logfn(debug, LOG_DEBUG, 0) /** * log_go_daemon() - tell logging subsystem that the process has been diff --git a/log.h b/log.h index a57c777..ed19415 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 errexit(const char *format, ...); void err(const char *format, ...); void warn(const char *format, ...); void info(const char *format, ...); -- 2.39.1

Stefano Brivio

9 Feb 9 Feb

6:45 p.m.

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

On Wed, 8 Feb 2023 12:48:31 -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 errxit() that will log an error and then exit.

Signed-off-by: Laine Stump --- log.c | 13 ++++++++----- log.h | 1 + 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/log.c b/log.c index 0ab0adf..4956914 100644 --- a/log.c +++ b/log.c @@ -45,7 +45,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; \ @@ -76,6 +76,8 @@ void name(const char *format, ...) { \ if (format[strlen(format)] != '\n') \ fprintf(stderr, "\n"); \ } \ + if (doexit) \

A blank line before this would make it more consistent.

...

+ exit(EXIT_FAILURE); \ }

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

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

/** * log_go_daemon() - tell logging subsystem that the process has been diff --git a/log.h b/log.h index a57c777..ed19415 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 errexit(const char *format, ...); void err(const char *format, ...); void warn(const char *format, ...); void info(const char *format, ...);

Other than that, this looks good to me. -- Stefano

David Gibson

13 Feb 13 Feb

4:22 a.m.

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

On Thu, Feb 09, 2023 at 06:45:32PM +0100, Stefano Brivio wrote:

...

On Wed, 8 Feb 2023 12:48:31 -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 errxit() that will log an error and then exit.

Signed-off-by: Laine Stump --- log.c | 13 ++++++++----- log.h | 1 + 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/log.c b/log.c index 0ab0adf..4956914 100644 --- a/log.c +++ b/log.c @@ -45,7 +45,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; \ @@ -76,6 +76,8 @@ void name(const char *format, ...) { \ if (format[strlen(format)] != '\n') \ fprintf(stderr, "\n"); \ } \ + if (doexit) \

A blank line before this would make it more consistent.

...
+ exit(EXIT_FAILURE); \ }

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

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

/** * log_go_daemon() - tell logging subsystem that the process has been diff --git a/log.h b/log.h index a57c777..ed19415 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 errexit(const char *format, ...); void err(const char *format, ...); void warn(const char *format, ...); void info(const char *format, ...);

Other than that, this looks good to me.

LGTM. Personally I like to call such functions "die()". -- 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

11:46 a.m.

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

On Mon, 13 Feb 2023 14:22:33 +1100 David Gibson wrote:

...

On Thu, Feb 09, 2023 at 06:45:32PM +0100, Stefano Brivio wrote:

...
On Wed, 8 Feb 2023 12:48:31 -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 errxit() that will log an error and then exit.

Signed-off-by: Laine Stump --- log.c | 13 ++++++++----- log.h | 1 + 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/log.c b/log.c index 0ab0adf..4956914 100644 --- a/log.c +++ b/log.c @@ -45,7 +45,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; \ @@ -76,6 +76,8 @@ void name(const char *format, ...) { \ if (format[strlen(format)] != '\n') \ fprintf(stderr, "\n"); \ } \ + if (doexit) \

A blank line before this would make it more consistent.

...
+ exit(EXIT_FAILURE); \ }

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

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

/** * log_go_daemon() - tell logging subsystem that the process has been diff --git a/log.h b/log.h index a57c777..ed19415 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 errexit(const char *format, ...); void err(const char *format, ...); void warn(const char *format, ...); void info(const char *format, ...);

Other than that, this looks good to me.

LGTM. Personally I like to call such functions "die()".

I was about to suggest that (it's shorter and conveys the same meaning), then I thought die() would be a common library function. Actually, it's not -- so I would also favour die() here. -- Stefano

Laine Stump

8 Feb 8 Feb

6:48 p.m.

New subject: [PATCH v2 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 errexit() 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 | 339 +++++++++++++++++++++------------------------------------ 1 file changed, 123 insertions(+), 216 deletions(-) diff --git a/conf.c b/conf.c index c37552d..e5035f7 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) + errexit("--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)) + errexit("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) + errexit("--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) + errexit("--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) + errexit("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) + errexit("--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) + errexit("--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) + errexit("--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) + errexit("--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]); + errexit("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) + errexit("--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) + errexit("Multiple --trace options given"); - if (c->quiet) { - err("Either --trace or --quiet"); - usage(argv[0]); - } + if (c->quiet) + errexit("Either --trace or --quiet"); c->trace = c->debug = 1; break; case 12: - if (runas) { - err("Multiple --runas options given"); - usage(argv[0]); - } + if (runas) + errexit("Multiple --runas options given"); runas = optarg; break; case 13: - if (logsize) { - err("Multiple --log-size options given"); - usage(argv[0]); - } + if (logsize) + errexit("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) + errexit("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) + errexit("Multiple --debug options given"); - if (c->quiet) { - err("Either --debug or --quiet"); - usage(argv[0]); - } + if (c->quiet) + errexit("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) + errexit("Can't log to both file and stderr"); - if (c->stderr) { - err("Multiple --stderr options given"); - usage(argv[0]); - } + if (c->stderr) + errexit("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) + errexit("Can't log to both stderr and file"); - if (logfile) { - err("Multiple --log-file options given"); - usage(argv[0]); - } + if (logfile) + errexit("Multiple --log-file options given"); logfile = optarg; break; case 'q': - if (c->quiet) { - err("Multiple --quiet options given"); - usage(argv[0]); - } + if (c->quiet) + errexit("Multiple --quiet options given"); - if (c->debug) { - err("Either --debug or --quiet"); - usage(argv[0]); - } + if (c->debug) + errexit("Either --debug or --quiet"); c->quiet = 1; break; case 'f': - if (c->foreground) { - err("Multiple --foreground options given"); - usage(argv[0]); - } + if (c->foreground) + errexit("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) + errexit("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)) + errexit("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) + errexit("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) + errexit("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) + errexit("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) + errexit("Invalid interface name: %s", optarg); + break; case 'p': - if (*c->pcap) { - err("Multiple --pcap options given"); - usage(argv[0]); - } + if (*c->pcap) + errexit("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)) + errexit("Invalid pcap path: %s", optarg); + break; case 'P': - if (*c->pid_file) { - err("Multiple --pid options given"); - usage(argv[0]); - } + if (*c->pid_file) + errexit("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)) + errexit("Invalid PID file: %s", optarg); + break; case 'm': - if (c->mtu) { - err("Multiple --mtu options given"); - usage(argv[0]); - } + if (c->mtu) + errexit("Multiple --mtu options given"); errno = 0; c->mtu = strtol(optarg, NULL, 0); @@ -1427,11 +1368,9 @@ void conf(struct ctx *c, int argc, char **argv) break; } - if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || - errno) { - err("Invalid MTU: %s", optarg); - usage(argv[0]); - } + if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || errno) + errexit("Invalid MTU: %s", optarg); + break; case 'a': if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr) && @@ -1451,24 +1390,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]); + errexit("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) + errexit("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) + errexit("Invalid MAC address: %s", optarg); + c->mac[i] = b; } break; @@ -1486,41 +1422,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]); + errexit("Invalid gateway address: %s", optarg); break; case 'i': - if (ifi) { - err("Redundant interface: %s", optarg); - usage(argv[0]); - } + if (ifi) + errexit("Redundant interface: %s", optarg); - if (!(ifi = if_nametoindex(optarg))) { - err("Invalid interface name %s: %s", optarg, - strerror(errno)); - usage(argv[0]); - } + if (!(ifi = if_nametoindex(optarg))) + errexit("Invalid interface name %s: %s", optarg, + strerror(errno)); break; case 'D': if (!strcmp(optarg, "none")) { - if (c->no_dns) { - err("Redundant DNS options"); - usage(argv[0]); - } + if (c->no_dns) + errexit("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) + errexit("Conflicting DNS options"); c->no_dns = 1; break; } - if (c->no_dns) { - err("Conflicting DNS options"); - usage(argv[0]); - } + if (c->no_dns) + errexit("Conflicting DNS options"); if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) && inet_pton(AF_INET, optarg, dns4)) { @@ -1534,29 +1459,22 @@ void conf(struct ctx *c, int argc, char **argv) break; } - err("Cannot use DNS address %s", optarg); - usage(argv[0]); + errexit("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) + errexit("Redundant DNS search options"); - if (dnss != c->dns_search) { - err("Conflicting DNS search options"); - usage(argv[0]); - } + if (dnss != c->dns_search) + errexit("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) + errexit("Conflicting DNS search options"); if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) { ret = snprintf(dnss->n, sizeof(*c->dns_search), @@ -1568,8 +1486,7 @@ void conf(struct ctx *c, int argc, char **argv) break; } - err("Cannot use DNS search domain %s", optarg); - usage(argv[0]); + errexit("Cannot use DNS search domain %s", optarg); break; case '4': v4_only = true; @@ -1578,15 +1495,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) + errexit("--one-off is for passt mode only"); - if (c->one_off) { - err("Redundant --one-off option"); - usage(argv[0]); - } + if (c->one_off) + errexit("Redundant --one-off option"); c->one_off = true; break; @@ -1604,15 +1517,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) + errexit("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) + errexit("Options --socket and --fd are mutually exclusive"); ret = conf_ugid(runas, &uid, &gid); if (ret) @@ -1628,10 +1537,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) + errexit("External interface not usable"); /* Inbound port options can be parsed now (after IPv4/IPv6 settings) */ optind = 1; -- 2.39.1

Stefano Brivio

9 Feb 9 Feb

6:45 p.m.

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

On Wed, 8 Feb 2023 12:48:32 -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 errexit() instead. A few other usage() calls remain, but their removal involves bit more nuance that should be properly explained in separate commit messages.

This looks good to me, just a few nits:

...

Signed-off-by: Laine Stump --- conf.c | 339 +++++++++++++++++++++------------------------------------ 1 file changed, 123 insertions(+), 216 deletions(-)

diff --git a/conf.c b/conf.c index c37552d..e5035f7 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) + errexit("--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)) + errexit("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) + errexit("--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) + errexit("--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) + errexit("Invalid MAC address: %s", optarg);

You should split the line before '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) + errexit("--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) + errexit("--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) + errexit("--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) + errexit("--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]); + errexit("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) + errexit("--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) + errexit("Multiple --trace options given");

- if (c->quiet) { - err("Either --trace or --quiet"); - usage(argv[0]); - } + if (c->quiet) + errexit("Either --trace or --quiet");

c->trace = c->debug = 1; break; case 12: - if (runas) { - err("Multiple --runas options given"); - usage(argv[0]); - } + if (runas) + errexit("Multiple --runas options given");

runas = optarg; break; case 13: - if (logsize) { - err("Multiple --log-size options given"); - usage(argv[0]); - } + if (logsize) + errexit("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) + errexit("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) + errexit("Multiple --debug options given");

- if (c->quiet) { - err("Either --debug or --quiet"); - usage(argv[0]); - } + if (c->quiet) + errexit("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) + errexit("Can't log to both file and stderr");

- if (c->stderr) { - err("Multiple --stderr options given"); - usage(argv[0]); - } + if (c->stderr) + errexit("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) + errexit("Can't log to both stderr and file");

- if (logfile) { - err("Multiple --log-file options given"); - usage(argv[0]); - } + if (logfile) + errexit("Multiple --log-file options given");

logfile = optarg; break; case 'q': - if (c->quiet) { - err("Multiple --quiet options given"); - usage(argv[0]); - } + if (c->quiet) + errexit("Multiple --quiet options given");

- if (c->debug) { - err("Either --debug or --quiet"); - usage(argv[0]); - } + if (c->debug) + errexit("Either --debug or --quiet");

c->quiet = 1; break; case 'f': - if (c->foreground) { - err("Multiple --foreground options given"); - usage(argv[0]); - } + if (c->foreground) + errexit("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) + errexit("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)) + errexit("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) + errexit("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) + errexit("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) + errexit("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) + errexit("Invalid interface name: %s", optarg); + break; case 'p': - if (*c->pcap) { - err("Multiple --pcap options given"); - usage(argv[0]); - } + if (*c->pcap) + errexit("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)) + errexit("Invalid pcap path: %s", optarg); + break; case 'P': - if (*c->pid_file) { - err("Multiple --pid options given"); - usage(argv[0]); - } + if (*c->pid_file) + errexit("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)) + errexit("Invalid PID file: %s", optarg); + break; case 'm': - if (c->mtu) { - err("Multiple --mtu options given"); - usage(argv[0]); - } + if (c->mtu) + errexit("Multiple --mtu options given");

errno = 0; c->mtu = strtol(optarg, NULL, 0); @@ -1427,11 +1368,9 @@ void conf(struct ctx *c, int argc, char **argv) break; }

- if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || - errno) { - err("Invalid MTU: %s", optarg); - usage(argv[0]); - } + if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || errno)

This line should still be split like it was.

...

+ errexit("Invalid MTU: %s", optarg); + break; case 'a': if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr) && @@ -1451,24 +1390,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]); + errexit("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) + errexit("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) + errexit("Invalid MAC address: %s", optarg);

Split before 'optarg'.

...

+ c->mac[i] = b; } break; @@ -1486,41 +1422,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]); + errexit("Invalid gateway address: %s", optarg); break; case 'i': - if (ifi) { - err("Redundant interface: %s", optarg); - usage(argv[0]); - } + if (ifi) + errexit("Redundant interface: %s", optarg);

- if (!(ifi = if_nametoindex(optarg))) { - err("Invalid interface name %s: %s", optarg, - strerror(errno)); - usage(argv[0]); - } + if (!(ifi = if_nametoindex(optarg))) + errexit("Invalid interface name %s: %s", optarg, + strerror(errno)); break; case 'D': if (!strcmp(optarg, "none")) { - if (c->no_dns) { - err("Redundant DNS options"); - usage(argv[0]); - } + if (c->no_dns) + errexit("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) + errexit("Conflicting DNS options");

c->no_dns = 1; break; }

- if (c->no_dns) { - err("Conflicting DNS options"); - usage(argv[0]); - } + if (c->no_dns) + errexit("Conflicting DNS options");

if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) && inet_pton(AF_INET, optarg, dns4)) { @@ -1534,29 +1459,22 @@ void conf(struct ctx *c, int argc, char **argv) break; }

- err("Cannot use DNS address %s", optarg); - usage(argv[0]); + errexit("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) + errexit("Redundant DNS search options");

- if (dnss != c->dns_search) { - err("Conflicting DNS search options"); - usage(argv[0]); - } + if (dnss != c->dns_search) + errexit("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) + errexit("Conflicting DNS search options");

if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) { ret = snprintf(dnss->n, sizeof(*c->dns_search), @@ -1568,8 +1486,7 @@ void conf(struct ctx *c, int argc, char **argv) break; }

- err("Cannot use DNS search domain %s", optarg); - usage(argv[0]); + errexit("Cannot use DNS search domain %s", optarg); break; case '4': v4_only = true; @@ -1578,15 +1495,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) + errexit("--one-off is for passt mode only");

- if (c->one_off) { - err("Redundant --one-off option"); - usage(argv[0]); - } + if (c->one_off) + errexit("Redundant --one-off option");

c->one_off = true; break; @@ -1604,15 +1517,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) + errexit("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) + errexit("Options --socket and --fd are mutually exclusive");

ret = conf_ugid(runas, &uid, &gid); if (ret) @@ -1628,10 +1537,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) + errexit("External interface not usable");

/* Inbound port options can be parsed now (after IPv4/IPv6 settings) */ optind = 1;

-- Stefano

Laine Stump

8 Feb 8 Feb

6:48 p.m.

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

Rather than having conf_ports() (possibly) log an error, and then let the caller log the entire usage() message and exit, just log the error and exit immediately. 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 errexit() (logging a specific error), thus permitting us to make conf_ports() return void, which simplifies the caller. We can further simplify the two callers to conf_ports by moving the check for a missing argument to the port options into conf_ports itself, and make it more useful by again logging an informative error for missing option instead of the generic usage() message. Signed-off-by: Laine Stump --- conf.c | 55 +++++++++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/conf.c b/conf.c index e5035f7..799b9ff 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; @@ -185,25 +183,37 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, sa_family_t af = AF_UNSPEC; bool exclude_only = true; + if (!optarg) + errexit("missing argument to -%s option", optname); + 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) + errexit("'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) + errexit("'all' port forwarding is only allowed for passt"); + fwd->mode = FWD_ALL; memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8); @@ -214,11 +224,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; + errexit("specific ports cannot be specified together with all/none/auto"); fwd->mode = FWD_SPEC; @@ -292,7 +302,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 +349,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; - + errexit("Invalid port specifier %s", optarg); overlap: - err("Overlapping port specifier %s", optarg); - return -EINVAL; + errexit("Overlapping port specifier %s", optarg); +mode_conflict: + errexit("Port forwarding mode '%s' conflicts with previous mode", optarg); } /** @@ -1549,8 +1558,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); @@ -1588,8 +1596,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

Stefano Brivio

9 Feb 9 Feb

6:45 p.m.

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

On Wed, 8 Feb 2023 12:48:33 -0500 Laine Stump wrote:

...

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

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 errexit() (logging a specific error), thus permitting us to make conf_ports() return void, which simplifies the caller.

We can further simplify the two callers to conf_ports by moving the check for a missing argument to the port options into conf_ports itself, and make it more useful by again logging an informative error for missing option instead of the generic usage() message.

Signed-off-by: Laine Stump --- conf.c | 55 +++++++++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/conf.c b/conf.c index e5035f7..799b9ff 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; @@ -185,25 +183,37 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, sa_family_t af = AF_UNSPEC; bool exclude_only = true;

+ if (!optarg) + errexit("missing argument to -%s option", optname);

This can't happen, as all the options relevant to this function are passed to getopt_long() with 'required_argument'. I see there's a check on !optarg in the caller, but I think you can skip it.

...

+ 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) + errexit("'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) + errexit("'all' port forwarding is only allowed for passt"); + fwd->mode = FWD_ALL; memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);

@@ -214,11 +224,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; + errexit("specific ports cannot be specified together with all/none/auto");

s/specific/Specific/, for consistency.

...

fwd->mode = FWD_SPEC;

@@ -292,7 +302,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 +349,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; - + errexit("Invalid port specifier %s", optarg); overlap: - err("Overlapping port specifier %s", optarg); - return -EINVAL; + errexit("Overlapping port specifier %s", optarg); +mode_conflict: + errexit("Port forwarding mode '%s' conflicts with previous mode", optarg);

Split before 'optarg'.

...

}

/** @@ -1549,8 +1558,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);

@@ -1588,8 +1596,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);

-- Stefano

Laine Stump

8 Feb 8 Feb

6:48 p.m.

New subject: [PATCH v2 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 | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/conf.c b/conf.c index 799b9ff..1e9c6f6 100644 --- a/conf.c +++ b/conf.c @@ -500,21 +500,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) + errexit("Both --userns and --netns-only given"); - if (*netns && optind != argc) { - err("Both --netns and PID or command given"); - return -EINVAL; - } + if (*netns && optind != argc) + errexit("Both --netns and PID or command given"); if (optind + 1 == argc) { char *endptr; @@ -523,23 +517,19 @@ 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) + errexit("Invalid PID %s", argv[optind]); snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval); if (!*userns) snprintf(userns, PATH_MAX, "/proc/%ld/ns/user", - pidval); + pidval); } } /* 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 @@ -1562,13 +1552,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

Stefano Brivio

9 Feb 9 Feb

6:45 p.m.

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

On Wed, 8 Feb 2023 12:48:34 -0500 Laine Stump wrote:

...

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

Signed-off-by: Laine Stump --- conf.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/conf.c b/conf.c index 799b9ff..1e9c6f6 100644 --- a/conf.c +++ b/conf.c @@ -500,21 +500,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) + errexit("Both --userns and --netns-only given");

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

if (optind + 1 == argc) { char *endptr; @@ -523,23 +517,19 @@ 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) + errexit("Invalid PID %s", argv[optind]);

snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval); if (!*userns) snprintf(userns, PATH_MAX, "/proc/%ld/ns/user", - pidval); + pidval);

Stray change.

...

} }

/* 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 @@ -1562,13 +1552,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);

-- Stefano

Laine Stump

8 Feb 8 Feb

6:48 p.m.

New subject: [PATCH v2 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 | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/conf.c b/conf.c index 1e9c6f6..5e9a6f9 100644 --- a/conf.c +++ b/conf.c @@ -998,10 +998,8 @@ 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]; @@ -1012,8 +1010,7 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) if (runas) { ret = conf_runas(runas, uid, gid); if (ret) - err("Invalid --runas option: %s", runas); - return ret; + errexit("Invalid --runas option: %s", runas); } /* ...otherwise default to current user and group... */ @@ -1022,20 +1019,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; + errexit("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); @@ -1059,7 +1054,6 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) *uid = *gid = 65534; #endif } - return 0; } /** @@ -1522,9 +1516,7 @@ void conf(struct ctx *c, int argc, char **argv) if (*c->sock_path && c->fd_tap >= 0) errexit("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

Laine Stump

13 Feb 13 Feb

5:23 a.m.

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

On 2/8/23 12:48 PM, Laine Stump wrote:

...

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

Signed-off-by: Laine Stump --- conf.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/conf.c b/conf.c index 1e9c6f6..5e9a6f9 100644 --- a/conf.c +++ b/conf.c @@ -998,10 +998,8 @@ 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]; @@ -1012,8 +1010,7 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) if (runas) { ret = conf_runas(runas, uid, gid); if (ret) - err("Invalid --runas option: %s", runas); - return ret; + errexit("Invalid --runas option: %s", runas);

Noticed while reviewing my own patches in email - I was moving too quick/tired and counted the err() inside the "if (ret)" together with the subsequent return, when the return is actually outside of "if (ret)". I'll fix that up before reposting (along with your other suggestions).

...

}

/* ...otherwise default to current user and group... */ @@ -1022,20 +1019,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; + errexit("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); @@ -1059,7 +1054,6 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) *uid = *gid = 65534; #endif } - return 0; }

/** @@ -1522,9 +1516,7 @@ void conf(struct ctx *c, int argc, char **argv) if (*c->sock_path && c->fd_tap >= 0) errexit("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",

Laine Stump

8 Feb 8 Feb

6:48 p.m.

New subject: [PATCH v2 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 5e9a6f9..1bfbc55 100644 --- a/conf.c +++ b/conf.c @@ -467,10 +467,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; @@ -482,12 +480,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) + errexit("Network namespace name/path %s too long"); } /** @@ -1161,9 +1155,7 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode != MODE_PASTA) errexit("--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

Laine Stump

6:48 p.m.

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

Telling the user "this bit is wrong" is more useful than telling them "these are all the potential things that would be right; actual error identification is left as an exercise for the reader". Signed-off-by: Laine Stump --- conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conf.c b/conf.c index 1bfbc55..ce4e3e5 100644 --- a/conf.c +++ b/conf.c @@ -1539,7 +1539,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]); + errexit("Extra non-option argument: %s", argv[optind]); isolate_user(uid, gid, !netns_only, userns, c->mode); -- 2.39.1

Stefano Brivio

9 Feb 9 Feb

6:45 p.m.

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

On Wed, 8 Feb 2023 12:48:37 -0500 Laine Stump wrote:

...

Telling the user "this bit is wrong" is more useful than telling them "these are all the potential things that would be right; actual error identification is left as an exercise for the reader".

Ah, yes, I guess. :)

...

Signed-off-by: Laine Stump --- conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conf.c b/conf.c index 1bfbc55..ce4e3e5 100644 --- a/conf.c +++ b/conf.c @@ -1539,7 +1539,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]); + errexit("Extra non-option argument: %s", argv[optind]);

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

-- Stefano

Laine Stump

8 Feb 8 Feb

6:48 p.m.

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

This actually leaves us with 0 uses of err(), but someone could want to use it in the future, so may as well leave it around. Signed-off-by: Laine Stump --- isolation.c | 78 +++++++++++++++++++---------------------------------- log.c | 6 ++--- netlink.c | 3 +-- passt.c | 12 +++------ pasta.c | 21 ++++++--------- tap.c | 30 +++++++-------------- 6 files changed, 53 insertions(+), 97 deletions(-) diff --git a/isolation.c b/isolation.c index 4e6637d..0f709c6 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)) + errexit("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)) + errexit("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", - i, strerror(errno)); - exit(EXIT_FAILURE); - } + errno != EINVAL && errno != EPERM) + errexit("Couldn't drop cap %i from bounding set: %s", + i, strerror(errno)); } - if (syscall(SYS_capget, &hdr, data)) { - err("Couldn't get current capabilities: %s", strerror(errno)); - exit(EXIT_FAILURE); - } + if (syscall(SYS_capget, &hdr, data)) + errexit("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", - strerror(errno)); - exit(EXIT_FAILURE); - } + if (syscall(SYS_capset, &hdr, data)) + errexit("Couldn't drop inheritable capabilities: %s", + strerror(errno)); } /** @@ -229,46 +219,34 @@ 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", - strerror(errno)); - exit(EXIT_FAILURE); - } + if (errno != EPERM) + errexit("Can't drop supplementary groups: %s", + strerror(errno)); } - if (setgid(gid) != 0) { - err("Can't set GID to %u: %s", gid, strerror(errno)); - exit(EXIT_FAILURE); - } + if (setgid(gid) != 0) + errexit("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) + errexit("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", - userns, strerror(errno)); - exit(EXIT_FAILURE); - } - - if (setns(ufd, CLONE_NEWUSER) != 0) { - err("Couldn't enter user namespace %s: %s", - userns, strerror(errno)); - exit(EXIT_FAILURE); - } + if (ufd < 0) + errexit("Couldn't open user namespace %s: %s", + userns, strerror(errno)); + + if (setns(ufd, CLONE_NEWUSER) != 0) + errexit("Couldn't enter user namespace %s: %s", + userns, strerror(errno)); 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) + errexit("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 4956914..983c82f 100644 --- a/log.c +++ b/log.c @@ -204,10 +204,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) + errexit("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 0850cbe..a6c6e1e 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); + errexit("Failed to get netlink socket"); } /** diff --git a/passt.c b/passt.c index cf010e8..443d51a 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)) + errexit("Couldn't install signal handlers"); c.mode = MODE_PASTA; log_name = "pasta"; @@ -291,10 +289,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)) + errexit("Failed to sandbox process, exiting\n"); if (!c.foreground) { __daemon(pidfile_fd, devnull_fd); diff --git a/pasta.c b/pasta.c index 528f02a..b1463c9 100644 --- a/pasta.c +++ b/pasta.c @@ -123,19 +123,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) + errexit("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) + errexit("Couldn't switch to pasta namespaces"); if (!c->no_netns_quit) { char buf[PATH_MAX] = { 0 }; @@ -217,11 +213,10 @@ 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)) + errexit("$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 2e603ed..edb3184 100644 --- a/tap.c +++ b/tap.c @@ -886,10 +886,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) + errexit("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 @@ -907,18 +905,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) + errexit("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) + errexit("Socket path %s already in use", path); close(ex); continue; @@ -931,10 +925,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) + errexit("UNIX socket bind: %s", strerror(errno)); info("UNIX domain socket bound at %s\n", addr.sun_path); @@ -1037,10 +1029,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) + errexit("Failed to open tun socket in namespace"); pasta_ns_conf(c); -- 2.39.1

Stefano Brivio

9 Feb 9 Feb

6:45 p.m.

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

On Wed, 8 Feb 2023 12:48:38 -0500 Laine Stump wrote:

...

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

Signed-off-by: Laine Stump --- isolation.c | 78 +++++++++++++++++++---------------------------------- log.c | 6 ++--- netlink.c | 3 +-- passt.c | 12 +++------ pasta.c | 21 ++++++--------- tap.c | 30 +++++++-------------- 6 files changed, 53 insertions(+), 97 deletions(-)

diff --git a/isolation.c b/isolation.c index 4e6637d..0f709c6 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)) + errexit("Couldn't get current capabilities: %s", strerror(errno));

Split before strerror().

...

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)) + errexit("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", - i, strerror(errno)); - exit(EXIT_FAILURE); - } + errno != EINVAL && errno != EPERM) + errexit("Couldn't drop cap %i from bounding set: %s", + i, strerror(errno)); }

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

Same here.

...

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

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

/** @@ -229,46 +219,34 @@ 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", - strerror(errno)); - exit(EXIT_FAILURE); - } + if (errno != EPERM) + errexit("Can't drop supplementary groups: %s", + strerror(errno)); }

- if (setgid(gid) != 0) { - err("Can't set GID to %u: %s", gid, strerror(errno)); - exit(EXIT_FAILURE); - } + if (setgid(gid) != 0) + errexit("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) + errexit("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", - userns, strerror(errno)); - exit(EXIT_FAILURE); - } - - if (setns(ufd, CLONE_NEWUSER) != 0) { - err("Couldn't enter user namespace %s: %s", - userns, strerror(errno)); - exit(EXIT_FAILURE); - } + if (ufd < 0) + errexit("Couldn't open user namespace %s: %s", + userns, strerror(errno)); + + if (setns(ufd, CLONE_NEWUSER) != 0) + errexit("Couldn't enter user namespace %s: %s", + userns, strerror(errno));

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) + errexit("Couldn't create user namespace: %s", strerror(errno));

And here.

...

}

/* Joining a new userns gives us full capabilities; drop the diff --git a/log.c b/log.c index 4956914..983c82f 100644 --- a/log.c +++ b/log.c @@ -204,10 +204,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) + errexit("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 0850cbe..a6c6e1e 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); + errexit("Failed to get netlink socket"); }

/** diff --git a/passt.c b/passt.c index cf010e8..443d51a 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)) + errexit("Couldn't install signal handlers");

c.mode = MODE_PASTA; log_name = "pasta"; @@ -291,10 +289,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)) + errexit("Failed to sandbox process, exiting\n");

if (!c.foreground) { __daemon(pidfile_fd, devnull_fd); diff --git a/pasta.c b/pasta.c index 528f02a..b1463c9 100644 --- a/pasta.c +++ b/pasta.c @@ -123,19 +123,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) + errexit("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) + errexit("Couldn't switch to pasta namespaces");

if (!c->no_netns_quit) { char buf[PATH_MAX] = { 0 }; @@ -217,11 +213,10 @@ 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)) + errexit("$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 2e603ed..edb3184 100644 --- a/tap.c +++ b/tap.c @@ -886,10 +886,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) + errexit("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 @@ -907,18 +905,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) + errexit("UNIX domain socket check: %s", strerror(errno));

And here.

...

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) + errexit("Socket path %s already in use", path);

close(ex); continue; @@ -931,10 +925,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) + errexit("UNIX socket bind: %s", strerror(errno));

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

@@ -1037,10 +1029,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) + errexit("Failed to open tun socket in namespace");

pasta_ns_conf(c);

-- Stefano

831

Age (days ago)

837

Last active (days ago)

List overview

Download

20 comments

4 participants

participants (4)

David Gibson
Laine Stump
Laine Stump
Stefano Brivio