Re: [PATCH v3 2/4] util: Introduce read_file() and read_file_long() function

15 Oct 2025

On Wed, Oct 15, 2025 at 7:28 AM David Gibson
 wrote:
...
On Tue, Oct 14, 2025 at 03:38:34PM +0800, Yumei Huang wrote:
...
Signed-off-by: Yumei Huang 
---
 util.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 util.h |  2 ++
 2 files changed, 94 insertions(+)

diff --git a/util.c b/util.c
index c492f90..d331f08 100644
--- a/util.c
+++ b/util.c
@@ -579,6 +579,98 @@ 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
+*/
Looks ok, but I think there's a simpler way.
...
+int 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;
+     bool truncated = false;
+
+     if (fd < 0) {
+             warn_perror("Could not open %s", path);
+             return -1;
+     }
+
+     while (total_read < buf_size - 1) {
+             rc = read(fd, buf + total_read, buf_size - 1 - total_read);
The '- 1' is to leave space for the \0, but if you instead attempt to
read the entire buffer...
...
+
+             if (rc < 0 ) {
(nit: extra space before ')')
...
+                     warn_perror("Couldn't read from %s", path);
+                     close(fd);
+                     return -1;
+             }
+
+             if (rc == 0) {
+                     break;
+             }
+
+             total_read += rc;
+
+             if (total_read == buf_size - 1) {
+                     char test_byte;
+                     rc = read(fd, &test_byte, 1);
+                     if (rc >0) {
+                             truncated = true;
+                             warn_perror("File %s truncated, buffer too small", path);
+                     }
+             }
...then you can tell if you have to truncate by finishing the loop
then checking if (total_read < buf_size).  If it is, there's space for
the \0, otherwise there isn't and you report truncation.  No need for
test_byte.
Yeah, that's much simpler. Will update in v4.
...
...
+     }
+
+     close(fd);
+
+     if (total_read < buf_size){
+             buf[total_read] = '\0';
And if you test for truncation and exit early, you can  make this
unconditional.
Agree.
...
...
+     }
+
+     return truncated ? -2 : (int)total_read;
+}
+
+/**
+ * read_file_long() - Read a long integer value from a file
When I first read this name I thought it was for reading a long file,
rather than reading a long (int) from a file.  Not immediately sure
how to clarify that.  read_file_long_int() is clear, but awkward.
A better choice might be to change this to use strtoimax() and call it
read_file_integer().
Good point. Will use strtoimax() and return intmax_t.
...
...
+ * @path: Path to the sysctl file
+ * @fallback: Default value if file can't be read
+ *
+ * Return: Parameter value, fallback on failure
+*/
+long read_file_long(const char *path, long fallback)
+{
+        char buf[32];
Rather than just using a semi-arbitrary 32 here, I'd suggest defining
a new constant similar to UINT16_STRLEN.  Except that's trickier for a
type that doesn't have a known fixed width.  Pity the C library
doesn't have constants for these AFAICT.
I will just define a UINTMAX_STRLEN with (sizeof("2147483647")).
...
...
+        char *end;
+        long value;
+        int bytes_read;
+
+        bytes_read = read_file(path, buf, sizeof(buf));
+        if (bytes_read < 0) {
+                debug("Unable to read %s", path);
If there's a an error on open() or read(), this will produce two very
similar error messages in a row, which isn't ideal.
...
+                return fallback;
+        }
+
+        if (bytes_read == 0) {
+                debug("Empty file %s", path);
+                return fallback;
+        }
Might be worth checking strtol()'s behaviour on an empty string to see
if this special case would already be handled below.
Checked both strtol() and strtoimax(), if the string is empty, it will
return 0 and set end to buf, which is \0, and errno remains unchanged.
So it's not handled below.
...
...
+
+        errno = 0;
+        value = strtol(buf, &end, 10);
+        if (*end && *end != '\n') {
+                debug("Invalid format in %s", path);
+                return fallback;
+        }
+        if (errno || value < 0 || value > LONG_MAX) {
No need to exclude negative values here.  (value > LONG_MAX) can never
be true since value is a long.
...
+                debug("Invalid value in %s: %ld", path, value);
If errno != 0, value might be uninitialised here, and certainly won't
have something useful.  Better to print the contents as a string.
Right.
...
...
+                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..e509bec 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);
+long read_file_long(const char *path, long 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);
--
2.47.0
--
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

    