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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6378 bytes --]

On Tue, Nov 18, 2025 at 01:19:38AM +0100, Stefano Brivio wrote:
> 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.

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, ...

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

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

Arguably read_file_integer() is still too low-level to useful report
the error to the user.  But that would require a clunkier interface so
we can return an error code as well as the parsed value.

Saying it's falling back here would certainly be an improvement.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]