From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202410 header.b=SYGnOSRS; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 4028C5A004E for ; Tue, 29 Oct 2024 05:25:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202410; t=1730175927; bh=3c43dG1I/8ZRG46yv1LQorE2lQ9Sewox8nhFxExkkZk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SYGnOSRSnmxDVTRWAckjTQxWVORAVq7kd7VPWDuljlkoVxJW+0EiYj1gJTJYjNbEl 85kKg2pz2QhOz2+9summJgK7EawxQx9TYStWG44l+wftkk6y4AuwSNIUITtvn9rIl8 BXgjX/R38VrkzMntbircFdqjlMcxH4jjFsvK+1QDD4YnnLICTVd3GLUxfKsiDO+FZo whkc7J/PoUeKXqZ6PS6i1gZ76aJ/6xX2/LUE+G3SN22GId9x2gszJgCC8cuLeKAYGD ADpXNYUQYX1eJHh9myUO0jCew+Ut0/Gd8QQIKPqOaKB789yupt+cNsfyd4kKGHCh4E bdvDmbEA+gHEg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Xcxxz6hxLz4x8D; Tue, 29 Oct 2024 15:25:27 +1100 (AEDT) Date: Tue, 29 Oct 2024 15:24:03 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 6/9] treewide: Suppress clang-tidy warning if we already use O_CLOEXEC or if we can't Message-ID: References: <20241028100044.939714-1-sbrivio@redhat.com> <20241028100044.939714-7-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="srOK5SR6eZwB4ox8" Content-Disposition: inline In-Reply-To: <20241028100044.939714-7-sbrivio@redhat.com> Message-ID-Hash: JPWJMWO4AVPVK6I6JA7RIJYVCF4AQI5K X-Message-ID-Hash: JPWJMWO4AVPVK6I6JA7RIJYVCF4AQI5K 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 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: --srOK5SR6eZwB4ox8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 28, 2024 at 11:00:41AM +0100, Stefano Brivio wrote: > In pcap_init(), we should always open the packet capture file with > O_CLOEXEC, even if we're not running in foreground: O_CLOEXEC means > close-on-exec, not close-on-fork. >=20 > In logfile_init() and pidfile_open(), the fact that we pass a third > 'mode' argument to open() seems to confuse the android-cloexec-open > checker in LLVM versions from 16 to 19 (at least). >=20 > The checker is suggesting to add O_CLOEXEC to 'mode', and not in > 'flags', where we already have it. =2E. well.. the checker with the googletest package installed, anyway :/ > Add a suppression for clang-tidy and a comment, and avoid repeating > those three time by adding a new helper, output_file_open(). >=20 > Signed-off-by: Stefano Brivio Reviewed-by: David Gibson > --- > conf.c | 3 ++- > log.c | 3 +-- > pcap.c | 7 ++----- > util.c | 26 ++++++++++---------------- > util.h | 2 +- > 5 files changed, 16 insertions(+), 25 deletions(-) >=20 > diff --git a/conf.c b/conf.c > index 4db7c64..b28f411 100644 > --- a/conf.c > +++ b/conf.c > @@ -1194,7 +1194,8 @@ static void conf_open_files(struct ctx *c) > if (c->mode !=3D MODE_PASTA && c->fd_tap =3D=3D -1) > c->fd_tap_listen =3D tap_sock_unix_open(c->sock_path); > =20 > - c->pidfile_fd =3D pidfile_open(c->pidfile); > + if (*c->pidfile && (c->pidfile_fd =3D output_file_open(c->pidfile) < 0)) > + die_perror("Couldn't open PID file %s", c->pidfile); > } > =20 > /** > diff --git a/log.c b/log.c > index dd25862..48db4d9 100644 > --- a/log.c > +++ b/log.c > @@ -410,8 +410,7 @@ void logfile_init(const char *name, const char *path,= size_t size) > 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_RDWR | O_CLOEXEC, > - S_IRUSR | S_IWUSR); > + log_file =3D output_file_open(path); > if (log_file =3D=3D -1) > die_perror("Couldn't open log file %s", path); > =20 > diff --git a/pcap.c b/pcap.c > index 6ee6cdf..a07eb33 100644 > --- a/pcap.c > +++ b/pcap.c > @@ -158,18 +158,15 @@ void pcap_iov(const struct iovec *iov, size_t iovcn= t, size_t offset) > */ > void pcap_init(struct ctx *c) > { > - int flags =3D O_WRONLY | O_CREAT | O_TRUNC; > - > if (pcap_fd !=3D -1) > return; > =20 > if (!*c->pcap) > return; > =20 > - flags |=3D c->foreground ? O_CLOEXEC : 0; > - pcap_fd =3D open(c->pcap, flags, S_IRUSR | S_IWUSR); > + pcap_fd =3D output_file_open(c->pcap); > if (pcap_fd =3D=3D -1) { > - perror("open"); > + err_perror("Couldn't open pcap file %s", c->pcap); > return; > } > =20 > diff --git a/util.c b/util.c > index 9cb705e..d838b34 100644 > --- a/util.c > +++ b/util.c > @@ -407,25 +407,19 @@ void pidfile_write(int fd, pid_t pid) > } > =20 > /** > - * pidfile_open() - Open PID file if needed > - * @path: Path for PID file, empty string if no PID file is requested > + * output_file_open() - Open file for output, if needed > + * @path: Path for output file > * > - * Return: descriptor for PID file, -1 if path is NULL, won't return on = failure > + * Return: file descriptor on success, -1 on failure with errno set by o= pen() > */ > -int pidfile_open(const char *path) > +int output_file_open(const char *path) > { > - int fd; > - > - if (!*path) > - return -1; > - > - if ((fd =3D open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, > - S_IRUSR | S_IWUSR)) < 0) { > - perror("PID file open"); > - exit(EXIT_FAILURE); > - } > - > - return fd; > + /* We use O_CLOEXEC here, but clang-tidy as of LLVM 16 to 19 looks for > + * it in the 'mode' argument if we have one > + */ > + return open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, > + /* NOLINTNEXTLINE(android-cloexec-open) */ > + S_IRUSR | S_IWUSR); > } > =20 > /** > diff --git a/util.h b/util.h > index 4f8b768..73b4a49 100644 > --- a/util.h > +++ b/util.h > @@ -193,7 +193,7 @@ char *line_read(char *buf, size_t len, int fd); > void ns_enter(const struct ctx *c); > bool ns_is_init(void); > int open_in_ns(const struct ctx *c, const char *path, int flags); > -int pidfile_open(const char *path); > +int output_file_open(const char *path); > void pidfile_write(int fd, pid_t pid); > int __daemon(int pidfile_fd, int devnull_fd); > int fls(unsigned long x); --=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 --srOK5SR6eZwB4ox8 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmcgY2IACgkQzQJF27ox 2Gcpfg/+NnMzqkw7X3OkLk1BYxOeJCF7V6LiMvIb7bpeisAuGvP6/+xZqbH1ocbD TVT9coMUbAmM4yg/8Nz+CByzIiy6NSXpoM+O1FMqkjbzEBouH1ZIrmTy6QRhP1Ia uo4T70+oI4bfrTORARuWzCVIUTqmrJ/b4vL98gZ4ExsRapsPePgdE7qiKHmtkncG wreH3feWwAt8KwKdLdNoXcvnQNkYU8SjQHDs9W0tXlLmuSf09oEmRrYB/hDdomLa pQdHtgWJUY6mZ1gm/tOZG2RS1J+QwsUPFP2ErdBkatuh6MIqtvMLm9z15XlRVUIs /ZPb3J55wht6cZ3B4VN3ZI1zE+VZP3OCm9TAqLB9fmts2xtL3cvL6w43O84zyhs5 oV7Kw4M3FbqRo2P89IBDCmno9jJOO+nTBvA7IzM7J4M9ixkG09Xzh8NkOzXIHbVo KkBj30HgvKTHD8RHA8XOpjvdH0o4N4PDxwzIVCYIdv0x4mkd2UGgkEw1tc6Qas1O 9zTw4463tGD5l6P0lrGQQaf3/i8gg7zRdo9owqM2fJ8CZQ1cBdqMPrYFjSREGh5Y Nt81MExgLr/QyWutFNTKezaMzgK65IZSJcVZaQWEgA5/WZ6gIjAIRjoK7v2ayd15 Wi3Xn4AP/T70L9fpMhTPYv7TwLTjQozURomUOJzPhwmXtbxMroI= =Mpsl -----END PGP SIGNATURE----- --srOK5SR6eZwB4ox8--