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. 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 eliminate80 characters (I did address all the rest of the things he pointedout, 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 logfile 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
Once a logfile (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 logmask 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 logmask to something else). This period
*almost* matches with "the time before the process is daemonized"; if
we just delay setting the logmask 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 logfile has been specified and opened.
This patch delays the setting of logmask until immediately before the
call to __daemon(). It also modifies logfn() slightly, so that it will
log to stderr any time logmask is LOG_EMERG, even if a logfile has
been opened.
Signed-off-by: Laine Stump <laine(a)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..1228e5b 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 logmask != LOG_EMERG, we will no longer
+ * log to stderr if there is a logfile 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
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(a)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, ...);
--
2.39.1
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(a)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;
--
2.39.1
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(a)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);
--
2.39.1
As with conf_ports, this allows us to make the function return void.
Signed-off-by: Laine Stump <laine(a)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);
--
2.39.1
Again, it can then be made to return void, simplifying the caller.
Signed-off-by: Laine Stump <laine(a)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",
--
2.39.1
...and return void to simplify the caller.
Signed-off-by: Laine Stump <laine(a)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)
--
2.39.1
Signed-off-by: Laine Stump <laine(a)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);
--
2.39.1
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(a)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 1228e5b..cd77cc1 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 logmask != LOG_EMERG, we will no longer
* log to stderr if there is a logfile specified
diff --git a/pasta.c b/pasta.c
index d4d3dc8..6c9a412 100644
--- a/pasta.c
+++ b/pasta.c
@@ -131,19 +131,15 @@ void pasta_open_ns(struct ctx *c, const char *netns)
int nfd = -1;
nfd = open(netns, O_RDONLY | O_CLOEXEC);
- if (nfd < 0) {
- err("Couldn't open network namespace %s", netns);
- exit(EXIT_FAILURE);
- }
+ if (nfd < 0)
+ die("Couldn't open network namespace %s", netns);
c->pasta_netns_fd = nfd;
NS_CALL(ns_check, c);
- if (c->pasta_netns_fd < 0) {
- err("Couldn't switch to pasta namespaces");
- exit(EXIT_FAILURE);
- }
+ if (c->pasta_netns_fd < 0)
+ die("Couldn't switch to pasta namespaces");
if (!c->no_netns_quit) {
char buf[PATH_MAX] = { 0 };
@@ -232,11 +228,9 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
arg.exe = "/bin/sh";
if ((size_t)snprintf(sh_arg0, sizeof(sh_arg0),
- "-%s", arg.exe) >= sizeof(sh_arg0)) {
- err("$SHELL is too long (%u bytes)",
- strlen(arg.exe));
- exit(EXIT_FAILURE);
- }
+ "-%s", arg.exe) >= sizeof(sh_arg0))
+ die("$SHELL is too long (%u bytes)", strlen(arg.exe));
+
sh_argv[0] = sh_arg0;
arg.argv = sh_argv;
}
diff --git a/tap.c b/tap.c
index 716d887..02da84d 100644
--- a/tap.c
+++ b/tap.c
@@ -1008,10 +1008,8 @@ static void tap_sock_unix_init(struct ctx *c)
};
int i;
- if (fd < 0) {
- err("UNIX socket: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (fd < 0)
+ die("UNIX socket: %s", strerror(errno));
/* In passt mode, we don't know the guest's MAC until it sends
* us packets. Use the broadcast address so our first packets
@@ -1029,18 +1027,14 @@ static void tap_sock_unix_init(struct ctx *c)
snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
- if (ex < 0) {
- err("UNIX domain socket check: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (ex < 0)
+ die("UNIX domain socket check: %s", strerror(errno));
ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
errno != EACCES)) {
- if (*c->sock_path) {
- err("Socket path %s already in use", path);
- exit(EXIT_FAILURE);
- }
+ if (*c->sock_path)
+ die("Socket path %s already in use", path);
close(ex);
continue;
@@ -1053,10 +1047,8 @@ static void tap_sock_unix_init(struct ctx *c)
break;
}
- if (i == UNIX_SOCK_MAX) {
- err("UNIX socket bind: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (i == UNIX_SOCK_MAX)
+ die("UNIX socket bind: %s", strerror(errno));
info("UNIX domain socket bound at %s\n", addr.sun_path);
@@ -1159,10 +1151,8 @@ static void tap_sock_tun_init(struct ctx *c)
struct epoll_event ev = { 0 };
NS_CALL(tap_ns_tun, c);
- if (tun_ns_fd == -1) {
- err("Failed to open tun socket in namespace");
- exit(EXIT_FAILURE);
- }
+ if (tun_ns_fd == -1)
+ die("Failed to open tun socket in namespace");
pasta_ns_conf(c);
--
2.39.1