* [PATCH v4 0/9] error logging fixes
@ 2023-02-15 8:24 Laine Stump
2023-02-15 8:24 ` [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set Laine Stump
` (10 more replies)
0 siblings, 11 replies; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-15 11:54 ` Stefano Brivio
2023-02-16 5:30 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 2/9] add die() to log an error message and exit with a single call Laine Stump
` (9 subsequent siblings)
10 siblings, 2 replies; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
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 <laine@redhat.com>
---
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
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 2/9] add die() to log an error message and exit with a single call
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
2023-02-15 8:24 ` [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-16 5:31 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 3/9] eliminate most calls to usage() in conf() Laine Stump
` (8 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
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 <laine@redhat.com>
---
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, ...);
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 3/9] eliminate most calls to usage() in conf()
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
2023-02-15 8:24 ` [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set Laine Stump
2023-02-15 8:24 ` [PATCH v4 2/9] add die() to log an error message and exit with a single call Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-16 5:34 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 4/9] make conf_ports() exit immediately after logging error Laine Stump
` (7 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
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 <laine@redhat.com>
---
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;
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 4/9] make conf_ports() exit immediately after logging error
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
` (2 preceding siblings ...)
2023-02-15 8:24 ` [PATCH v4 3/9] eliminate most calls to usage() in conf() Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-16 5:36 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 5/9] make conf_pasta_ns() " Laine Stump
` (6 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
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 <laine@redhat.com>
---
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);
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 5/9] make conf_pasta_ns() exit immediately after logging error
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
` (3 preceding siblings ...)
2023-02-15 8:24 ` [PATCH v4 4/9] make conf_ports() exit immediately after logging error Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-16 5:37 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 6/9] make conf_ugid() " Laine Stump
` (5 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
As with conf_ports, this allows us to make the function return void.
Signed-off-by: Laine Stump <laine@redhat.com>
---
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);
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 6/9] make conf_ugid() exit immediately after logging error
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
` (4 preceding siblings ...)
2023-02-15 8:24 ` [PATCH v4 5/9] make conf_pasta_ns() " Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-16 5:38 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 7/9] make conf_netns_opt() " Laine Stump
` (4 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
Again, it can then be made to return void, simplifying the caller.
Signed-off-by: Laine Stump <laine@redhat.com>
---
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",
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 7/9] make conf_netns_opt() exit immediately after logging error
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
` (5 preceding siblings ...)
2023-02-15 8:24 ` [PATCH v4 6/9] make conf_ugid() " Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-16 5:38 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 8/9] log a detailed error (not usage()) when there are extra non-option arguments Laine Stump
` (3 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
...and return void to simplify the caller.
Signed-off-by: Laine Stump <laine@redhat.com>
---
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)
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 8/9] log a detailed error (not usage()) when there are extra non-option arguments
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
` (6 preceding siblings ...)
2023-02-15 8:24 ` [PATCH v4 7/9] make conf_netns_opt() " Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-16 5:39 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 9/9] convert all remaining err() followed by exit() to die() Laine Stump
` (2 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
Signed-off-by: Laine Stump <laine@redhat.com>
---
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);
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 9/9] convert all remaining err() followed by exit() to die()
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
` (7 preceding siblings ...)
2023-02-15 8:24 ` [PATCH v4 8/9] log a detailed error (not usage()) when there are extra non-option arguments Laine Stump
@ 2023-02-15 8:24 ` Laine Stump
2023-02-16 5:40 ` David Gibson
2023-02-15 11:56 ` [PATCH v4 0/9] error logging fixes Stefano Brivio
2023-02-16 22:21 ` Stefano Brivio
10 siblings, 1 reply; 23+ messages in thread
From: Laine Stump @ 2023-02-15 8:24 UTC (permalink / raw)
To: passt-dev
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 <laine@redhat.com>
---
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);
--
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set
2023-02-15 8:24 ` [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set Laine Stump
@ 2023-02-15 11:54 ` Stefano Brivio
2023-02-16 5:30 ` David Gibson
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Brivio @ 2023-02-15 11:54 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
On Wed, 15 Feb 2023 03:24:29 -0500
Laine Stump <laine@redhat.com> 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 <laine@redhat.com>
> ---
> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 0/9] error logging fixes
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
` (8 preceding siblings ...)
2023-02-15 8:24 ` [PATCH v4 9/9] convert all remaining err() followed by exit() to die() Laine Stump
@ 2023-02-15 11:56 ` Stefano Brivio
2023-02-16 22:21 ` Stefano Brivio
10 siblings, 0 replies; 23+ messages in thread
From: Stefano Brivio @ 2023-02-15 11:56 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
On Wed, 15 Feb 2023 03:24:28 -0500
Laine Stump <laine@redhat.com> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set
2023-02-15 8:24 ` [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set Laine Stump
2023-02-15 11:54 ` Stefano Brivio
@ 2023-02-16 5:30 ` David Gibson
2023-02-16 17:50 ` Stefano Brivio
1 sibling, 1 reply; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:30 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3353 bytes --]
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 <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/9] add die() to log an error message and exit with a single call
2023-02-15 8:24 ` [PATCH v4 2/9] add die() to log an error message and exit with a single call Laine Stump
@ 2023-02-16 5:31 ` David Gibson
0 siblings, 0 replies; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:31 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2557 bytes --]
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 <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/9] eliminate most calls to usage() in conf()
2023-02-15 8:24 ` [PATCH v4 3/9] eliminate most calls to usage() in conf() Laine Stump
@ 2023-02-16 5:34 ` David Gibson
0 siblings, 0 replies; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:34 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]
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 <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 4/9] make conf_ports() exit immediately after logging error
2023-02-15 8:24 ` [PATCH v4 4/9] make conf_ports() exit immediately after logging error Laine Stump
@ 2023-02-16 5:36 ` David Gibson
0 siblings, 0 replies; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:36 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 5015 bytes --]
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 <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 5/9] make conf_pasta_ns() exit immediately after logging error
2023-02-15 8:24 ` [PATCH v4 5/9] make conf_pasta_ns() " Laine Stump
@ 2023-02-16 5:37 ` David Gibson
0 siblings, 0 replies; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:37 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2958 bytes --]
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 <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 6/9] make conf_ugid() exit immediately after logging error
2023-02-15 8:24 ` [PATCH v4 6/9] make conf_ugid() " Laine Stump
@ 2023-02-16 5:38 ` David Gibson
0 siblings, 0 replies; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:38 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]
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 <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 7/9] make conf_netns_opt() exit immediately after logging error
2023-02-15 8:24 ` [PATCH v4 7/9] make conf_netns_opt() " Laine Stump
@ 2023-02-16 5:38 ` David Gibson
0 siblings, 0 replies; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:38 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]
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 <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 8/9] log a detailed error (not usage()) when there are extra non-option arguments
2023-02-15 8:24 ` [PATCH v4 8/9] log a detailed error (not usage()) when there are extra non-option arguments Laine Stump
@ 2023-02-16 5:39 ` David Gibson
0 siblings, 0 replies; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:39 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 897 bytes --]
On Wed, Feb 15, 2023 at 03:24:36AM -0500, Laine Stump wrote:
> Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 9/9] convert all remaining err() followed by exit() to die()
2023-02-15 8:24 ` [PATCH v4 9/9] convert all remaining err() followed by exit() to die() Laine Stump
@ 2023-02-16 5:40 ` David Gibson
0 siblings, 0 replies; 23+ messages in thread
From: David Gibson @ 2023-02-16 5:40 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 9992 bytes --]
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 <laine@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set
2023-02-16 5:30 ` David Gibson
@ 2023-02-16 17:50 ` Stefano Brivio
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Brivio @ 2023-02-16 17:50 UTC (permalink / raw)
To: David Gibson; +Cc: Laine Stump, passt-dev
On Thu, 16 Feb 2023 16:30:53 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <laine@redhat.com>
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Sorry David, I missed to add *all* your Reviewed-by tags on this
series. :/ Thanks a lot for reviewing all this.
--
Stefano
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 0/9] error logging fixes
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
` (9 preceding siblings ...)
2023-02-15 11:56 ` [PATCH v4 0/9] error logging fixes Stefano Brivio
@ 2023-02-16 22:21 ` Stefano Brivio
10 siblings, 0 replies; 23+ messages in thread
From: Stefano Brivio @ 2023-02-16 22:21 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev
On Wed, 15 Feb 2023 03:24:28 -0500
Laine Stump <laine@redhat.com> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2023-02-16 22:22 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-15 8:24 [PATCH v4 0/9] error logging fixes Laine Stump
2023-02-15 8:24 ` [PATCH v4 1/9] log to stderr until process is daemonized, even if a log file is set Laine Stump
2023-02-15 11:54 ` Stefano Brivio
2023-02-16 5:30 ` David Gibson
2023-02-16 17:50 ` Stefano Brivio
2023-02-15 8:24 ` [PATCH v4 2/9] add die() to log an error message and exit with a single call Laine Stump
2023-02-16 5:31 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 3/9] eliminate most calls to usage() in conf() Laine Stump
2023-02-16 5:34 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 4/9] make conf_ports() exit immediately after logging error Laine Stump
2023-02-16 5:36 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 5/9] make conf_pasta_ns() " Laine Stump
2023-02-16 5:37 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 6/9] make conf_ugid() " Laine Stump
2023-02-16 5:38 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 7/9] make conf_netns_opt() " Laine Stump
2023-02-16 5:38 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 8/9] log a detailed error (not usage()) when there are extra non-option arguments Laine Stump
2023-02-16 5:39 ` David Gibson
2023-02-15 8:24 ` [PATCH v4 9/9] convert all remaining err() followed by exit() to die() Laine Stump
2023-02-16 5:40 ` David Gibson
2023-02-15 11:56 ` [PATCH v4 0/9] error logging fixes Stefano Brivio
2023-02-16 22:21 ` Stefano Brivio
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).