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

On Tue, Oct 21, 2025 at 11:50:59PM +0200, Stefano Brivio wrote:

On Tue, 21 Oct 2025 17:32:58 +0800 Yumei Huang wrote:

...

On Sun, Oct 19, 2025 at 6:07 PM Stefano Brivio wrote:

...

On Fri, 17 Oct 2025 14:28:36 +0800 Yumei Huang wrote:

...

Signed-off-by: Yumei Huang --- util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 8 ++++++ 2 files changed, 92 insertions(+)

diff --git a/util.c b/util.c index c492f90..5c8c4bc 100644 --- a/util.c +++ b/util.c @@ -579,6 +579,90 @@ int write_file(const char *path, const char *buf) return len == 0 ? 0 : -1; }

+/** + * read_file() - Read contents of file into a buffer + * @path: File to read + * @buf: Buffer to store file contents + * @buf_size: Size of buffer + * + * Return: number of bytes read on success, -1 on any error, -2 on truncation +*/ +ssize_t read_file(const char *path, char *buf, size_t buf_size) +{ + int fd = open(path, O_RDONLY | O_CLOEXEC); + size_t total_read = 0; + ssize_t rc; + + if (fd < 0) { + warn_perror("Could not open %s", path); + return -1; + } + + while (total_read < buf_size) { + rc = 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 == 0) + break; + + total_read += rc; + } + + close(fd); + + if (total_read == buf_size) { + warn("File %s truncated, buffer too small", path); + return -2; + } + + buf[total_read] = '\0'; + + return total_read; +} + +/** + * 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]; + ssize_t bytes_read; + intmax_t value; + char *end; + + 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..3f9f296 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); +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, size_t skip); int read_all_buf(int fd, void *buf, size_t len); @@ -250,6 +252,12 @@ static inline const char *af_name(sa_family_t af)

#define UINT16_STRLEN (sizeof("65535"))

+/* Each byte expands to at most 3 decimal digits since 0xff == 255. + * Plus 2 extra bytes for the sign and null terminator. + * See https://stackoverflow.com/a/10536254.

This is not an acceptable form of attribution according to the CC BY-SA 3.0 terms. See:

https://stackoverflow.com/help/licensing https://creativecommons.org/licenses/by-sa/3.0/

and checksum.h in this tree for some examples of how to combine different licensing terms in a single file, in a way that's human-readable but still machine-friendly (for license / compliance scanners such as REUSE).

As I commented on a previous version, anyway, I don't think we need this at all. I guess my comment was ignored though.

I guess you meant the comment of suggesting using BUFSIZ in V2?

Right, but on v4, and that was just Friday for everybody involved...

...

David replied as quote:

"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)."

I wanted to reply to that because sure, BUFSIZ is typically 8192 bytes on glibc and 1024 with musl, but adding 10 or 8192 to the stack pointer doesn't really make a difference.

It's not like we allocate that memory anyway, and I don't think any of that memory (or unused holes on the stack we create) is prefetched. And regardless of all that... we don't use these functions on any data path, it's just during configuration. It can be (relatively) slow.

Right. Sorry, I didn't follow up yet, but after I wrote that, Stefano convinced me that BUFSIZ is fine. I originally suggested avoiding the arbitrary buffer largely because I thought having an INTMAX_STRLEN constant might be useful for other reasons too, but honestly, not very. -- 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