Re: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

17 Oct 2025

On Fri, Oct 17, 2025 at 7:16 AM 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 PM 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 == buf_size) {
+             warn_perror("File %s truncated, buffer too small", path);
+             return -2;
+     }
+
+     buf[total_read] = '\0';
+
+     return (int)total_read;
Probably makes more sense for total_read and the return type to be 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 :).
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.
...
...
...
...
...
+}
+
+/**
+ * read_file_integer() - Read an integer value from a file
+ * @path: 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)
+{
+     char buf[INTMAX_STRLEN];
+     char *end;
passt coding style is to list (where possible) local variables in
reverse order of line length, so this should go after bytes_read.
Oh, I didn't notice that. Will update later.
Rationale (added to my further list for CONTRIBUTING.md):
https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/
and see also https://lwn.net/Articles/758552/.
If you want to update CONTRIBUTING.md to cover this, Yumei, that would
be much appreciated.
Sure, will do it.
...
...
...
...
...
+     intmax_t value;
+     int bytes_read;
+
+     bytes_read = read_file(path, buf, sizeof(buf));
+
+     if (bytes_read < 0)
+             return fallback;
+
+     if (bytes_read == 0) {
+             debug("Empty file %s", path);
+             return fallback;
+     }
+
+     errno = 0;
+     value = strtoimax(buf, &end, 10);
+     if (*end && *end != '\n') {
+             debug("Invalid format in %s", path);
+             return fallback;
+     }
+     if (errno) {
+             debug("Invalid value in %s: %s", path, buf);
+             return fallback;
+     }
+
+     return value;
+}
+
 #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 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 iovcnt, 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("-9223372036854775808"))
It's correct for now, and probably for any systems we're likely to 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 length,
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 null
terminator.
1 bit = log₁₀(2) ≈ 0.30103 decimal digits
  1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits
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.
Actually, it's a suggestion from Claude and I double checked the logic and math.
Maybe I should mention Claude and the logic in the commit message instead?
...
Good point.
...
But I was thinking... what if we keep it much simpler, use BUFSIZ, and
error out if the buffer is too small? It would be good to be robust
against any potential kernel issue anyway, so I think we need a
mechanism like that in any case.
It already handles the case where the buffer isn't big enough (in
read_file()).  We could use BUFSIZ, but it's massive overkill for
reading a single integer: 8192 versus ~21 bytes (or ~42 bytes if
intmax_t were 128-bit).
...
It's not a security matter, because if the kernel was compromised,
we're compromised too, simply a matter of robustness.
--
Stefano
--
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
-- 
Thanks,

Yumei Huang

    