Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.

From: Jeff King <peff@peff.net>
Date: Feb 27, 2024, 08:20 UTC
Message-ID: <20240227082027.GH3263678@coredump.intra.peff.net>
In-Reply-To: <20240226220539.3494-4-randall.becker@nexbridge.ca>

On Mon, Feb 26, 2024 at 05:05:37PM -0500, Randall S. Becker wrote:

Show 6 quoted lines

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> This change is required because some platforms do not support file writes of
> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
> maximum single I/O size possible for the destination device. The result of
> write_in_full() is also passed to the caller, which was previously ignored.

These are going to be tiny compared to single-write() I/O limits, I'd think, but in general we should be on guard for the OS returning short reads (this is a pipe and so for most systems PIPE_BUF would guarantee atomicity, I think, but IMHO it is simpler to just make things obviously-correct by looping with write_in_full). So I'd be surprised if this spot was the cause of a visible bug, but I think it's worth changing regardless.

The error detection is a separate question, though. I think it is good to check the result of the write here, as an error here means that the child pack-objects misses some objects we wanted it to pack, which could lead to a corrupt repository. But I don't think what you have here is quite enough:

Show 13 quoted lines

> @@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,
>  			die(_("could not start pack-objects to repack promisor objects"));
>  	}
>  
> -	xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> -	xwrite(cmd->in, "\n", 1);
> +	err = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> +	if (err <= 0)
> +		return err;
> +	err = write_in_full(cmd->in, "\n", 1);
> +	if (err <= 0)
> +		return err;
>  	return 0;

OK, so we detect the error and return it to the caller. Who is the caller? The only use of this function is in repack_promisor_objects(), which calls:

        for_each_packed_object(write_oid, &cmd,
                               FOR_EACH_OBJECT_PROMISOR_ONLY);

So when we return the error, now for_each_packed_object() will stop traversing, and propagate that error up to the caller. But as we can see above, the caller ignores it!

So I think you'd either want to die directly (perhaps using write_or_die). Or you'd need to additionally check the return from for_each_packed_object(). That would also catch cases where that function failed to open a pack (I'm not sure how important that is to this code).

But as it is, your patch just causes a write error to truncate the list of oids send to the child process (though that is probably not materially different from the current behavior, as the subsequent calls would presumably fail, too).

-Peff