On Mon, Jan 27, 2025 at 10:57:20AM -0600, Justin Tobler wrote:
> On 25/01/27 02:04PM, Patrick Steinhardt wrote:
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index f7c1845e15..9490366795 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -115,13 +115,16 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
> >
> > static int fd_read_lines(int fd, char ***namesp)
> > {
> > - off_t size = lseek(fd, 0, SEEK_END);
> > char *buf = NULL;
> > int err = 0;
> > + off_t size;
> > +
> > + size = lseek(fd, 0, SEEK_END);
> > if (size < 0) {
> > err = REFTABLE_IO_ERROR;
> > goto done;
> > }
> > +
> > err = lseek(fd, 0, SEEK_SET);
> > if (err < 0) {
> > err = REFTABLE_IO_ERROR;
> > @@ -134,9 +137,16 @@ static int fd_read_lines(int fd, char ***namesp)
> > goto done;
> > }
> >
> > - if (read_in_full(fd, buf, size) != size) {
> > - err = REFTABLE_IO_ERROR;
> > - goto done;
> > + for (size_t total_read = 0; total_read < (size_t) size; ) {
>
> The cast from off_t -> size_t matches the currect behavior, but is it
> always safe to do this? In `git-compat-util.h` it looks like we have
> `xsize_t()` to safely handle these conversions. Since this series is
> moving away from `git-compat-util.h` should ideally something similar be
> implemented?
It is safe, because a couple lines further up we check for `size < 0`
and error out if that is the case. So we know it's a positive integer,
and thus it can be represented via `size_t`.
> > + ssize_t bytes_read = read(fd, buf + total_read, size - total_read);
> > + if (bytes_read < 0 && (errno == EAGAIN || errno == EINTR))
>
> The error handling here for EAGAIN doesn't go as far as what `xread()`
> does via `handle_nonblock()`. In this scenario is that ok?
Yes, because we don't set `O_NONBLOCK` in the reftable library.
I'll note that in the commit message.
Patrick
