public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
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


  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).