Re: [PATCH v3 7/8] treewide: Replace strerror() calls

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 16576 bytes --]

On Wed, Jun 19, 2024 at 09:40:27PM +0200, Stefano Brivio wrote:
> Now that we have logging functions embedding perror() functionality,
> we can make _some_ calls more terse by using them. In many places,
> the strerror() calls are still more convenient because, for example,
> they are used in flow debugging functions, or because the return code
> variable of interest is not 'errno'.
> 
> While at it, convert a few error messages from a scant perror style
> to proper failure descriptions.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  conf.c      |  6 +++---
>  fwd.c       |  2 +-
>  isolation.c | 28 +++++++++++-----------------
>  log.c       |  2 +-
>  netlink.c   |  4 ++--
>  passt.c     | 12 ++++--------
>  pasta.c     | 18 ++++++++----------
>  pcap.c      |  8 +++-----
>  tap.c       | 14 +++++++-------
>  tcp.c       | 24 ++++++++----------------
>  util.c      | 12 +++++-------
>  11 files changed, 53 insertions(+), 77 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index b8247c4..09bf6dd 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -461,7 +461,7 @@ static void get_dns(struct ctx *c)
>  	}
>  
>  	if (line_len < 0)
> -		warn("Error reading /etc/resolv.conf: %s", strerror(errno));
> +		warn_perror("Error reading /etc/resolv.conf");
>  	close(fd);
>  
>  out:
> @@ -626,6 +626,7 @@ static unsigned int conf_ip4(unsigned int ifi,
>  		int rc = nl_link_get_mac(nl_sock, ifi, mac);
>  		if (rc < 0) {
>  			char ifname[IFNAMSIZ];
> +
>  			err("Couldn't discover MAC address for %s: %s",
>  			    if_indextoname(ifi, ifname), strerror(-rc));
>  			return 0;
> @@ -1549,8 +1550,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  				die("Redundant interface: %s", optarg);
>  
>  			if (!(ifi4 = ifi6 = if_nametoindex(optarg)))
> -				die("Invalid interface name %s: %s", optarg,
> -				    strerror(errno));
> +				die_perror("Invalid interface name %s", optarg);
>  			break;
>  		case 'o':
>  			if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out)	  &&
> diff --git a/fwd.c b/fwd.c
> index b3d5a37..d3f1798 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -52,7 +52,7 @@ static void procfs_scan_listen(int fd, unsigned int lstate,
>  		return;
>  
>  	if (lseek(fd, 0, SEEK_SET)) {
> -		warn("lseek() failed on /proc/net file: %s", strerror(errno));
> +		warn_perror("lseek() failed on /proc/net file");
>  		return;
>  	}
>  
> diff --git a/isolation.c b/isolation.c
> index c936674..4956d7e 100644
> --- a/isolation.c
> +++ b/isolation.c
> @@ -105,7 +105,7 @@ static void drop_caps_ep_except(uint64_t keep)
>  	int i;
>  
>  	if (syscall(SYS_capget, &hdr, data))
> -		die("Couldn't get current capabilities: %s", strerror(errno));
> +		die_perror("Couldn't get current capabilities");
>  
>  	for (i = 0; i < CAP_WORDS; i++) {
>  		uint32_t mask = keep >> (32 * i);
> @@ -115,7 +115,7 @@ static void drop_caps_ep_except(uint64_t keep)
>  	}
>  
>  	if (syscall(SYS_capset, &hdr, data))
> -		die("Couldn't drop capabilities: %s", strerror(errno));
> +		die_perror("Couldn't drop capabilities");
>  }
>  
>  /**
> @@ -152,19 +152,17 @@ static void clamp_caps(void)
>  		 */
>  		if (prctl(PR_CAPBSET_DROP, i, 0, 0, 0) &&
>  		    errno != EINVAL && errno != EPERM)
> -			die("Couldn't drop cap %i from bounding set: %s",
> -			    i, strerror(errno));
> +			die_perror("Couldn't drop cap %i from bounding set", i);
>  	}
>  
>  	if (syscall(SYS_capget, &hdr, data))
> -		die("Couldn't get current capabilities: %s", strerror(errno));
> +		die_perror("Couldn't get current capabilities");
>  
>  	for (i = 0; i < CAP_WORDS; i++)
>  		data[i].inheritable = 0;
>  
>  	if (syscall(SYS_capset, &hdr, data))
> -		die("Couldn't drop inheritable capabilities: %s",
> -		    strerror(errno));
> +		die_perror("Couldn't drop inheritable capabilities");
>  }
>  
>  /**
> @@ -234,34 +232,30 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns,
>  	if (setgroups(0, NULL)) {
>  		/* If we don't have CAP_SETGID, this will EPERM */
>  		if (errno != EPERM)
> -			die("Can't drop supplementary groups: %s",
> -			    strerror(errno));
> +			die_perror("Can't drop supplementary groups");
>  	}
>  
>  	if (setgid(gid) != 0)
> -		die("Can't set GID to %u: %s", gid, strerror(errno));
> +		die_perror("Can't set GID to %u", gid);
>  
>  	if (setuid(uid) != 0)
> -		die("Can't set UID to %u: %s", uid, strerror(errno));
> +		die_perror("Can't set UID to %u", uid);
>  
>  	if (*userns) { /* If given a userns, join it */
>  		int ufd;
>  
>  		ufd = open(userns, O_RDONLY | O_CLOEXEC);
>  		if (ufd < 0)
> -			die("Couldn't open user namespace %s: %s",
> -			    userns, strerror(errno));
> +			die_perror("Couldn't open user namespace %s", userns);
>  
>  		if (setns(ufd, CLONE_NEWUSER) != 0)
> -			die("Couldn't enter user namespace %s: %s",
> -			    userns, strerror(errno));
> +			die_perror("Couldn't enter user namespace %s", userns);
>  
>  		close(ufd);
>  
>  	} else if (use_userns) { /* Create and join a new userns */
>  		if (unshare(CLONE_NEWUSER) != 0)
> -			die("Couldn't create user namespace: %s",
> -			    strerror(errno));
> +			die_perror("Couldn't create user namespace");
>  	}
>  
>  	/* Joining a new userns gives us full capabilities; drop the
> diff --git a/log.c b/log.c
> index 09a7155..b34c203 100644
> --- a/log.c
> +++ b/log.c
> @@ -212,7 +212,7 @@ 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)
> -		die("Couldn't open log file %s: %s", path, strerror(errno));
> +		die_perror("Couldn't open log file %s", path);
>  
>  	log_size = size ? size : LOGFILE_SIZE_DEFAULT;
>  
> diff --git a/netlink.c b/netlink.c
> index 2c9e71f..3aed7e5 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -133,7 +133,7 @@ static uint32_t nl_send(int s, void *req, uint16_t type,
>  
>  	n = send(s, req, len, 0);
>  	if (n < 0)
> -		die("netlink: Failed to send(): %s", strerror(errno));
> +		die_perror("netlink: Failed to send()");
>  	else if (n < len)
>  		die("netlink: Short send (%zd of %zd bytes)", n, len);
>  
> @@ -189,7 +189,7 @@ static struct nlmsghdr *nl_next(int s, char *buf, struct nlmsghdr *nh, ssize_t *
>  
>  	*n = recv(s, buf, NLBUFSIZ, 0);
>  	if (*n < 0)
> -		die("netlink: Failed to recv(): %s", strerror(errno));
> +		die_perror("netlink: Failed to recv()");
>  
>  	nh = (struct nlmsghdr *)buf;
>  	if (!NLMSG_OK(nh, *n))
> diff --git a/passt.c b/passt.c
> index 5d154e1..0ba208d 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -227,15 +227,11 @@ int main(int argc, char **argv)
>  		__openlog("pasta", 0, LOG_DAEMON);
>  
>  		sa.sa_handler = pasta_child_handler;
> -		if (sigaction(SIGCHLD, &sa, NULL)) {
> -			die("Couldn't install signal handlers: %s",
> -			    strerror(errno));
> -		}
> +		if (sigaction(SIGCHLD, &sa, NULL))
> +			die_perror("Couldn't install signal handlers");
>  
> -		if (signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
> -			die("Couldn't set disposition for SIGPIPE: %s",
> -			    strerror(errno));
> -		}
> +		if (signal(SIGPIPE, SIG_IGN) == SIG_ERR)
> +			die_perror("Couldn't set disposition for SIGPIPE");
>  
>  		c.mode = MODE_PASTA;
>  	} else if (strstr(name, "passt")) {
> diff --git a/pasta.c b/pasta.c
> index d08391f..084288c 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -138,17 +138,15 @@ void pasta_open_ns(struct ctx *c, const char *netns)
>  	int nfd = -1;
>  
>  	nfd = open(netns, O_RDONLY | O_CLOEXEC);
> -	if (nfd < 0) {
> -		die("Couldn't open network namespace %s: %s",
> -		    netns, strerror(errno));
> -	}
> +	if (nfd < 0)
> +		die_perror("Couldn't open network namespace %s", netns);
>  
>  	c->pasta_netns_fd = nfd;
>  
>  	NS_CALL(ns_check, c);
>  
>  	if (c->pasta_netns_fd < 0)
> -		die("Couldn't switch to pasta namespaces: %s", strerror(errno));
> +		die_perror("Couldn't switch to pasta namespaces");
>  
>  	if (!c->no_netns_quit) {
>  		char buf[PATH_MAX] = { 0 };
> @@ -184,7 +182,7 @@ static int pasta_spawn_cmd(void *arg)
>  
>  	/* We run in a detached PID and mount namespace: mount /proc over */
>  	if (mount("", "/proc", "proc", 0, NULL))
> -		warn("Couldn't mount /proc: %s", strerror(errno));
> +		warn_perror("Couldn't mount /proc");
>  
>  	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
>  		warn("Cannot set ping_group_range, ICMP requests might fail");
> @@ -265,7 +263,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
>  
>  	NS_CALL(pasta_wait_for_ns, c);
>  	if (c->pasta_netns_fd < 0)
> -		die("Failed to join network namespace: %s", strerror(errno));
> +		die_perror("Failed to join network namespace");
>  }
>  
>  /**
> @@ -369,12 +367,12 @@ static int pasta_netns_quit_timer(void)
>  	struct itimerspec it = { { 1, 0 }, { 1, 0 } }; /* one-second interval */
>  
>  	if (fd == -1) {
> -		err("timerfd_create(): %s", strerror(errno));
> +		err_perror("Failed to create timerfd for quit timer");
>  		return -errno;
>  	}
>  
>  	if (timerfd_settime(fd, 0, &it, NULL) < 0) {
> -		err("timerfd_settime(): %s", strerror(errno));
> +		err_perror("Failed to set interval for quit timer");
>  		close(fd);
>  		return -errno;
>  	}
> @@ -467,7 +465,7 @@ void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref)
>  
>  	n = read(ref.fd, &expirations, sizeof(expirations));
>  	if (n < 0)
> -		die("Namespace watch timer read() error: %s", strerror(errno));
> +		die_perror("Namespace watch timer read() error");
>  	if ((size_t)n < sizeof(expirations))
>  		warn("Namespace watch timer: short read(): %zi", n);
>  
> diff --git a/pcap.c b/pcap.c
> index 507be2a..46cc4b0 100644
> --- a/pcap.c
> +++ b/pcap.c
> @@ -89,10 +89,8 @@ static void pcap_frame(const struct iovec *iov, size_t iovcnt,
>  	struct iovec hiov = { &h, sizeof(h) };
>  
>  	if (write_remainder(pcap_fd, &hiov, 1, 0) < 0 ||
> -	    write_remainder(pcap_fd, iov, iovcnt, offset) < 0) {
> -		debug("Cannot log packet, length %zu: %s",
> -		      l2len, strerror(errno));
> -	}
> +	    write_remainder(pcap_fd, iov, iovcnt, offset) < 0)
> +		debug_perror("Cannot log packet, length %zu", l2len);
>  }
>  
>  /**
> @@ -178,5 +176,5 @@ void pcap_init(struct ctx *c)
>  	info("Saving packet capture to %s", c->pcap);
>  
>  	if (write(pcap_fd, &pcap_hdr, sizeof(pcap_hdr)) < 0)
> -		warn("Cannot write PCAP header: %s", strerror(errno));
> +		warn_perror("Cannot write PCAP header");
>  }
> diff --git a/tap.c b/tap.c
> index c9aeff1..ec994a2 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -325,7 +325,7 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
>  		size_t framelen = iov_size(iov + i, bufs_per_frame);
>  
>  		if (rc < 0) {
> -			debug("tap write: %s", strerror(errno));
> +			debug_perror("tap write");
>  
>  			switch (errno) {
>  			case EAGAIN:
> @@ -387,7 +387,7 @@ static size_t tap_send_frames_passt(const struct ctx *c,
>  		size_t rembufs = bufs_per_frame - (i % bufs_per_frame);
>  
>  		if (write_remainder(c->fd_tap, &iov[i], rembufs, buf_offset) < 0) {
> -			err("tap: partial frame send: %s", strerror(errno));
> +			err_perror("tap: partial frame send");
>  			return i;
>  		}
>  		i += rembufs;
> @@ -1122,7 +1122,7 @@ int tap_sock_unix_open(char *sock_path)
>  	int i;
>  
>  	if (fd < 0)
> -		die("UNIX socket: %s", strerror(errno));
> +		die_perror("Failed to open UNIX domain socket");
>  
>  	for (i = 1; i < UNIX_SOCK_MAX; i++) {
>  		char *path = addr.sun_path;
> @@ -1135,7 +1135,7 @@ int tap_sock_unix_open(char *sock_path)
>  
>  		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
>  		if (ex < 0)
> -			die("UNIX domain socket check: %s", strerror(errno));
> +			die_perror("Failed to check for UNIX domain conflicts");
>  
>  		ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
>  		if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
> @@ -1155,7 +1155,7 @@ int tap_sock_unix_open(char *sock_path)
>  	}
>  
>  	if (i == UNIX_SOCK_MAX)
> -		die("UNIX socket bind: %s", strerror(errno));
> +		die_perror("Failed to bind UNIX domain socket");
>  
>  	info("UNIX domain socket bound at %s", addr.sun_path);
>  	if (!*sock_path)
> @@ -1261,11 +1261,11 @@ static int tap_ns_tun(void *arg)
>  
>  	fd = open("/dev/net/tun", flags);
>  	if (fd < 0)
> -		die("Failed to open() /dev/net/tun: %s", strerror(errno));
> +		die_perror("Failed to open() /dev/net/tun");
>  
>  	rc = ioctl(fd, TUNSETIFF, &ifr);
>  	if (rc < 0)
> -		die("TUNSETIFF failed: %s", strerror(errno));
> +		die_perror("TUNSETIFF ioctl on /dev/net/tun failed");
>  
>  	if (!(c->pasta_ifi = if_nametoindex(c->pasta_ifn)))
>  		die("Tap device opened but no network interface found");
> diff --git a/tcp.c b/tcp.c
> index a2e81d5..698e7ec 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1553,19 +1553,15 @@ static void tcp_bind_outbound(const struct ctx *c, int s, sa_family_t af)
>  				.sin_addr = c->ip4.addr_out,
>  			};
>  
> -			if (bind(s, (struct sockaddr *)&addr4, sizeof(addr4))) {
> -				debug("Can't bind IPv4 TCP socket address: %s",
> -				      strerror(errno));
> -			}
> +			if (bind(s, (struct sockaddr *)&addr4, sizeof(addr4)))
> +				debug_perror("IPv4 TCP socket address bind");
>  		}
>  
>  		if (*c->ip4.ifname_out) {
>  			if (setsockopt(s, SOL_SOCKET, SO_BINDTODEVICE,
>  				       c->ip4.ifname_out,
> -				       strlen(c->ip4.ifname_out))) {
> -				debug("Can't bind IPv4 TCP socket to interface:"
> -				      " %s", strerror(errno));
> -			}
> +				       strlen(c->ip4.ifname_out)))
> +				debug_perror("IPv4 TCP socket interface bind");
>  		}
>  	} else if (af == AF_INET6) {
>  		if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out)) {
> @@ -1575,19 +1571,15 @@ static void tcp_bind_outbound(const struct ctx *c, int s, sa_family_t af)
>  				.sin6_addr = c->ip6.addr_out,
>  			};
>  
> -			if (bind(s, (struct sockaddr *)&addr6, sizeof(addr6))) {
> -				debug("Can't bind IPv6 TCP socket address: %s",
> -				      strerror(errno));
> -			}
> +			if (bind(s, (struct sockaddr *)&addr6, sizeof(addr6)))
> +				debug_perror("IPv6 TCP socket address bind");
>  		}
>  
>  		if (*c->ip6.ifname_out) {
>  			if (setsockopt(s, SOL_SOCKET, SO_BINDTODEVICE,
>  				       c->ip6.ifname_out,
> -				       strlen(c->ip6.ifname_out))) {
> -				debug("Can't bind IPv6 TCP socket to interface:"
> -				      " %s", strerror(errno));
> -			}
> +				       strlen(c->ip6.ifname_out)))
> +				debug_perror("IPv6 TCP socket interface bind");
>  		}
>  	}
>  }
> diff --git a/util.c b/util.c
> index 77448ec..dd2e57f 100644
> --- a/util.c
> +++ b/util.c
> @@ -315,7 +315,7 @@ void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b)
>  void ns_enter(const struct ctx *c)
>  {
>  	if (setns(c->pasta_netns_fd, CLONE_NEWNET))
> -		die("setns() failed entering netns: %s", strerror(errno));
> +		die_perror("setns() failed entering netns");
>  }
>  
>  /**
> @@ -330,10 +330,8 @@ bool ns_is_init(void)
>  	bool ret = true;
>  	int fd;
>  
> -	if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) {
> -		die("Can't determine if we're in init namespace: %s",
> -		    strerror(errno));
> -	}
> +	if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0)
> +		die_perror("Can't determine if we're in init namespace");
>  
>  	if (read(fd, buf, sizeof(root_uid_map)) != sizeof(root_uid_map) - 1 ||
>  	    strncmp(buf, root_uid_map, sizeof(root_uid_map)))
> @@ -509,7 +507,7 @@ int write_file(const char *path, const char *buf)
>  	size_t len = strlen(buf);
>  
>  	if (fd < 0) {
> -		warn("Could not open %s: %s", path, strerror(errno));
> +		warn_perror("Could not open %s", path);
>  		return -1;
>  	}
>  
> @@ -517,7 +515,7 @@ int write_file(const char *path, const char *buf)
>  		ssize_t rc = write(fd, buf, len);
>  
>  		if (rc <= 0) {
> -			warn("Couldn't write to %s: %s", path, strerror(errno));
> +			warn_perror("Couldn't write to %s", path);
>  			break;
>  		}
>  

-- 
David Gibson (he or they)	| 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 --]