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=rj/jCEI/; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id BB9805A0619 for ; Fri, 17 Oct 2025 04:30:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1760668211; bh=1VCZ/pJ689rs5HvS8I4E9vtZWlKdNSPF3scawqQlDxY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rj/jCEI/zPmBowsg4efaAqjwMoHvj0SuIUOyJm52SKB+hIdZo/g6tBvFwpqHE+TD5 1qV65yX1+E/9TEPcYPuDatE19TkxMCoHaLUCvPDcO4Cl+owYbjuT3vgUElmNnBx5eB dJzQWMwSSUWeO3gtsH54GBBY6UwuQU9D8fICiYU6CzMPJ1FDHGpZ9d7TEtdKq9AVQ+ Xc94BYF2XeLlPHS5azhRTE3K8hsjSeUWkMNrWAt0PNGphdfmf9xH4QZ6eCZOMX55Nw EaJE5EHd34lBazMkxh8jpzH2EFXyoblogaPJpQvk4jg0oAE4qhuTTWQsI1buXfMMk3 ZvSROKvFV4JeQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cnph34B4wz4wCx; Fri, 17 Oct 2025 13:30:11 +1100 (AEDT) Date: Fri, 17 Oct 2025 13:29:48 +1100 From: David Gibson To: Yumei Huang Subject: Re: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function Message-ID: References: <20251016023423.8923-1-yuhuang@redhat.com> <20251016023423.8923-3-yuhuang@redhat.com> <20251017002214.3fd4955b@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="cDeKyE/IxOynEiju" Content-Disposition: inline In-Reply-To: Message-ID-Hash: KOUZHET62V3C3DP2ULLO5FVCXHHRGGNX X-Message-ID-Hash: KOUZHET62V3C3DP2ULLO5FVCXHHRGGNX 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: Stefano Brivio , 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: --cDeKyE/IxOynEiju Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 17, 2025 at 10:11:21AM +0800, Yumei Huang wrote: > On Fri, Oct 17, 2025 at 7:16=E2=80=AFAM David Gibson > wrote: > > > > On Fri, Oct 17, 2025 at 12:22:14AM +0200, Stefano Brivio wrote: > > > On Thu, 16 Oct 2025 15:49:39 +0800 > > > Yumei Huang wrote: > > > > > > > On Thu, Oct 16, 2025 at 2:30=E2=80=AFPM David Gibson > > > > wrote: > > > > > > > > > > On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote: > > > > > > Signed-off-by: Yumei Huang > > [snip] > > > > > > + if (total_read =3D=3D buf_size) { > > > > > > + warn_perror("File %s truncated, buffer too small"= , path); > > > > > > + return -2; > > > > > > + } > > > > > > + > > > > > > + buf[total_read] =3D '\0'; > > > > > > + > > > > > > + return (int)total_read; > > > > > > > > > > Probably makes more sense for total_read and the return type to b= e ssize_t. > > > > > > > > Just tried to be consistent with write_file(). I can change it to > > > > ssize_t if needed. > > > > > > ssize_t is the type designed for this, if write_file() has it wrong (I > > > didn't check), we should fix that as well. > > > > It does, and we should :). >=20 > Checked write_file(), seems the return value is not the same to > read_file(). It returns 0 or 1 depending on success or failure. So int > works fine here. I will update read_file() only. Ah, good point. [snip] > > > Rationale (added to my further list for CONTRIBUTING.md): > > > > > > https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyra= mids/ > > > > > > and see also https://lwn.net/Articles/758552/. > > > > If you want to update CONTRIBUTING.md to cover this, Yumei, that would > > be much appreciated. >=20 > Sure, will do it. Thanks. [snip] > > > > > > diff --git a/util.h b/util.h > > > > > > index 22eaac5..887d795 100644 > > > > > > --- a/util.h > > > > > > +++ b/util.h > > > > > > @@ -222,6 +222,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); > > > > > > +int 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 io= vcnt, size_t skip); > > > > > > int read_all_buf(int fd, void *buf, size_t len); > > > > > > @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family= _t af) > > > > > > } > > > > > > > > > > > > #define UINT16_STRLEN (sizeof("65535")) > > > > > > +#define INTMAX_STRLEN (sizeof("-922337203685477= 5808")) > > > > > > > > > > It's correct for now, and probably for any systems we're likely t= o run > > > > > on, but I dislike hard-assuming the size of intmax_t here. I feel > > > > > like there must be a better way to derive the correct string leng= th, > > > > > but I haven't figured out what it is yet :(. > > > > > > > > How about this: > > > > > > > > #define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2) > > > > > > > > Each byte can represent about 2.4 decimal digits as below, > > > > sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and n= ull > > > > terminator. > > > > > > > > 1 bit =3D log=E2=82=81=E2=82=80(2) =E2=89=88 0.30103 decimal digi= ts > > > > 1 byte =3D 8 bits =3D 8 =C3=97 0.30103 =E2=89=88 2.408 decimal di= gits > > > > Works for me. > > > > > If it's sourced from https://stackoverflow.com/a/10536254 and comment, > > > don't forget to mention that in whatever implementation / commit > > > message. >=20 > Actually, it's a suggestion from Claude and I double checked the logic an= d math. > Maybe I should mention Claude and the logic in the commit message instead? Heh, so Claude got it from that stackoverflow page or one like it, probably. It's not correctness we're concerned about, but proper attribution. This is small enough that it's probably not copyrightable, but that would be a concern for larger segments as well. --=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 --cDeKyE/IxOynEiju Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjxqgkACgkQzQJF27ox 2Gda9w//SL0gkKl309AkpTtDYXcnSh67j9TV30Fk/0KBIwEphb2G+eaR89mKGnBS ViDd+JOla/Yb3wDskgZU20ETPlcGOXhswwqL+aVT7zbh/PGjluChnqehbT/qCK++ kgiZPHqtFZddriT1kfGRl8Fg8qoGPyvWRtqCOlq/ad/84cSO1lbMbQNJB3LE/3mn ER2b67GxN7fpdVHJFVWhDASaWFwEPaeJZ4QempseK6IE+NO+L1hXzdd+oqz+ZG6d 38FLpGx+4+ahKF71ByGFIsUYbG0ZhdDtGyipUOLpW4Xxrh+hwgbv92eDk0lPlSn2 1DBoYxhd5JKfembjjF9MELOmS99yK/OQFWKw5cmGlY4ZVygR+2OQCHVKgjS9Velu 1OYgaEIDBvBAyowRoKo9WiCs9Zy/XkRTHzs1QQnaXKYa9OERE32bWXdWV6Pqgp5g jZ/ghdiv9cZFjetefm3v77eqQB/OnC/45Khy7nfTNioL83O5W0/qDcLk6GzmfbUg GABV3xraLyyVvyTIrBTOD//hvrXPe+Okjyotlgf1Y0c5kEP1Jzi6EFhqY1RQcsZr 7ChCDkifVni7f0DwvvNkzFp3PX8+sY9Hf4g+KjflCz6yWwFZvGqtsggHVbDTJqhi tByAFi/aoCVyd8gqp2Qm+b/4kSgjUn/BXoii5KYI94nRvhl9JlE= =iGZA -----END PGP SIGNATURE----- --cDeKyE/IxOynEiju--