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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 21 Oct 2025 17:32:58 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> On Sun, Oct 19, 2025 at 6:07 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Fri, 17 Oct 2025 14:28:36 +0800
> > Yumei Huang <yuhuang@redhat.com> wrote:
> >  
> > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> > > ---
> > >  util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  util.h |  8 ++++++
> > >  2 files changed, 92 insertions(+)
> > >
> > > diff --git a/util.c b/util.c
> > > index c492f90..5c8c4bc 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
> > > +*/
> > > +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);
> > > +
> > > +             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 truncated, buffer too small", path);
> > > +             return -2;
> > > +     }
> > > +
> > > +     buf[total_read] = '\0';
> > > +
> > > +     return total_read;
> > > +}
> > > +
> > > +/**
> > > + * 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];
> > > +     ssize_t bytes_read;
> > > +     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("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..3f9f296 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);
> > > +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);
> > > @@ -250,6 +252,12 @@ static inline const char *af_name(sa_family_t af)
> > >
> > >  #define UINT16_STRLEN                (sizeof("65535"))
> > >
> > > +/* Each byte expands to at most 3 decimal digits since 0xff == 255.
> > > + * Plus 2 extra bytes for the sign and null terminator.
> > > + * See https://stackoverflow.com/a/10536254.  
> >
> > This is not an acceptable form of attribution according to the
> > CC BY-SA 3.0 terms. See:
> >
> >   https://stackoverflow.com/help/licensing
> >   https://creativecommons.org/licenses/by-sa/3.0/
> >
> > and checksum.h in this tree for some examples of how to combine
> > different licensing terms in a single file, in a way that's
> > human-readable but still machine-friendly (for license / compliance
> > scanners such as REUSE).
> >
> > As I commented on a previous version, anyway, I don't think we need
> > this at all. I guess my comment was ignored though.  
> 
> I guess you meant the comment of suggesting using BUFSIZ in V2?

Right, but on v4, and that was just Friday for everybody involved...

> David replied as quote:
> 
>  "We could use BUFSIZ, but it's massive overkill for
> reading a single integer: 8192 versus ~21 bytes (or ~42 bytes if
> intmax_t were 128-bit)."

I wanted to reply to that because sure, BUFSIZ is typically 8192 bytes
on glibc and 1024 with musl, but adding 10 or 8192 to the stack pointer
doesn't really make a difference.

It's not like we allocate that memory anyway, and I don't think any of
that memory (or unused holes on the stack we create) is prefetched. And
regardless of all that... we don't use these functions on any data
path, it's just during configuration. It can be (relatively) slow.

> It's my fault. I should have waited to see if you have more comments
> before sending the new version.

Thanks for understanding. My biggest problem with this is: where do I
comment now? On v4 or v6? Should I even read v5?

Well, anyway, I'll directly review v6 (in a bit) to keep things sane.

-- 
Stefano