From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 12B0F5A004F for ; Wed, 19 Jun 2024 04:34:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718764462; bh=HQ9fJvW3NaAzr8phO/Go12+4nN7Nvas6WM1IbFWfPpY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eDYjSV2yfTMnEOheZDzfpeVj75ukU6n2f7lZazFFrIflTbPyRkZguJkkXMtUKlhdf le0ziKBtC/Dm7y1a6BO+WHm3nrABOSELdf4q2YAitMGwRUelksTf6bcYTrjzEo0s7+ N+8DuYNTsy1wDR/GF1y5cH/dDSzqa3wiNx9BoZraNaMzfNTFkntSG9s+zLvWerYQBn Ktj57ej+qR0SdoSzTvCX7SEWAH6Q3z0X8TypfXANI16zD/KpMKnY+dIkHGdWoEAxXN IT5zkEhenfftoAO/onifbPyp7EPsC+JUpGA1173O9E7zhndEZ91Ds4sPCnWOjv7Gs6 8oaZd4nJWqpSg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W3nkk3y44z4wyQ; Wed, 19 Jun 2024 12:34:22 +1000 (AEST) Date: Wed, 19 Jun 2024 12:24:49 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 5/6] treewide: Replace perror() calls with calls to logging functions Message-ID: References: <20240618071427.1544869-1-sbrivio@redhat.com> <20240618071427.1544869-6-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gLQz5NqaoSoHjsKr" Content-Disposition: inline In-Reply-To: <20240618071427.1544869-6-sbrivio@redhat.com> Message-ID-Hash: REWOO3CIM4ORTH3GBCWQGU2JAWVDOSMH X-Message-ID-Hash: REWOO3CIM4ORTH3GBCWQGU2JAWVDOSMH 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: --gLQz5NqaoSoHjsKr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jun 18, 2024 at 09:14:26AM +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 As noted elsewhere, I'm not a huge fan of the _perror() helpers, but regardless of that this is a big improvement to clarity. 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 14feee1..344eb07 100644 > --- a/conf.c > +++ b/conf.c > @@ -1098,10 +1098,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 9ddc58c..ec833c4 100644 > --- a/log.c > +++ b/log.c > @@ -208,10 +208,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); > @@ -224,10 +222,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 7436120..542d3fb 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)) > @@ -324,10 +317,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 --gLQz5NqaoSoHjsKr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZyQXAACgkQzQJF27ox 2GcGrQ//dq4HqoJXC0PNx5vaQnnG9WqayEwdSILgshF9cveuABrZDYXsEwxmoKSJ HrUQY379mLBAKLu97cYOkvA1xrSDACGpsyfYooLNCoBwLVek658Bv4zZ+NlzcZCL AgrlCWUHgSh5xbuoocnBaZMWrZATcssu6U3EI1InTe3e8swhxgr7WjBgtSR8hfTM SBPZFA9gzLJBrbgmDYFInCzWZ6QVT8bzaPqu+DWL6o3qBIaNEmXrLZqMaPp4o6yA ViMwSgZ0jqeFz5A9Qtz2QDWdng4yyrvCF/fi48P+GBCAXh0yr8M48hxEInNhEquR OHZDp6L2TIzw3OtdNAZBC/ZKaPff4AJKipJA/IsSpeSp/jEpGJUp0t/eJylbxnGy rzAJ+q1AFaZ5xNcBONNDrXyvz6Ph0iwOB9FGIkcweet1NPKGlFjcIDIx2RP074Gz CA9wy42dFZjfxA3S0R/lXobQXE+susGJSzgmoL71lb+ww84BG2rUvBHUsu5dzBD/ +udvP16VONRMf60d879b3R8wC2DPvoYRy87IwYWKFGE2Mrs+FXfzyEqLDIPM7gzL ozjwuxuLtZdjDSZGC954+AIgsA+8CYVCHbpf/NbqM2eDaFhbXie6i98ych/yl3tR 6uYT5L80Crmov9Naw+D+T9Bpy7XGE7j3qeeuYamknPj70tod+UY= =HXx8 -----END PGP SIGNATURE----- --gLQz5NqaoSoHjsKr--