git/list[1] front-page[2] threads[3] people[4] search[5] about
 

RE: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.

From
rsbecker@nexbridge.com <rsbecker@nexbridge.com>
Date
Feb 27, 2024, 19:04 UTC
Message-ID
<03be01da69af$d8366e10$88a34a30$@nexbridge.com>
In-Reply-To
<xmqq1q8xspht.fsf@gitster.g>
On Tuesday, February 27, 2024 1:59 PM, Junio C Hamano wrote:
Show 10 quoted lines
>"Randall S. Becker" <the.n.e.key@gmail.com> writes:
>
>> 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 if the supplied len value exceeds the supported value.
>> Replacing xwrite with write_in_full corrects this problem. Future
>> optimisations could remove the loop in favour of just calling
write_in_full.
Show 25 quoted lines
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>>  builtin/unpack-objects.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index
>> e0a701f2b3..6935c4574e 100644
>> --- a/builtin/unpack-objects.c
>> +++ b/builtin/unpack-objects.c
>> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char
>> **argv, const char *prefix UNUSED)
>>
>>  	/* Write the last part of the buffer to stdout */
>>  	while (len) {
>> -		int ret = xwrite(1, buffer + offset, len);
>> +		int ret = write_in_full(1, buffer + offset, len);
>>  		if (ret <= 0)
>>  			break;
>>  		len -= ret;
>
>Why do we need this with a retry loop that is prepared for short
>write(2) specifically like this?
>
>If xwrite() calls underlying write(2) with too large a value, then your
MAX_IO_SIZE is misconfigured, and the fix should go there, not
>here in a loop that expects a working xwrite() that is allowed to return on
short write(2), I would think.

I experimented with using write_in_full vs. keeping xwrite. With xwrite in this loop, t7704.9 consistently fails as described in the other thread. With write_in_full, the code works correctly. I assume there are side-effects that are present. This change is critical to having the code work on NonStop. Otherwise git seems to be at risk of actually being seriously broken if unpack does not work correctly. I am happy to have my series ignored as long as the problem is otherwise corrected.

Previous: Junio C HamanoNext: Jeff King
Message 8 of 12 in “Change xwrite() to write_in_full() in builtins.”
  1. 0/2 Change xwrite() to write_in_full() in builtins.Randall S. Becker, Feb 27, 2024
  2. 1/3 builtin/repack.c: change xwrite to write_in_full and report errors.Randall S. Becker, Feb 27, 2024
  3. Junio C HamanoFeb 27, 2024
  4. 2/3 builtin/receive-pack.c: change xwrite to write_in_full.Randall S. Becker, Feb 27, 2024
  5. Junio C HamanoFeb 27, 2024
  6. 3/3 builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.Randall S. Becker, Feb 27, 2024
  7. Junio C HamanoFeb 27, 2024
  8. rsbecker@nexbridge.comFeb 27, 2024
  9. Jeff KingFeb 27, 2024
  10. rsbecker@nexbridge.comFeb 27, 2024
  11. Jeff KingMar 7, 2024
  12. rsbecker@nexbridge.comFeb 27, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.