From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 539D85A0059 for ; Thu, 20 Jun 2024 02:49:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718844566; bh=n5APq7k81Y792XbOf0TDaBfvyeZAcyJFUtJl8L9jPLw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=F2muzwR//IEatAll0X/LUBoGADlqIme5yQGOL47SxU50SSK3OTE0FSUgcq0c6jYJu oFZOgxI6JYNTI1tnJ5P6n7RiEBx/gSHT+J5/h/fX1lOhy7HfM+OjWBSouw3ID3WKX6 AxoS7FPPongnhXghCJkYTv1UIBf6XyUU5WCPEPtvj6A0C0al0aCxZ05+lJYoemPRM/ h+vj4vkOveoTfeIxgoU+T7tZ9l+gEhneethehvWPOPROypqakZW6KVWaWD7yCWSzKr 7iG3l5pTZmdb3XBDSYr+1xJIKe1kdVhV9PqCyM/zKTQPaVpmDRh2W/fnNF/aTAFolY 9Qc09JCTWU+ow== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W4MMB5CYsz4wyl; Thu, 20 Jun 2024 10:49:26 +1000 (AEST) Date: Thu, 20 Jun 2024 10:47:00 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 6/8] treewide: Replace perror() calls with calls to logging functions Message-ID: References: <20240619194028.2913930-1-sbrivio@redhat.com> <20240619194028.2913930-7-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="eYV34gTcHC1mJNRS" Content-Disposition: inline In-Reply-To: <20240619194028.2913930-7-sbrivio@redhat.com> Message-ID-Hash: TBEQM6VDMWXYDWGO7HMDFATKIF2NCOOE X-Message-ID-Hash: TBEQM6VDMWXYDWGO7HMDFATKIF2NCOOE X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Yalan Zhang X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --eYV34gTcHC1mJNRS Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 19, 2024 at 09:40:26PM +0200, Stefano Brivio wrote: > perror() prints directly to standard error, but in many cases standard > error might be already closed, or we might want to skip logging, based > on configuration. Our logging functions provide all that. >=20 > While at it, make errors more descriptive, replacing some of the > existing basic perror-style messages. >=20 > Signed-off-by: Stefano Brivio Reviewed-by: David Gibson > --- > arch.c | 10 +++++----- > conf.c | 6 ++---- > isolation.c | 18 ++++++++---------- > log.c | 12 ++++-------- > passt.c | 41 ++++++++++++++++------------------------- > pasta.c | 9 +++------ > 6 files changed, 38 insertions(+), 58 deletions(-) >=20 > diff --git a/arch.c b/arch.c > index 80a41bc..04bebfc 100644 > --- a/arch.c > +++ b/arch.c > @@ -18,6 +18,8 @@ > #include > #include > =20 > +#include "log.h" > + > /** > * arch_avx2_exec() - Switch to AVX2 build if supported > * @argv: Arguments from command line > @@ -28,10 +30,8 @@ void arch_avx2_exec(char **argv) > char exe[PATH_MAX] =3D { 0 }; > const char *p; > =20 > - if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0) { > - perror("readlink /proc/self/exe"); > - exit(EXIT_FAILURE); > - } > + if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0) > + die_perror("Failed to read own /proc/self/exe link"); > =20 > p =3D strstr(exe, ".avx2"); > if (p && strlen(p) =3D=3D strlen(".avx2")) > @@ -42,7 +42,7 @@ void arch_avx2_exec(char **argv) > =20 > snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe); > execve(new_path, argv, environ); > - perror("Can't run AVX2 build, using non-AVX2 version"); > + warn_perror("Can't run AVX2 build, using non-AVX2 version"); > } > } > #else > diff --git a/conf.c b/conf.c > index 726efaf..b8247c4 100644 > --- a/conf.c > +++ b/conf.c > @@ -1096,10 +1096,8 @@ static void conf_ugid(char *runas, uid_t *uid, gid= _t *gid) > const struct passwd *pw; > /* cppcheck-suppress getpwnamCalled */ > pw =3D getpwnam("nobody"); > - if (!pw) { > - perror("getpwnam"); > - exit(EXIT_FAILURE); > - } > + if (!pw) > + die_perror("Can't get password file entry for nobody"); > =20 > *uid =3D pw->pw_uid; > *gid =3D pw->pw_gid; > diff --git a/isolation.c b/isolation.c > index ca2c68b..c936674 100644 > --- a/isolation.c > +++ b/isolation.c > @@ -316,34 +316,34 @@ int isolate_prefork(const struct ctx *c) > flags |=3D CLONE_NEWPID; > =20 > if (unshare(flags)) { > - perror("unshare"); > + err_perror("Failed to detach isolating namespaces"); > return -errno; > } > =20 > if (mount("", "/", "", MS_UNBINDABLE | MS_REC, NULL)) { > - perror("mount /"); > + err_perror("Failed to remount /"); > return -errno; > } > =20 > if (mount("", TMPDIR, "tmpfs", > MS_NODEV | MS_NOEXEC | MS_NOSUID | MS_RDONLY, > "nr_inodes=3D2,nr_blocks=3D0")) { > - perror("mount tmpfs"); > + err_perror("Failed to mount empty tmpfs for pivot_root()"); > return -errno; > } > =20 > if (chdir(TMPDIR)) { > - perror("chdir"); > + err_perror("Failed to change directory into empty tmpfs"); > return -errno; > } > =20 > if (syscall(SYS_pivot_root, ".", ".")) { > - perror("pivot_root"); > + err_perror("Failed to pivot_root() into empty tmpfs"); > return -errno; > } > =20 > if (umount2(".", MNT_DETACH | UMOUNT_NOFOLLOW)) { > - perror("umount2"); > + err_perror("Failed to unmount original root filesystem"); > return -errno; > } > =20 > @@ -388,8 +388,6 @@ void isolate_postfork(const struct ctx *c) > } > =20 > if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) || > - prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) { > - perror("prctl"); > - exit(EXIT_FAILURE); > - } > + prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) > + die_perror("Failed to apply seccomp filter"); > } > diff --git a/log.c b/log.c > index 43117c1..09a7155 100644 > --- a/log.c > +++ b/log.c > @@ -206,10 +206,8 @@ void logfile_init(const char *name, const char *path= , size_t size) > char nl =3D '\n', exe[PATH_MAX] =3D { 0 }; > int n; > =20 > - if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0) { > - perror("readlink /proc/self/exe"); > - exit(EXIT_FAILURE); > - } > + if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0) > + die_perror("Failed to read own /proc/self/exe link"); > =20 > log_file =3D open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEX= EC, > S_IRUSR | S_IWUSR); > @@ -222,10 +220,8 @@ void logfile_init(const char *name, const char *path= , size_t size) > name, exe, getpid()); > =20 > if (write(log_file, log_header, n) <=3D 0 || > - write(log_file, &nl, 1) <=3D 0) { > - perror("Couldn't write to log file\n"); > - exit(EXIT_FAILURE); > - } > + write(log_file, &nl, 1) <=3D 0) > + die_perror("Couldn't write to log file"); > =20 > /* For FALLOC_FL_COLLAPSE_RANGE: VFS block size can be up to one page */ > log_cut_size =3D ROUND_UP(log_size * LOGFILE_CUT_RATIO / 100, PAGE_SIZE= ); > diff --git a/passt.c b/passt.c > index c5538bd..5d154e1 100644 > --- a/passt.c > +++ b/passt.c > @@ -136,14 +136,13 @@ static void secret_init(struct ctx *c) > } > if (dev_random >=3D 0) > close(dev_random); > - if (random_read < sizeof(c->hash_secret)) { > + > + if (random_read < sizeof(c->hash_secret)) > #else > if (getrandom(&c->hash_secret, sizeof(c->hash_secret), > - GRND_RANDOM) < 0) { > + GRND_RANDOM) < 0) > #endif /* !HAS_GETRANDOM */ > - perror("TCP initial sequence getrandom"); > - exit(EXIT_FAILURE); > - } > + die_perror("Failed to get random bytes for hash table and TCP"); > } > =20 > /** > @@ -250,20 +249,16 @@ int main(int argc, char **argv) > madvise(pkt_buf, TAP_BUF_BYTES, MADV_HUGEPAGE); > =20 > c.epollfd =3D epoll_create1(EPOLL_CLOEXEC); > - if (c.epollfd =3D=3D -1) { > - perror("epoll_create1"); > - exit(EXIT_FAILURE); > - } > + if (c.epollfd =3D=3D -1) > + die_perror("Failed to create epoll file descriptor"); > + > + if (getrlimit(RLIMIT_NOFILE, &limit)) > + die_perror("Failed to get maximum value of open files limit"); > =20 > - if (getrlimit(RLIMIT_NOFILE, &limit)) { > - perror("getrlimit"); > - exit(EXIT_FAILURE); > - } > c.nofile =3D limit.rlim_cur =3D limit.rlim_max; > - if (setrlimit(RLIMIT_NOFILE, &limit)) { > - perror("setrlimit"); > - exit(EXIT_FAILURE); > - } > + if (setrlimit(RLIMIT_NOFILE, &limit)) > + die_perror("Failed to set current limit for open files"); > + > sock_probe_mem(&c); > =20 > conf(&c, argc, argv); > @@ -293,10 +288,8 @@ int main(int argc, char **argv) > pcap_init(&c); > =20 > if (!c.foreground) { > - if ((devnull_fd =3D open("/dev/null", O_RDWR | O_CLOEXEC)) < 0) { > - perror("/dev/null open"); > - exit(EXIT_FAILURE); > - } > + if ((devnull_fd =3D open("/dev/null", O_RDWR | O_CLOEXEC)) < 0) > + die_perror("Failed to open /dev/null"); > } > =20 > if (isolate_prefork(&c)) > @@ -320,10 +313,8 @@ loop: > /* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */ > /* cppcheck-suppress [duplicateValueTernary, unmatchedSuppression] */ > nfds =3D epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL); > - if (nfds =3D=3D -1 && errno !=3D EINTR) { > - perror("epoll_wait"); > - exit(EXIT_FAILURE); > - } > + if (nfds =3D=3D -1 && errno !=3D EINTR) > + die_perror("epoll_wait() failed in main loop"); > =20 > clock_gettime(CLOCK_MONOTONIC, &now); > =20 > diff --git a/pasta.c b/pasta.c > index b85ea2b..d08391f 100644 > --- a/pasta.c > +++ b/pasta.c > @@ -197,8 +197,7 @@ static int pasta_spawn_cmd(void *arg) > a =3D (const struct pasta_spawn_cmd_arg *)arg; > execvp(a->exe, a->argv); > =20 > - perror("execvp"); > - exit(EXIT_FAILURE); > + die_perror("Failed to start command or shell"); > } > =20 > /** > @@ -261,10 +260,8 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t = gid, > CLONE_NEWUTS | CLONE_NEWNS | SIGCHLD, > (void *)&arg); > =20 > - if (pasta_child_pid =3D=3D -1) { > - perror("clone"); > - exit(EXIT_FAILURE); > - } > + if (pasta_child_pid =3D=3D -1) > + die_perror("Failed to clone process with detached namespaces"); > =20 > NS_CALL(pasta_wait_for_ns, c); > if (c->pasta_netns_fd < 0) --=20 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 --eYV34gTcHC1mJNRS Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZzfAMACgkQzQJF27ox 2GdlcA//dFDdNgsnpbI/kyudXPjnKwMl4v+w9WswlW6OxR6qM05Y7606GsXGHXAZ fd77EhJpZH64f61R2N+XqpecjltE5wS3v68CDXA6M5CSvBlTMnCNXypFvfap5/No Lz9vivbqmIIE4odtpNYNQIHPQ3HxHaOd4rG1lQVwrcJZs8q9+31LJoL/1uvl0Ci2 mJRv9QmLF3TzZpl9b+1gOC0sDQP2rFaJG73TErhUNZxmZE9UsE3zm1oXskALcXpm 9PU0trd0SXTiz8YvXI+qKIw3MOahnnOopbv9YaKt2RrOA5mEFY6qj3e49u6g1jxh szEobeUm+edg4qKnt8k5SHDEU8pw7V5HotMyAqgiHD+xKk85zy28c1Svf525V4Sg lUwrHbE/3M7/hp1ITHHZEbPrsxI/YaCCPbNHrYvZTITdhsKN7ct3n8Woh+9dRwk4 mjimi6lIPk64fLguy8e/NU3+f0onNCt3rgE6/Fh7Pobbx1rVV41q59NFSsaPlVWT zizaFQSiv2JF4PsYDe+UYm7tjatgdbvi5obIoCzoEMxdrlpKyeTg5gGXOWBB6dAT JNSkXqU52BWUSHCp81T4mWBImVSE+7PYuP3Mxdro+smuY6Rqxu3LLYhuYPHuwbxp Gn1P/PmpiqAtxEbKNXjoKVjao7S7WxRbSuVZl5Wp9QV5qXVYo0g= =fZ6d -----END PGP SIGNATURE----- --eYV34gTcHC1mJNRS--