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=202502 header.b=gcysAHkK; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 49BB05A026F for ; Fri, 28 Mar 2025 00:36:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1743118600; bh=OILaCemJ1ioOW7UIXEGVG4xgZA/25YhvwA4oLEEt9ys=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gcysAHkK3PhEgLSVA6jSVL7DlD61w62OfUhO/9sKCqSXS5VPr4FxP+FG/A3a6DqrQ HJQxorphkRqiYNcFQZ+/tosZI7utrHilq92a6jJdcfMT1dbmIdJeTPj4qECxCCwUwz m57gbPLYh8/Sqck11yEmQEpVUCoXj8Ri9H+e/JHquWM3VzKqod29l93AUaYRXZsiIS kbjHCZNYj6HDm/RM2G/s2I8CeGxWHudaHu6VDo7wJ/6lPOJRL//Nlf2Igh0CrXEnSj 9w9SB22DeyKqEBJfjfDe0fm588IgWhf+cWED1Uzgo2XSlCqZKgWjBIk4Atz2PsEktp dqmRt/NmLZbzA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ZP0RX1s5wz4x1t; Fri, 28 Mar 2025 10:36:40 +1100 (AEDT) Date: Fri, 28 Mar 2025 10:36:39 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [RFC PATCH] pasta, passt-repair: Support multiple events per read() in inotify handlers Message-ID: References: <20250327212822.3315053-1-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XTyKzDsC75ScqQtl" Content-Disposition: inline In-Reply-To: <20250327212822.3315053-1-sbrivio@redhat.com> Message-ID-Hash: Q5SRMR6L2UOUDAAJV55UPKWSAK67T36I X-Message-ID-Hash: Q5SRMR6L2UOUDAAJV55UPKWSAK67T36I 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: --XTyKzDsC75ScqQtl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 27, 2025 at 10:28:22PM +0100, Stefano Brivio wrote: > The current code assumes that we'll get one event per read() on > inotify descriptors, but that's not the case, not from documentation, > and not from reports. >=20 > Add loops in the two inotify handlers we have, in pasta-specific code > and passt-repair, to go through all the events we receive. >=20 > While in pasta the the inotify watch is in an epoll list, and we'll "the the" > get epoll wakeups as long as there's data to read, in passt-repair > that's not the case. So, in pasta we can simply size the buffer for > a single event and try to read one, but in passt-repair, we'll need > to size the buffer to a safer, reasonable amount of events. I'm not following the reasoning here. In passt-repair we're just looping on the read() until we find what we want, so we'll still eventually get what we want. AFAICT it's a stream-like interface, so we won't lose events just because we didn't get them in the first read(). > Link: https://bugs.passt.top/show_bug.cgi?id=3D119 > Signed-off-by: Stefano Brivio Otherwise, LGTM. > --- > I'm posting this as RFC because, while it seems to do the job and I > tested all the code paths, Coverity isn't amused by the fact that > we assume that inotify 'name' attributes (ev->name) are > NULL-terminated. They actually are, but the code is not very robust. >=20 > Addressing that is kind of trivial but I keep getting it wrong, so > I'll start posting this and fix that up later (direct fixes/edits of > this patch are also welcome, of course). Ok, I might try to polish this up today >=20 > passt-repair.c | 26 +++++++++++++++++++------- > pasta.c | 20 +++++++++++++------- > 2 files changed, 32 insertions(+), 14 deletions(-) >=20 > diff --git a/passt-repair.c b/passt-repair.c > index 120f7aa..86581c6 100644 > --- a/passt-repair.c > +++ b/passt-repair.c > @@ -111,14 +111,14 @@ int main(int argc, char **argv) > } > =20 > if ((sb.st_mode & S_IFMT) =3D=3D S_IFDIR) { > - char buf[sizeof(struct inotify_event) + NAME_MAX + 1]; > + char buf[(sizeof(struct inotify_event) + NAME_MAX + 1) * 128] > + __attribute__ ((aligned(__alignof__(struct inotify_event)))); > const struct inotify_event *ev; > char path[PATH_MAX + 1]; > + bool found =3D false; > ssize_t n; > int fd; > =20 > - ev =3D (struct inotify_event *)buf; > - > if ((fd =3D inotify_init1(IN_CLOEXEC)) < 0) { > fprintf(stderr, "inotify_init1: %i\n", errno); > _exit(1); > @@ -130,6 +130,8 @@ int main(int argc, char **argv) > } > =20 > do { > + char *p; > + > n =3D read(fd, buf, sizeof(buf)); > if (n < 0) { > fprintf(stderr, "inotify read: %i", errno); > @@ -138,11 +140,21 @@ int main(int argc, char **argv) > =20 > if (n < (ssize_t)sizeof(*ev)) { > fprintf(stderr, "Short inotify read: %zi", n); > - _exit(1); > + continue; > + } > + > + for (p =3D buf; p < buf + n; p +=3D sizeof(*ev) + ev->len) { > + ev =3D (const struct inotify_event *)p; > + > + if (ev->len >=3D REPAIR_EXT_LEN && > + !memcmp(ev->name + strlen(ev->name) - > + REPAIR_EXT_LEN, > + REPAIR_EXT, REPAIR_EXT_LEN)) { > + found =3D true; > + break; > + } > } > - } while (ev->len < REPAIR_EXT_LEN || > - memcmp(ev->name + strlen(ev->name) - REPAIR_EXT_LEN, > - REPAIR_EXT, REPAIR_EXT_LEN)); > + } while (!found); > =20 > snprintf(path, sizeof(path), "%s/%s", argv[1], ev->name); > if ((stat(path, &sb))) { > diff --git a/pasta.c b/pasta.c > index fa3e7de..017fa32 100644 > --- a/pasta.c > +++ b/pasta.c > @@ -498,17 +498,23 @@ void pasta_netns_quit_init(const struct ctx *c) > */ > void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd) > { > - char buf[sizeof(struct inotify_event) + NAME_MAX + 1]; > - const struct inotify_event *in_ev =3D (struct inotify_event *)buf; > + char buf[sizeof(struct inotify_event) + NAME_MAX + 1] > + __attribute__ ((aligned(__alignof__(struct inotify_event)))); > + const struct inotify_event *ev; > + ssize_t n; > + char *p; > =20 > - if (read(inotify_fd, buf, sizeof(buf)) < (ssize_t)sizeof(*in_ev)) > + if ((n =3D read(inotify_fd, buf, sizeof(buf))) < (ssize_t)sizeof(*ev)) > return; > =20 > - if (strncmp(in_ev->name, c->netns_base, sizeof(c->netns_base))) > - return; > + for (p =3D buf; p < buf + n; p +=3D sizeof(*ev) + ev->len) { > + ev =3D (const struct inotify_event *)p; > =20 > - info("Namespace %s is gone, exiting", c->netns_base); > - _exit(EXIT_SUCCESS); > + if (!strncmp(ev->name, c->netns_base, sizeof(c->netns_base))) { > + info("Namespace %s is gone, exiting", c->netns_base); > + _exit(EXIT_SUCCESS); > + } > + } > } > =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 --XTyKzDsC75ScqQtl Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmfl4QIACgkQzQJF27ox 2Gf4DQ//XPcvt9XKwv1U5DwmqgJWrkkejFQZl4LvhDx9umxNVv9VUdxKpd25nM8a isyPvS0sOF82WMLKZDybyfSIOSsU2ntN4pOlEZaauv+xykKIW3DXC80L6lxZyq3k VdtdCXa6aPgy6KNmAohiYsTIekqDQs+ZWKZpFbX3B3nFOGiSlQ6sHqGU3qTNVb66 vJd7vUVXe0ciTqHZeddy0EXctyaY6gOkidn52PQ9puneihKt5wmwCNIsxfyCV0Qf mr5Ugfo7nbhESomjeD8qOndBenfyEA7QQB7/M/gkwuMCnA0JFOWVWficx8QcUNec O4ZiHKocCcWD6xsPWJ51wpWFix8v/d9im1KexHKrDUJP5yE1Ncb3z7gCCQavtuFy aFD2Vp6iMOWHuGq8yeaHs19DyaEYkNWGDpwKslZC90X0r5IRtPMhKLidW9k4pfD7 UmTuktB7kwERuAnv4VD+TZvxaUH+eJBg7Z9NuYcKeX4GJYE0HpaHxFhGEn2j1yzk p9fygvgcX9zIuZgr60f8ExbjOlLKKWb6Bwdoba+/U8Uo13Xdad2KeJFyIFd2CMkc RxlHgampJ42cL+NoUBhFk7h3cgDcCbMEwPoJ0T4jdtlcWLrzxq29Rf85eC7Rkweh fixMeH86Y4iGSBxBzgIWg1AruI77WVxNDXHMemP5K5nCG6mGHfE= =vUfc -----END PGP SIGNATURE----- --XTyKzDsC75ScqQtl--