From: Stefano Brivio <sbrivio@redhat.com>
To: Laine Stump <laine@redhat.com>
Cc: passt-dev@passt.top, laine@laine.org
Subject: Re: [PATCH v2 3/9] eliminate most calls to usage() in conf()
Date: Thu, 9 Feb 2023 18:45:36 +0100 [thread overview]
Message-ID: <20230209184536.571c4a28@elisabeth> (raw)
In-Reply-To: <20230208174838.1680517-4-laine@redhat.com>
On Wed, 8 Feb 2023 12:48:32 -0500
Laine Stump <laine@redhat.com> wrote:
> Nearly all of the calls to usage() in conf() occur immediately after
> logging a more detailed error message, and the fact that these errors
> are occuring indicates that the user has already seen the passt usage
> message (or read the manpage). Spamming the logfile with the complete
> contents of the usage message serves only to obscure the more detailed
> error message. The only time when the full usage message should be output
> is if the user explicitly asks for it with -h (or its synonyms)
>
> AS a start to eliminating the excessive calls to usage(), this patch
> replaces most calls to err() followed by usage() with a call to
> errexit() instead. A few other usage() calls remain, but their removal
> involves bit more nuance that should be properly explained in
> separate commit messages.
This looks good to me, just a few nits:
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> conf.c | 339 +++++++++++++++++++++------------------------------------
> 1 file changed, 123 insertions(+), 216 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index c37552d..e5035f7 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1156,69 +1156,57 @@ void conf(struct ctx *c, int argc, char **argv)
> case 0:
> break;
> case 2:
> - if (c->mode != MODE_PASTA) {
> - err("--userns is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--userns is for pasta mode only");
>
> ret = snprintf(userns, sizeof(userns), "%s", optarg);
> - if (ret <= 0 || ret >= (int)sizeof(userns)) {
> - err("Invalid userns: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= (int)sizeof(userns))
> + errexit("Invalid userns: %s", optarg);
> +
> break;
> case 3:
> - if (c->mode != MODE_PASTA) {
> - err("--netns is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--netns is for pasta mode only");
>
> ret = conf_netns_opt(netns, optarg);
> if (ret < 0)
> usage(argv[0]);
> break;
> case 4:
> - if (c->mode != MODE_PASTA) {
> - err("--ns-mac-addr is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--ns-mac-addr is for pasta mode only");
>
> for (i = 0; i < ETH_ALEN; i++) {
> errno = 0;
> b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
> - if (b < 0 || b > UCHAR_MAX || errno) {
> - err("Invalid MAC address: %s", optarg);
> - usage(argv[0]);
> - }
> + if (b < 0 || b > UCHAR_MAX || errno)
> + errexit("Invalid MAC address: %s", optarg);
You should split the line before 'optarg'.
> +
> c->mac_guest[i] = b;
> }
> break;
> case 5:
> - if (c->mode != MODE_PASTA) {
> - err("--dhcp-dns is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--dhcp-dns is for pasta mode only");
> +
> c->no_dhcp_dns = 0;
> break;
> case 6:
> - if (c->mode != MODE_PASST) {
> - err("--no-dhcp-dns is for passt mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASST)
> + errexit("--no-dhcp-dns is for passt mode only");
> +
> c->no_dhcp_dns = 1;
> break;
> case 7:
> - if (c->mode != MODE_PASTA) {
> - err("--dhcp-search is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--dhcp-search is for pasta mode only");
> +
> c->no_dhcp_dns_search = 0;
> break;
> case 8:
> - if (c->mode != MODE_PASST) {
> - err("--no-dhcp-search is for passt mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASST)
> + errexit("--no-dhcp-search is for passt mode only");
> +
> c->no_dhcp_dns_search = 1;
> break;
> case 9:
> @@ -1235,50 +1223,39 @@ void conf(struct ctx *c, int argc, char **argv)
> !IN4_IS_ADDR_LOOPBACK(&c->ip4.dns_match))
> break;
>
> - err("Invalid DNS forwarding address: %s", optarg);
> - usage(argv[0]);
> + errexit("Invalid DNS forwarding address: %s", optarg);
> break;
> case 10:
> - if (c->mode != MODE_PASTA) {
> - err("--no-netns-quit is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--no-netns-quit is for pasta mode only");
> +
> c->no_netns_quit = 1;
> break;
> case 11:
> - if (c->trace) {
> - err("Multiple --trace options given");
> - usage(argv[0]);
> - }
> + if (c->trace)
> + errexit("Multiple --trace options given");
>
> - if (c->quiet) {
> - err("Either --trace or --quiet");
> - usage(argv[0]);
> - }
> + if (c->quiet)
> + errexit("Either --trace or --quiet");
>
> c->trace = c->debug = 1;
> break;
> case 12:
> - if (runas) {
> - err("Multiple --runas options given");
> - usage(argv[0]);
> - }
> + if (runas)
> + errexit("Multiple --runas options given");
>
> runas = optarg;
> break;
> case 13:
> - if (logsize) {
> - err("Multiple --log-size options given");
> - usage(argv[0]);
> - }
> + if (logsize)
> + errexit("Multiple --log-size options given");
>
> errno = 0;
> logsize = strtol(optarg, NULL, 0);
>
> - if (logsize < LOGFILE_SIZE_MIN || errno) {
> - err("Invalid --log-size: %s", optarg);
> - usage(argv[0]);
> - }
> + if (logsize < LOGFILE_SIZE_MIN || errno)
> + errexit("Invalid --log-size: %s", optarg);
> +
> break;
> case 14:
> fprintf(stdout,
> @@ -1286,138 +1263,102 @@ void conf(struct ctx *c, int argc, char **argv)
> fprintf(stdout, VERSION_BLOB);
> exit(EXIT_SUCCESS);
> case 'd':
> - if (c->debug) {
> - err("Multiple --debug options given");
> - usage(argv[0]);
> - }
> + if (c->debug)
> + errexit("Multiple --debug options given");
>
> - if (c->quiet) {
> - err("Either --debug or --quiet");
> - usage(argv[0]);
> - }
> + if (c->quiet)
> + errexit("Either --debug or --quiet");
>
> c->debug = 1;
> break;
> case 'e':
> - if (logfile) {
> - err("Can't log to both file and stderr");
> - usage(argv[0]);
> - }
> + if (logfile)
> + errexit("Can't log to both file and stderr");
>
> - if (c->stderr) {
> - err("Multiple --stderr options given");
> - usage(argv[0]);
> - }
> + if (c->stderr)
> + errexit("Multiple --stderr options given");
>
> c->stderr = 1;
> break;
> case 'l':
> - if (c->stderr) {
> - err("Can't log to both stderr and file");
> - usage(argv[0]);
> - }
> + if (c->stderr)
> + errexit("Can't log to both stderr and file");
>
> - if (logfile) {
> - err("Multiple --log-file options given");
> - usage(argv[0]);
> - }
> + if (logfile)
> + errexit("Multiple --log-file options given");
>
> logfile = optarg;
> break;
> case 'q':
> - if (c->quiet) {
> - err("Multiple --quiet options given");
> - usage(argv[0]);
> - }
> + if (c->quiet)
> + errexit("Multiple --quiet options given");
>
> - if (c->debug) {
> - err("Either --debug or --quiet");
> - usage(argv[0]);
> - }
> + if (c->debug)
> + errexit("Either --debug or --quiet");
>
> c->quiet = 1;
> break;
> case 'f':
> - if (c->foreground) {
> - err("Multiple --foreground options given");
> - usage(argv[0]);
> - }
> + if (c->foreground)
> + errexit("Multiple --foreground options given");
>
> c->foreground = 1;
> break;
> case 's':
> - if (*c->sock_path) {
> - err("Multiple --socket options given");
> - usage(argv[0]);
> - }
> + if (*c->sock_path)
> + errexit("Multiple --socket options given");
>
> ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
> optarg);
> - if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) {
> - err("Invalid socket path: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
> + errexit("Invalid socket path: %s", optarg);
> +
> break;
> case 'F':
> - if (c->fd_tap >= 0) {
> - err("Multiple --fd options given");
> - usage(argv[0]);
> - }
> + if (c->fd_tap >= 0)
> + errexit("Multiple --fd options given");
>
> errno = 0;
> c->fd_tap = strtol(optarg, NULL, 0);
>
> - if (c->fd_tap < 0 || errno) {
> - err("Invalid --fd: %s", optarg);
> - usage(argv[0]);
> - }
> + if (c->fd_tap < 0 || errno)
> + errexit("Invalid --fd: %s", optarg);
>
> c->one_off = true;
>
> break;
> case 'I':
> - if (*c->pasta_ifn) {
> - err("Multiple --ns-ifname options given");
> - usage(argv[0]);
> - }
> + if (*c->pasta_ifn)
> + errexit("Multiple --ns-ifname options given");
>
> ret = snprintf(c->pasta_ifn, IFNAMSIZ - 1, "%s",
> optarg);
> - if (ret <= 0 || ret >= IFNAMSIZ - 1) {
> - err("Invalid interface name: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= IFNAMSIZ - 1)
> + errexit("Invalid interface name: %s", optarg);
> +
> break;
> case 'p':
> - if (*c->pcap) {
> - err("Multiple --pcap options given");
> - usage(argv[0]);
> - }
> + if (*c->pcap)
> + errexit("Multiple --pcap options given");
>
> ret = snprintf(c->pcap, sizeof(c->pcap), "%s", optarg);
> - if (ret <= 0 || ret >= (int)sizeof(c->pcap)) {
> - err("Invalid pcap path: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= (int)sizeof(c->pcap))
> + errexit("Invalid pcap path: %s", optarg);
> +
> break;
> case 'P':
> - if (*c->pid_file) {
> - err("Multiple --pid options given");
> - usage(argv[0]);
> - }
> + if (*c->pid_file)
> + errexit("Multiple --pid options given");
>
> ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s",
> optarg);
> - if (ret <= 0 || ret >= (int)sizeof(c->pid_file)) {
> - err("Invalid PID file: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= (int)sizeof(c->pid_file))
> + errexit("Invalid PID file: %s", optarg);
> +
> break;
> case 'm':
> - if (c->mtu) {
> - err("Multiple --mtu options given");
> - usage(argv[0]);
> - }
> + if (c->mtu)
> + errexit("Multiple --mtu options given");
>
> errno = 0;
> c->mtu = strtol(optarg, NULL, 0);
> @@ -1427,11 +1368,9 @@ void conf(struct ctx *c, int argc, char **argv)
> break;
> }
>
> - if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU ||
> - errno) {
> - err("Invalid MTU: %s", optarg);
> - usage(argv[0]);
> - }
> + if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || errno)
This line should still be split like it was.
> + errexit("Invalid MTU: %s", optarg);
> +
> break;
> case 'a':
> if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr) &&
> @@ -1451,24 +1390,21 @@ void conf(struct ctx *c, int argc, char **argv)
> !IN4_IS_ADDR_MULTICAST(&c->ip4.addr))
> break;
>
> - err("Invalid address: %s", optarg);
> - usage(argv[0]);
> + errexit("Invalid address: %s", optarg);
> break;
> case 'n':
> c->ip4.prefix_len = conf_ip4_prefix(optarg);
> - if (c->ip4.prefix_len < 0) {
> - err("Invalid netmask: %s", optarg);
> - usage(argv[0]);
> - }
> + if (c->ip4.prefix_len < 0)
> + errexit("Invalid netmask: %s", optarg);
> +
> break;
> case 'M':
> for (i = 0; i < ETH_ALEN; i++) {
> errno = 0;
> b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
> - if (b < 0 || b > UCHAR_MAX || errno) {
> - err("Invalid MAC address: %s", optarg);
> - usage(argv[0]);
> - }
> + if (b < 0 || b > UCHAR_MAX || errno)
> + errexit("Invalid MAC address: %s", optarg);
Split before 'optarg'.
> +
> c->mac[i] = b;
> }
> break;
> @@ -1486,41 +1422,30 @@ void conf(struct ctx *c, int argc, char **argv)
> !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw))
> break;
>
> - err("Invalid gateway address: %s", optarg);
> - usage(argv[0]);
> + errexit("Invalid gateway address: %s", optarg);
> break;
> case 'i':
> - if (ifi) {
> - err("Redundant interface: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ifi)
> + errexit("Redundant interface: %s", optarg);
>
> - if (!(ifi = if_nametoindex(optarg))) {
> - err("Invalid interface name %s: %s", optarg,
> - strerror(errno));
> - usage(argv[0]);
> - }
> + if (!(ifi = if_nametoindex(optarg)))
> + errexit("Invalid interface name %s: %s", optarg,
> + strerror(errno));
> break;
> case 'D':
> if (!strcmp(optarg, "none")) {
> - if (c->no_dns) {
> - err("Redundant DNS options");
> - usage(argv[0]);
> - }
> + if (c->no_dns)
> + errexit("Redundant DNS options");
>
> - if (dns4 - c->ip4.dns || dns6 - c->ip6.dns) {
> - err("Conflicting DNS options");
> - usage(argv[0]);
> - }
> + if (dns4 - c->ip4.dns || dns6 - c->ip6.dns)
> + errexit("Conflicting DNS options");
>
> c->no_dns = 1;
> break;
> }
>
> - if (c->no_dns) {
> - err("Conflicting DNS options");
> - usage(argv[0]);
> - }
> + if (c->no_dns)
> + errexit("Conflicting DNS options");
>
> if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) &&
> inet_pton(AF_INET, optarg, dns4)) {
> @@ -1534,29 +1459,22 @@ void conf(struct ctx *c, int argc, char **argv)
> break;
> }
>
> - err("Cannot use DNS address %s", optarg);
> - usage(argv[0]);
> + errexit("Cannot use DNS address %s", optarg);
> break;
> case 'S':
> if (!strcmp(optarg, "none")) {
> - if (c->no_dns_search) {
> - err("Redundant DNS search options");
> - usage(argv[0]);
> - }
> + if (c->no_dns_search)
> + errexit("Redundant DNS search options");
>
> - if (dnss != c->dns_search) {
> - err("Conflicting DNS search options");
> - usage(argv[0]);
> - }
> + if (dnss != c->dns_search)
> + errexit("Conflicting DNS search options");
>
> c->no_dns_search = 1;
> break;
> }
>
> - if (c->no_dns_search) {
> - err("Conflicting DNS search options");
> - usage(argv[0]);
> - }
> + if (c->no_dns_search)
> + errexit("Conflicting DNS search options");
>
> if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) {
> ret = snprintf(dnss->n, sizeof(*c->dns_search),
> @@ -1568,8 +1486,7 @@ void conf(struct ctx *c, int argc, char **argv)
> break;
> }
>
> - err("Cannot use DNS search domain %s", optarg);
> - usage(argv[0]);
> + errexit("Cannot use DNS search domain %s", optarg);
> break;
> case '4':
> v4_only = true;
> @@ -1578,15 +1495,11 @@ void conf(struct ctx *c, int argc, char **argv)
> v6_only = true;
> break;
> case '1':
> - if (c->mode != MODE_PASST) {
> - err("--one-off is for passt mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASST)
> + errexit("--one-off is for passt mode only");
>
> - if (c->one_off) {
> - err("Redundant --one-off option");
> - usage(argv[0]);
> - }
> + if (c->one_off)
> + errexit("Redundant --one-off option");
>
> c->one_off = true;
> break;
> @@ -1604,15 +1517,11 @@ void conf(struct ctx *c, int argc, char **argv)
> }
> } while (name != -1);
>
> - if (v4_only && v6_only) {
> - err("Options ipv4-only and ipv6-only are mutually exclusive");
> - usage(argv[0]);
> - }
> + if (v4_only && v6_only)
> + errexit("Options ipv4-only and ipv6-only are mutually exclusive");
>
> - if (*c->sock_path && c->fd_tap >= 0) {
> - err("Options --socket and --fd are mutually exclusive");
> - usage(argv[0]);
> - }
> + if (*c->sock_path && c->fd_tap >= 0)
> + errexit("Options --socket and --fd are mutually exclusive");
>
> ret = conf_ugid(runas, &uid, &gid);
> if (ret)
> @@ -1628,10 +1537,8 @@ void conf(struct ctx *c, int argc, char **argv)
> c->ifi4 = conf_ip4(ifi, &c->ip4, c->mac);
> if (!v4_only)
> c->ifi6 = conf_ip6(ifi, &c->ip6, c->mac);
> - if (!c->ifi4 && !c->ifi6) {
> - err("External interface not usable");
> - exit(EXIT_FAILURE);
> - }
> + if (!c->ifi4 && !c->ifi6)
> + errexit("External interface not usable");
>
> /* Inbound port options can be parsed now (after IPv4/IPv6 settings) */
> optind = 1;
--
Stefano
next prev parent reply other threads:[~2023-02-09 17:45 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
2023-02-08 17:48 ` [PATCH v2 1/9] log to stderr until process is daemonized, even if a logfile is set Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-14 3:41 ` Laine Stump
2023-02-08 17:48 ` [PATCH v2 2/9] add errexit() to log an error message and exit with a single call Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-13 3:22 ` David Gibson
2023-02-13 10:46 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 3/9] eliminate most calls to usage() in conf() Laine Stump
2023-02-09 17:45 ` Stefano Brivio [this message]
2023-02-08 17:48 ` [PATCH v2 4/9] make conf_ports() exit immediately after logging error Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 5/9] make conf_pasta_ns() " Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 6/9] make conf_ugid() " Laine Stump
2023-02-13 4:23 ` Laine Stump
2023-02-08 17:48 ` [PATCH v2 7/9] make conf_netns_opt() " Laine Stump
2023-02-08 17:48 ` [PATCH v2 8/9] log a detailed error (not usage()) when there are extra non-option arguments Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 9/9] convert all remaining err() followed by exit() to errexit() Laine Stump
2023-02-09 17:45 ` Stefano Brivio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230209184536.571c4a28@elisabeth \
--to=sbrivio@redhat.com \
--cc=laine@laine.org \
--cc=laine@redhat.com \
--cc=passt-dev@passt.top \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).