Re: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 16 Oct 2025 15:49:39 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> On Thu, Oct 16, 2025 at 2:30 PM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote:  
> > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> > > ---
> > >  util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  util.h |  3 +++
> > >  2 files changed, 87 insertions(+)
> > >
> > > diff --git a/util.c b/util.c
> > > index c492f90..197677e 100644
> > > --- a/util.c
> > > +++ b/util.c
> > > @@ -579,6 +579,90 @@ 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
> > > +*/
> > > +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;
> > > +
> > > +     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);
> > > +
> > > +             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_perror("File %s truncated, buffer too small", path);
> > > +             return -2;
> > > +     }
> > > +
> > > +     buf[total_read] = '\0';
> > > +
> > > +     return (int)total_read;  
> >
> > Probably makes more sense for total_read and the return type to be ssize_t.  
> 
> Just tried to be consistent with write_file().  I can change it to
> ssize_t if needed.

ssize_t is the type designed for this, if write_file() has it wrong (I
didn't check), we should fix that as well.

> > > +}
> > > +
> > > +/**
> > > + * read_file_integer() - Read an integer value from a file
> > > + * @path: 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)
> > > +{
> > > +     char buf[INTMAX_STRLEN];
> > > +     char *end;  
> >
> > passt coding style is to list (where possible) local variables in
> > reverse order of line length, so this should go after bytes_read.  
> 
> Oh, I didn't notice that. Will update later.

Rationale (added to my further list for CONTRIBUTING.md):

  https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

and see also https://lwn.net/Articles/758552/.

> >  
> > > +     intmax_t value;
> > > +     int bytes_read;
> > > +
> > > +     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("Invalid format in %s", path);
> > > +             return fallback;
> > > +     }
> > > +     if (errno) {
> > > +             debug("Invalid 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 22eaac5..887d795 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);
> > > +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);
> > > @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af)
> > >  }
> > >
> > >  #define UINT16_STRLEN                (sizeof("65535"))
> > > +#define INTMAX_STRLEN                (sizeof("-9223372036854775808"))  
> >
> > It's correct for now, and probably for any systems we're likely to run
> > on, but I dislike hard-assuming the size of intmax_t here.  I feel
> > like there must be a better way to derive the correct string length,
> > but I haven't figured out what it is yet :(.  
> 
> How about this:
> 
>      #define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2)
> 
> Each byte can represent about 2.4 decimal digits as below,
> sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null
> terminator.
> 
>   1 bit = log₁₀(2) ≈ 0.30103 decimal digits
>   1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits

If it's sourced from https://stackoverflow.com/a/10536254 and comment,
don't forget to mention that in whatever implementation / commit
message.

But I was thinking... what if we keep it much simpler, use BUFSIZ, and
error out if the buffer is too small? It would be good to be robust
against any potential kernel issue anyway, so I think we need a
mechanism like that in any case.

It's not a security matter, because if the kernel was compromised,
we're compromised too, simply a matter of robustness.

-- 
Stefano