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=202602 header.b=WpvBH82i; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 46F165A0265 for ; Tue, 02 Jun 2026 03:28:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1780363725; bh=WByIwVN++Z/XwaIEJWfpUoNJFzrf3g2ES1GTpphPax0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WpvBH82ietRs1FSAJzJ2uJMWqPjb9pWevlPKq2YsQFoYs0170OMXwqZH15Vim9d8f fY8csatnGdkv2mHFEyNCgmkTVRla3vr+Zl1t0eobhthuRoWTX8+p3dJ4cF6yNNBwiw TxzLqD48WKos83xhI09bxSmSVKdpBMLMKqWBROnDNZfEKLn32CteUFeiGmevF7d6Vm h13zpCDuen8XHvSsJi99MMG6uQrUbJLIyZGtkxE6OV32tYotdaBy31uNny9/VjhG5R 35GmsE200N9u9t9OwgCX5QeB+fcwmUjB5pX1g68SW8JDUzrBC4D0PLWqQgi1xwXBwp TdNaWqFr7JUDw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gTtWx09DDz4w9g; Tue, 02 Jun 2026 11:28:45 +1000 (AEST) Date: Tue, 2 Jun 2026 11:28:40 +1000 From: David Gibson To: Jon Maloy Subject: Re: [PATCH] util, passt: Close daemon-lifetime fds on exit to avoid Coverity warning Message-ID: References: <20260531201324.1714921-1-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="qdiIbStcdq/bbFyT" Content-Disposition: inline In-Reply-To: <20260531201324.1714921-1-jmaloy@redhat.com> Message-ID-Hash: 2MCJ6H62QOTU2ALOSBFQETL37UNA6RML X-Message-ID-Hash: 2MCJ6H62QOTU2ALOSBFQETL37UNA6RML 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: sbrivio@redhat.com, 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: --qdiIbStcdq/bbFyT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, May 31, 2026 at 04:13:24PM -0400, Jon Maloy wrote: > conf_open_files() opens four file descriptors (fd_tap_listen, > fd_repair_listen, pidfile_fd, fd_control_listen) that are held for > the entire daemon lifetime. Because no close() call exists for them > anywhere, Coverity flags each as INCOMPLETE_DEALLOCATOR. This is > clearly a false positive, but we still want to get rid of this > warning. A bit frustrating that Coverity doesn't recognize an exit() will close all remaining files, but ok. I'm ok with close()ing these at exit... but some details remain. First, for pidfile_fd specifically, we shouldn't need that after we've written the pid. So we should just close() at that point, rather than waiting until exit. >=20 > We now register the execution context so that passt_exit() can use > to close these descriptors before calling _exit(). All exit paths > (signal handler, die(), die_perror()) funnel through passt_exit(), > so this covers all cases. passt_exit() is the right place for this, but registering an actual global to point at a local that's sort of a global seems pretty perverse. It would make more sense to convert the main struct ctx itself to a global, rather than a main()-local. Note that I'm not suggesting changing all the places that use it (current or future) - I think it's still preferable for those (excepting I guess main() itself) to access it via a parameter. We can encourage that by using a bulky name for the global. I see that setting 'exit_cleanup_ctx' where you do also serves one other purpose - it stops early passt_exit()s from attempting to close() those fds before they're initialised, even to -1. We can fix that by using an initialiser on the global to set fd fields to -1 before main(), which is arguably more elegant than the cluster of assignments at the top of main() anyway. >=20 > Signed-off-by: Jon Maloy > --- > log.h | 2 ++ > passt.c | 1 + > util.c | 22 ++++++++++++++++++++++ > 3 files changed, 25 insertions(+) >=20 > diff --git a/log.h b/log.h > index 69cfb507..079f429c 100644 > --- a/log.h > +++ b/log.h > @@ -63,7 +63,9 @@ extern bool debug_flag; > /* This would make more sense in util.h, but because we use it in die(),= that > * would cause awkward circular reference problems. > */ > +struct ctx; > void passt_exit(int status) __attribute__((noreturn)); > +void passt_exit_set_ctx(struct ctx *c); > =20 > #define LOGFILE_SIZE_DEFAULT (1024 * 1024UL) > #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */ > diff --git a/passt.c b/passt.c > index b6fc12d4..ec6aa57a 100644 > --- a/passt.c > +++ b/passt.c > @@ -392,6 +392,7 @@ int main(int argc, char **argv) > sock_probe_features(&c); > =20 > conf(&c, argc, argv); > + passt_exit_set_ctx(&c); > trace_init(c.trace); > =20 > pasta_netns_quit_init(&c); > diff --git a/util.c b/util.c > index b64c29ed..f15b1f9a 100644 > --- a/util.c > +++ b/util.c > @@ -1097,6 +1097,17 @@ void abort_with_msg(const char *fmt, ...) > abort(); > } > =20 > +static struct ctx *exit_cleanup_ctx; > + > +/** > + * passt_exit_set_ctx() - Register context for cleanup on exit > + * @c: Execution context > + */ > +void passt_exit_set_ctx(struct ctx *c) > +{ > + exit_cleanup_ctx =3D c; > +} > + > /** > * passt_exit() - Perform vital cleanup and exit > * > @@ -1108,6 +1119,17 @@ void abort_with_msg(const char *fmt, ...) > */ > void passt_exit(int status) > { > + if (exit_cleanup_ctx) { > + if (exit_cleanup_ctx->fd_tap_listen >=3D 0) > + close(exit_cleanup_ctx->fd_tap_listen); > + if (exit_cleanup_ctx->fd_repair_listen >=3D 0) > + close(exit_cleanup_ctx->fd_repair_listen); > + if (exit_cleanup_ctx->pidfile_fd >=3D 0) > + close(exit_cleanup_ctx->pidfile_fd); > + if (exit_cleanup_ctx->fd_control_listen >=3D 0) > + close(exit_cleanup_ctx->fd_control_listen); > + } > + > /* Make sure we don't leave the pcap file truncated */ > if (pcap_fd !=3D -1 && fsync(pcap_fd)) > warn_perror("Failed to flush pcap file, it might be truncated"); > --=20 > 2.52.0 >=20 --=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 --qdiIbStcdq/bbFyT Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmoeMccACgkQzQJF27ox 2GfYNA/6AiDLFuRbwIeHj1LgWj1KivkEOwBCzzdknoj8R6s56wJPERDoYlfG9q8c 4k9Wi7tijlZdSf5aLclqA4but2Ro6e1Rnt6w5cQ7vCcnzN41OeYXkeUakiWYQinY /rtq1I7NQHy9z00rAc0RvI3hj680IJWfxintryd7PU1whtIjpwnomGhSWlzi5RMZ qxzFjAmanwiBhYDN7pmnbDKrjY/8lI5HO9JoYCOJBU0Cov4zp4P3vRBIdoIAXjBA yTWk2kXUzYWhrwdOe47bergj2RV9Yd0ja1NM0Z1td07cO/P9t0Llo2sGeT0VpaDn XWE/P+yJ34otpcKsDZEH7YINUFYRX3Wv9sm7lE2CxbQJLQnTl6MVBQNSvHw3NgQz 1etyNImC6g3N3VpdtnPT/23lz9vRc0jOh5t8Prq91n/MPKAlMf2I9bnxUivwTm9J Wqx0vrRpxsFWSwZvi48EJQL/4X0d02jer5aHtmgG0Talk7s8xognV+ACN+Oww3ib MNl77PPNJHA+DhE36D1kCL/Y3rBtKjRgTcDHAsjkoF5sLmzsaupCn2a8hXKMqRWO ytYM7AsYOGPwuUdqcqlzFVO3GVVrT2sK8Z/H1uZFT5xHw+qyhKvYkG2PANgbymcs BB/T8mnUxTUbhTzWAXmALJMdKAQNLtJMPSHG4E+BixyO2FgCHso= =VJ4c -----END PGP SIGNATURE----- --qdiIbStcdq/bbFyT--