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

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 10 Nov 2025 17:31:33 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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.

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:

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

...say something here, like "using %i as default value", so that it's
clear that the error doesn't have further consequences.

We should probably do that for all the failure cases here, so you might
want to 'goto error;' here, and also:

> +
> +	if (bytes_read == 0) {
> +		debug("Empty file %s", path);

here, and below, and then:

> +		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;

error:
	debug("Using ...")
	return fallback;

> +}
> +
>  #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