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 10:30 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Fri, Oct 17, 2025 at 10:11:21AM +0800, Yumei Huang wrote:
> > 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.
>
> Ah, good point.
>
>
> [snip]
> > > > 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.
>
> Thanks.
>
> [snip]
> > > > > > > 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?
>
> Heh, so Claude got it from that stackoverflow page or one like it,
> probably.  It's not correctness we're concerned about, but proper
> attribution.  This is small enough that it's probably not
> copyrightable, but that would be a concern for larger segments as
> well.

Yeah, I understand. For most of the time, I only asked claude to
explain the code instead of writing the code directly. For this piece,
I will add a comment explaining the logic before the define line.
Thanks.
>
> --
> 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