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=NCTUSDYm; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 732AC5A004C for ; Wed, 30 Oct 2024 01:34:27 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202410; t=1730248449; bh=lFhmhJl7ER0ESu0X6a3/dPwb7/KKuNuI8gBaYvbUDu8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=NCTUSDYmHO7UzpsbkXMCuY2RQx21ocr0XZAv9OJ7puGgLBbwdaQZXFd/kdUTU13Pd S8bAQEdUvkImjwQJFa4dZ1/y7nPSksr1WnEkkvrqK9JSA2ttY8yTo8T1bNnYwvD/W5 20SyDE5DPS5228UwKXJXZfamaiSLWX4TAX4f9/gDpKk9+YaktCINPngcfFBES4wQ0S sMszwFdXACQNPDDzZ/BVhLLMKRGCb43sNnyb1hLmYbNSyZTn8vKq2gn2mN71flWNC5 X4OZfu8TP7/NXUZalC3SPYSdQwuQfV47sjafSNAOBvI9/6AQv8prYGvtvnKnNmzA4S F/3t550GabUxg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4XdSmd0spvz4x8g; Wed, 30 Oct 2024 11:34:09 +1100 (AEDT) Date: Wed, 30 Oct 2024 11:34:06 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v4 5/8] treewide: Suppress clang-tidy warning if we already use O_CLOEXEC Message-ID: References: <20241029112823.1386613-1-sbrivio@redhat.com> <20241029112823.1386613-6-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="0OT/3puhZZMc2kF1" Content-Disposition: inline In-Reply-To: <20241029112823.1386613-6-sbrivio@redhat.com> Message-ID-Hash: VMSUVSPE7WQUW4GFLSNRHY324FFXFYLU X-Message-ID-Hash: VMSUVSPE7WQUW4GFLSNRHY324FFXFYLU 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: --0OT/3puhZZMc2kF1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 29, 2024 at 12:28:20PM +0100, Stefano Brivio wrote: 61;7604;1c> In pcap_init(), we should always open the packet capture file w= ith > 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. >=20 > Add a suppression for clang-tidy and a comment, and avoid repeating > those three times by adding a new helper, output_file_open(). >=20 > Signed-off-by: Stefano Brivio Reviewed-by: David Gibson > --- > conf.c | 5 ++++- > log.c | 3 +-- > pcap.c | 7 ++----- > util.c | 27 +++++++++++---------------- > util.h | 2 +- > 5 files changed, 19 insertions(+), 25 deletions(-) >=20 > diff --git a/conf.c b/conf.c > index 4db7c64..d6faa5e 100644 > --- a/conf.c > +++ b/conf.c > @@ -1194,7 +1194,10 @@ 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) { > + if ((c->pidfile_fd =3D output_file_open(c->pidfile, 0)) < 0) > + die_perror("Couldn't open PID file %s", c->pidfile); > + } > } > =20 > /** > diff --git a/log.c b/log.c > index 6932885..0adddff 100644 > --- a/log.c > +++ b/log.c > @@ -416,8 +416,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_APPEND | O_RDWR | O_CLOEX= EC, > - S_IRUSR | S_IWUSR); > + log_file =3D output_file_open(path, O_APPEND); > 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..12737d8 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, 0); > 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..1ad3b5c 100644 > --- a/util.c > +++ b/util.c > @@ -407,25 +407,20 @@ 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 > + * @flags: Additional flags for open() > * > - * 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 flags) > { > - 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 | flags, > + /* NOLINTNEXTLINE(android-cloexec-open) */ > + S_IRUSR | S_IWUSR); > } > =20 > /** > diff --git a/util.h b/util.h > index 4f8b768..3fc64cf 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, int flags); > 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 --0OT/3puhZZMc2kF1 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmchfvoACgkQzQJF27ox 2GfXSA/+JLMSvpkUXAIBZ8S0YBBI2dbi4k+tWhnIZTce0L/pnEpg/qT4xgGmTf2+ r6+m8t0FlEhGQlRYpuEyY3MqYZ0rt58CDkKeLSEhN7wNADzu+7Ag20Kyu9f6UeDF biYzhGA9erKf38ODBE1qAyqiYYOY19/3NhcjBzZApQ+HKyypZ1mTTjRgfpL528yQ kr+Us19F0ymDm1EHiEpZVNl/5lmmAzUsGtP747nDxIbVJJH4XhIfoHlZ1lQoOmbn WyT4zpB2jjyE7QVBDJ8oS+aNssolK7a6t2GRV6SZs8+S2pOa3CpD3L+OtVf1v1R3 TSURPuqoHkP6nvIKdsEnHn9m++wzabN+MqldnM0tNXoIn6hwEUamRs6uAlAj2/U4 hknasYiZ/PWLox2MRbSDg9kM1YajhpGoN0Oj30JD89qqW6UUyosv6RdE+sQaEiE8 1GbFQ+VVxYRl02QhVXOs8kidwVhYhbNML4pOn4mwvABWNaRqKfPxaQxMmaxlCiwM AHo3zjccMJnDgcvMEugh6mYj4ywPU/mAHTdiqdKdvzj8/40qNCORPGvSeSLW/Tm4 OBBtWYWElqGKHMY62vF/0I2dTRGJYBELAkbdLUphlOynwUaUDYeUgf7kGddmvf8y FkdPEOcRURGp8yOm8RVXWEMPS9iB4gGUg0woCVMW4ApuJfjKbn4= =xfQp -----END PGP SIGNATURE----- --0OT/3puhZZMc2kF1--