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

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

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

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

> > > > +}
> > > > +
> > > > +/**
> > > > + * 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.

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

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

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