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

[Yumei Huang <yuhuang@redhat.com>]

On Fri, Oct 17, 2025 at 7:16 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Fri, Oct 17, 2025 at 12:22:14AM +0200, Stefano Brivio wrote:
> > 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>
> [snip]
> > > > > +     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.
>
> It does, and we should :).

Checked write_file(), seems the return value is not the same to
read_file(). It returns 0 or 1 depending on success or failure. So int
works fine here. I will update read_file() only.
>
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * 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/.
>
> If you want to update CONTRIBUTING.md to cover this, Yumei, that would
> be much appreciated.

Sure, will do it.
>
> > > > > +     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
>
> Works for me.
>
> > If it's sourced from https://stackoverflow.com/a/10536254 and comment,
> > don't forget to mention that in whatever implementation / commit
> > message.

Actually, it's a suggestion from Claude and I double checked the logic and math.
Maybe I should mention Claude and the logic in the commit message instead?

>
> Good point.
>
> > 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 already handles the case where the buffer isn't big enough (in
> read_file()).  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).
>
> > It's not a security matter, because if the kernel was compromised,
> > we're compromised too, simply a matter of robustness.
> >
> > --
> > 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



-- 
Thanks,

Yumei Huang