Re: [PATCH v8 2/6] util: Introduce read_file() and read_file_integer() function

On Fri, Nov 14, 2025 at 6:12 PM Stefano Brivio wrote:

On Fri, 14 Nov 2025 09:58:57 +0800 Yumei Huang wrote:

...

On Fri, Nov 14, 2025 at 8:01 AM Stefano Brivio wrote:

...

On Mon, 10 Nov 2025 17:31:33 +0800 Yumei Huang wrote:

...

Signed-off-by: Yumei Huang Reviewed-by: David Gibson --- util.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 2 ++ 2 files changed, 88 insertions(+)

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 == 0 ? 0 : -1; }

+/** + * 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 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);

cppcheck rightfully says that:

util.c:604:10: style: The scope of the variable 'rc' can be reduced. [variableScope] ssize_t rc; ^

Right. Seems it also says:

tcp.c:2814:0: style: The function 'tcp_get_rto_params' should have static linkage since it is not used outside of its translation unit. [staticFunction] void tcp_get_rto_params(struct ctx *c) ^ util.c:601:0: style: The function 'read_file' should have static linkage since it is not used outside of its translation unit. [staticFunction] ssize_t read_file(const char *path, char *buf, size_t buf_size)

Oops, my current version (2.16.0) doesn't say that, I should upgrade it (but I typically try to remain a few versions behind as David usually upgrades right away, so that we catch also differences).

...

I understand read_file() may be called from other places in the future. But do we need to add static now? I guess we need it for tcp_get_rto_params().

On top of what David said, I'm not sure if we'll ever need it in tcp_get_rto_params(): I guess we'll always want to read integers there.

I meant we need to add static for tcp_get_rto_params(). But I got your point. I will add static for both the functions.

By the way, don't forget to drop the prototype from util.h as it's not needed if you make it static.

Thanks for the reminder!

...

...

...

+ + 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 contents exceed buffer size %zu", path, + buf_size); + buf[buf_size - 1] = '\0';

I suggested we need this, but Coverity Scan points out that:

--- /home/sbrivio/passt/util.c:631:3: Type: Overflowed constant (INTEGER_OVERFLOW)

/home/sbrivio/passt/util.c:606:2: 1. path: Condition "fd < 0", taking false branch. /home/sbrivio/passt/util.c:611:2: 2. path: Condition "total_read < buf_size", taking false branch. /home/sbrivio/passt/util.c:628:2: 3. path: Condition "total_read == buf_size", taking true branch. /home/sbrivio/passt/util.c:631:3: 4. overflow_const: Expression "buf_size - 1UL", where "buf_size" is known to be equal to 0, underflows the type of "buf_size - 1UL", which is type "unsigned long". ---

Somehow my Coverity Scan didn't complain about that.

Did you forget to pass '--security' to cov-analyze perhaps?

Yep, sorry.

...

...

in the (faulty) case where somebody calls this with 0 as buf_size.

On the other hand, the passed value of buf_size might be a result of a wrong calculation, and in that case we don't want to write some unrelated value on the stack of the caller or smash the stack.

We could ASSERT(buf_size), but in the future we might abuse read_file() to just check that a file is there and can be read, instead of actually reading it.

So maybe we could just return (after closing fd) before read() on !buf_size?

Sure. I can add that.

...

...

+ return -ENOBUFS; + } + + buf[total_read] = '\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 = 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("Non-numeric content in %s", path); + return fallback; + } + if (errno) { + debug("Out of range 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 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, size_t skip); int read_all_buf(int fd, void *buf, size_t len);

-- Stefano

-- Thanks, Yumei Huang