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=202510 header.b=yt2dLZPd; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 3635D5A0272 for ; Tue, 18 Nov 2025 04:35:58 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1763436954; bh=lSZwkclEb/6rxPH3/V2bnAWywi160bj0/SWj4sveVu4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=yt2dLZPdQRr1KVC9ZdsDLbGLGLug9zo07mPCoUt/rKUNYks502IfiA4h09haFfZvF s9LKVwl3C3tfBo+LPvLKlOXqRXqA82E998hdRQ0Melp2MUr5LoXZQkxSrSLnO/JjIp Br9iyC8NTsp8VAQ/bWf+B9x3yx0JZtwT4VMd8zPC9Ij2Gvm15ecGWxiHIur80XmW9b ruuTQugOKA8SvB+XBcsIIktXEC7GFwcpLCf0dxfiSUSv1mt/qs7x/hwdT7Fi4w01em YIwLfaey64IZV9nrCm0zmDV/4GzcrC0zt6WcxIzjCaedhdGggRx7uLiFhpxfzqaIyP VNJVun5qY2c+A== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4d9Vd614hNz58bQ; Tue, 18 Nov 2025 14:35:54 +1100 (AEDT) Date: Tue, 18 Nov 2025 14:22:40 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v8 2/6] util: Introduce read_file() and read_file_integer() function Message-ID: References: <20251110093137.87705-1-yuhuang@redhat.com> <20251110093137.87705-3-yuhuang@redhat.com> <20251118011938.47d5c1e2@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="xDyNwMKi5vQk8QE+" Content-Disposition: inline In-Reply-To: <20251118011938.47d5c1e2@elisabeth> Message-ID-Hash: 6KI2B46X6VJMLRORXWNWKTIEC2QVEKQY X-Message-ID-Hash: 6KI2B46X6VJMLRORXWNWKTIEC2QVEKQY 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: Yumei Huang , 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: --xDyNwMKi5vQk8QE+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 18, 2025 at 01:19:38AM +0100, Stefano Brivio wrote: > On Mon, 10 Nov 2025 17:31:33 +0800 > Yumei Huang wrote: >=20 > > Signed-off-by: Yumei Huang > > Reviewed-by: David Gibson > > --- > > util.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > util.h | 2 ++ > > 2 files changed, 88 insertions(+) > >=20 > > diff --git a/util.c b/util.c > > index 44c21a3..c4c849c 100644 > > --- a/util.c > > +++ b/util.c > > @@ -590,6 +590,92 @@ int write_file(const char *path, const char *buf) > > return len =3D=3D 0 ? 0 : -1; > > } > > =20 > > +/** > > + * read_file() - Read contents of file into a NULL-terminated buffer > > + * @path: Path to file to read > > + * @buf: Buffer to store file contents > > + * @buf_size: Size of buffer > > + * > > + * Return: number of bytes read on success, -1 on error, -ENOBUFS on t= runcation > > + */ > > +ssize_t read_file(const char *path, char *buf, size_t buf_size) > > +{ > > + int fd =3D open(path, O_RDONLY | O_CLOEXEC); > > + size_t total_read =3D 0; > > + ssize_t rc; > > + > > + if (fd < 0) { > > + warn_perror("Could not open %s", path); >=20 > While testing something unrelated I realised that this looks like a > serious failure, but it's perfectly normal on (even slightly) older > kernels: >=20 > --- > $ git describe --contains ccce324dabfe # tcp: make the first N SYN RTO ba= ckoffs linear > v6.5-rc1~163^2~299 > $ git describe --contains 1280c26228bd # tcp: add tcp_rto_max_ms sysctl > v6.15-rc1~160^2~352^2 > --- >=20 > On a 6.1 kernel, for example: >=20 > --- > $ ./pasta -d --config-net > [...] > 0.0258: Could not open /proc/sys/net/ipv4/tcp_syn_linear_timeouts: No suc= h file or directory > 0.0258: Could not open /proc/sys/net/ipv4/tcp_rto_max_ms: No such file or= directory > 0.0258: Read sysctl values syn_retries: 6, syn_linear_timeouts: 4, rto_ma= x: 120 > --- >=20 > and actually the third message isn't accurate, because we didn't read > those values, we are just using them. Right. In fact I'd say here the fact they came from sysctl is less important here than what we're using them for. So maybe Using TCP RTO parameters, syn_retries: 6, ... >=20 > Worse, yet: >=20 > --- > $ ./pasta > Could not open /proc/sys/net/ipv4/tcp_syn_linear_timeouts: No such file o= r directory > Could not open /proc/sys/net/ipv4/tcp_rto_max_ms: No such file or directo= ry > --- >=20 > so this would be logged *with default options* by Podman, rootlesskit / > Docker, via libvirt, and by other users too, meaning we would get > submerged by reports about this "error" if we release it like this (6.15 > is fairly recent). >=20 > So maybe we could turn this (and the messages below) to debug() / > debug_perror(), and then: I think we should remove the error message from read_file() entirely, just returning an error code. Essentially it is too low level to know the severity and meaning of a failure, so reporting the problem to the user is better left to the valler. > > + return -1; > > + } > > + > > + while (total_read < buf_size) { > > + rc =3D read(fd, buf + total_read, buf_size - total_read); > > + > > + if (rc < 0) { > > + warn_perror("Couldn't read from %s", path); > > + close(fd); > > + return -1; > > + } > > + > > + if (rc =3D=3D 0) > > + break; > > + > > + total_read +=3D rc; > > + } > > + > > + close(fd); > > + > > + if (total_read =3D=3D buf_size) { > > + warn("File %s contents exceed buffer size %zu", path, > > + buf_size); > > + buf[buf_size - 1] =3D '\0'; > > + return -ENOBUFS; > > + } > > + > > + buf[total_read] =3D '\0'; > > + > > + return total_read; > > +} > > + > > +/** > > + * read_file_integer() - Read an integer value from a file > > + * @path: Path to file to read > > + * @fallback: Default value if file can't be read > > + * > > + * Return: integer value, @fallback on failure > > + */ > > +intmax_t read_file_integer(const char *path, intmax_t fallback) > > +{ > > + ssize_t bytes_read; > > + char buf[BUFSIZ]; > > + intmax_t value; > > + char *end; > > + > > + bytes_read =3D read_file(path, buf, sizeof(buf)); > > + > > + if (bytes_read < 0) > > + return fallback; >=20 > ...say something here, like "using %i as default value", so that it's > clear that the error doesn't have further consequences. >=20 > We should probably do that for all the failure cases here, so you might > want to 'goto error;' here, and also: Arguably read_file_integer() is still too low-level to useful report the error to the user. But that would require a clunkier interface so we can return an error code as well as the parsed value. Saying it's falling back here would certainly be an improvement. > > + > > + if (bytes_read =3D=3D 0) { > > + debug("Empty file %s", path); >=20 > here, and below, and then: >=20 > > + return fallback; > > + } > > + > > + errno =3D 0; > > + value =3D strtoimax(buf, &end, 10); > > + if (*end && *end !=3D '\n') { > > + debug("Non-numeric content in %s", path); > > + return fallback; > > + } > > + if (errno) { > > + debug("Out of range value in %s: %s", path, buf); > > + return fallback; > > + } > > + > > + return value; >=20 > error: > debug("Using ...") > return fallback; >=20 > > +} > > + > > #ifdef __ia64__ > > /* Needed by do_clone() below: glibc doesn't export the prototype of _= _clone2(), > > * use the description from clone(2). > > diff --git a/util.h b/util.h > > index a0b2ada..c1502cc 100644 > > --- a/util.h > > +++ b/util.h > > @@ -229,6 +229,8 @@ void pidfile_write(int fd, pid_t pid); > > int __daemon(int pidfile_fd, int devnull_fd); > > int fls(unsigned long x); > > int write_file(const char *path, const char *buf); > > +ssize_t read_file(const char *path, char *buf, size_t buf_size); > > +intmax_t read_file_integer(const char *path, intmax_t fallback); > > int write_all_buf(int fd, const void *buf, size_t len); > > int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, si= ze_t skip); > > int read_all_buf(int fd, void *buf, size_t len); >=20 > --=20 > Stefano >=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 --xDyNwMKi5vQk8QE+ Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmkb5nAACgkQzQJF27ox 2Geyvg//WyhLcLAmJDrsxHM43PbSH2EM2RYr4DzgvHClf+96EJBamBwbdQdM1Kl0 OUS5rk37TL6DP3dn5tzQIrv/YhY/RuMpKOjWzNzHPRK6uhlAehveBFDDSDQ+1rNJ MQVaHlax1SNsX9MHl9F5PD7AuhIGHLCcMeN6cGIlT2IKps49WL5UQ6vaxU1BOqEQ 3v0yRbeOzYTWp/sOjsKilWeupllzU/nSD2PSboD9gmj5wSL7AmmMsci3JEc0J1Z1 JoUGJS6P3gWQ9yixPq0w49w//hZaDnom6grxlabrw7KnWDk7z6NUhRDj39LhfyPu 02xCqllhXLe/bu5UoulRAOM8D/ztAMTN6TZhPmFOSLmIYUGkLzaief/c+FRpR93a +nkLKRbF+kfz4w02a/MYBiWPP45y6ODGyZsM1bJNckjeSmnkuR2I1RDg7jaJ3Qcz Rau0ar4LHL7MsbSg/FYoi6SUuoBTf1bscoX/CsLCL9FNzoeOkQgqKpz4yn46TD0z sv62DSaVkT23Rh26a21BctyEn0r7FHeD5AAbAlfWEreBUPjHWYNoY3T2q89Gor83 tjL7CufCLgepQ32TaIVrRZPkM/NtBo6qM2rko9h69pHUjvkO7vIoBSa6NwLYVyuY tz/55FRPduT2gMPrw+lKzcGk45cQ9UsjtSV3gtR01Q1BD1HYBLo= =LL3k -----END PGP SIGNATURE----- --xDyNwMKi5vQk8QE+--