Re: [PATCH 01/19] reftable/stack: stop using `read_in_full()`

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 28, 2025, 08:06 UTC
Message-ID: <Z5iQH4C2JqazaJUl@pks.im>
In-Reply-To: <u7hbqd6at3owgka4pitd4vgzc5emgipnx3w4dat5ofpiv5hxxo@lbfj36uhqtsr>

On Mon, Jan 27, 2025 at 10:57:20AM -0600, Justin Tobler wrote:

Show 37 quoted lines

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

Show 5 quoted lines

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