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

On Wed, Nov 19, 2025 at 05:04:31PM +0800, Yumei Huang wrote:

On Tue, Nov 18, 2025 at 11:36 AM David Gibson wrote:

...

On Tue, Nov 18, 2025 at 01:19:38AM +0100, 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);

While testing something unrelated I realised that this looks like a serious failure, but it's perfectly normal on (even slightly) older kernels:

--- $ git describe --contains ccce324dabfe # tcp: make the first N SYN RTO backoffs linear v6.5-rc1~163^2~299 $ git describe --contains 1280c26228bd # tcp: add tcp_rto_max_ms sysctl v6.15-rc1~160^2~352^2 ---

On a 6.1 kernel, for example:

--- $ ./pasta -d --config-net [...] 0.0258: Could not open /proc/sys/net/ipv4/tcp_syn_linear_timeouts: No such file or directory 0.0258: Could not open /proc/sys/net/ipv4/tcp_rto_max_ms: No such file or directory 0.0258: Read sysctl values syn_retries: 6, syn_linear_timeouts: 4, rto_max: 120 ---

and actually the third message isn't accurate, because we didn't read those values, we are just using them.

Right. In fact I'd say here the fact they came from sysctl is less important here than what we're using them for. So maybe Using TCP RTO parameters, syn_retries: 6, ...

Got it.

...

...

Worse, yet:

--- $ ./pasta Could not open /proc/sys/net/ipv4/tcp_syn_linear_timeouts: No such file or directory Could not open /proc/sys/net/ipv4/tcp_rto_max_ms: No such file or directory ---

so this would be logged *with default options* by Podman, rootlesskit / Docker, via libvirt, and by other users too, meaning we would get submerged by reports about this "error" if we release it like this (6.15 is fairly recent).

So maybe we could turn this (and the messages below) to debug() / debug_perror(), and then:

I think we should remove the error message from read_file() entirely, just returning an error code. Essentially it is too low level to know the severity and meaning of a failure, so reporting the problem to the user is better left to the valler.

Then we will have a few more values to return, like errno for open or read file fails, -ENOBUFS, and another error code for when buf size is 0. I'd say it's more complicated than write_fiel(). Maybe we could update with debug()/debug_perror() first as Stefano suggested, and then fix them together later?

Right, returning full error details is somewhere between awkward and impossible in C. I think returning -errno for the first error to occur is probably good enough, though. -- 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