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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 17 Oct 2025 10:44:42 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

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

Please mention that the next time instead of making it look like it's
yours.

> > > Maybe I should mention Claude and the logic in the commit message instead?  

Using whatever tool to copy code or descriptions of techniques doesn't
make you exempt from licensing requirements, see:

  https://stackoverflow.com/help/licensing

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

That's not enough. Just to be very clear, I'm not going to merge
anything that violates licensing requirements, no matter if some tool
hides them from you.

-- 
Stefano