{"thread":{"id":"61012","subject":"[PATCH v1 0/4] Change xwrite() to write_in_full() in builtins.","startedAt":"2024-02-26T22:05:52Z","lastAt":"2024-02-27T08:22:54Z","messageCount":18,"participants":["Randall S. Becker","Taylor Blau","rsbecker@nexbridge.com","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"489434","messageId":"20240226220539.3494-1-randall.becker@nexbridge.ca","threadId":"61012","inReplyTo":null,"subject":"[PATCH v1 0/4] Change xwrite() to write_in_full() in builtins.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-26T22:05:34Z","receivedAt":"2024-02-26T22:05:52Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nThis series replaces xwrite to write_in_full in builtins/. The change is\nrequired to fix critical problems that prevent full writes to be\nprocessed by on platforms where xwrite may be limited to a platform size\nlimit. Further changes outside of builtins/ may be required but do not\nappear to be as urgent as this change, which causes test breakage in\nt7704. A separate series will be contributed for changes outside of\nbuiltins/ at a later date.\n\nRandall S. Becker (4):\n builtin/index-pack.c: change xwrite to write_in_full to allow large\n sizes.\n builtin/receive-pack.c: change xwrite to write_in_full to allow large\n sizes.\n builtin/repack.c: change xwrite to write_in_full to allow large sizes.\n builtin/unpack-objects.c: change xwrite to write_in_full to allow\n large sizes.\n\n builtin/index-pack.c | 2 +-\n builtin/receive-pack.c | 5 +++--\n builtin/repack.c | 9 +++++++--\n builtin/unpack-objects.c | 2 +-\n 4 files changed, 12 insertions(+), 6 deletions(-)\n\n-- \n2.42.1\n\n"},{"id":"489435","messageId":"20240226220539.3494-2-randall.becker@nexbridge.ca","threadId":"61012","inReplyTo":"20240226220539.3494-1-randall.becker@nexbridge.ca","subject":"[PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-26T22:05:35Z","receivedAt":"2024-02-26T22:05:53Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nThis change is required because some platforms do not support file writes of\narbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\nmaximum single I/O size possible for the destination device.\n\nSigned-off-by: Randall S. Becker \n---\n builtin/index-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a3a37bd215..f80b8d101a 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t * the last part of the input buffer to stdout.\n \t\t */\n \t\twhile (input_len) {\n-\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n+\t\t\terr = write_in_full(1, input_buffer + input_offset, input_len);\n \t\t\tif (err <= 0)\n \t\t\t\tbreak;\n \t\t\tinput_len -= err;\n-- \n2.42.1\n\n"},{"id":"489436","messageId":"20240226220539.3494-3-randall.becker@nexbridge.ca","threadId":"61012","inReplyTo":"20240226220539.3494-1-randall.becker@nexbridge.ca","subject":"[PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-26T22:05:36Z","receivedAt":"2024-02-26T22:05:54Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nThis change is required because some platforms do not support file writes of\narbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\nmaximum single I/O size possible for the destination device.\n\nSigned-off-by: Randall S. Becker \n---\n builtin/receive-pack.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex db65607485..5064f3d300 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -455,8 +455,9 @@ static void report_message(const char *prefix, const char *err, va_list params)\n \n \tif (use_sideband)\n \t\tsend_sideband(1, 2, msg, sz, use_sideband);\n-\telse\n-\t\txwrite(2, msg, sz);\n+\telse {\n+\t\twrite_in_full(2, msg, sz);\n+\t}\n }\n \n __attribute__((format (printf, 1, 2)))\n-- \n2.42.1\n\n"},{"id":"489437","messageId":"20240226220539.3494-4-randall.becker@nexbridge.ca","threadId":"61012","inReplyTo":"20240226220539.3494-1-randall.becker@nexbridge.ca","subject":"[PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-26T22:05:37Z","receivedAt":"2024-02-26T22:05:54Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nThis change is required because some platforms do not support file writes of\narbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\nmaximum single I/O size possible for the destination device. The result of\nwrite_in_full() is also passed to the caller, which was previously ignored.\n\nSigned-off-by: Randall S. Becker \n---\n builtin/repack.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ede36328a3..932d24c60b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,\n \t\t struct packed_git *pack UNUSED,\n \t\t uint32_t pos UNUSED, void *data)\n {\n+\tint err;\n \tstruct child_process *cmd = data;\n \n \tif (cmd->in == -1) {\n@@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,\n \t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n \t}\n \n-\txwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n-\txwrite(cmd->in, \"\\n\", 1);\n+\terr = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n+\tif (err <= 0)\n+\t\treturn err;\n+\terr = write_in_full(cmd->in, \"\\n\", 1);\n+\tif (err <= 0)\n+\t\treturn err;\n \treturn 0;\n }\n \n-- \n2.42.1\n\n"},{"id":"489438","messageId":"20240226220539.3494-5-randall.becker@nexbridge.ca","threadId":"61012","inReplyTo":"20240226220539.3494-1-randall.becker@nexbridge.ca","subject":"[PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-26T22:05:38Z","receivedAt":"2024-02-26T22:05:55Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nThis change is required because some platforms do not support file writes of\narbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\nmaximum single I/O size possible for the destination device.\n\nSigned-off-by: Randall S. Becker \n---\n builtin/unpack-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex e0a701f2b3..6935c4574e 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n \n \t/* Write the last part of the buffer to stdout */\n \twhile (len) {\n-\t\tint ret = xwrite(1, buffer + offset, len);\n+\t\tint ret = write_in_full(1, buffer + offset, len);\n \t\tif (ret <= 0)\n \t\t\tbreak;\n \t\tlen -= ret;\n-- \n2.42.1\n\n"},{"id":"489439","messageId":"Zd0S7aUIG1bhGkaX@nand.local","threadId":"61012","inReplyTo":"20240226220539.3494-2-randall.becker@nexbridge.ca","subject":"Re: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-02-26T22:38:37Z","receivedAt":"2024-02-26T22:38:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:\n> From: \"Randall S. Becker\" \n>\n> This change is required because some platforms do not support file writes of\n> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\n> maximum single I/O size possible for the destination device.\n\nHmm. I'm not sure I understand what NonStop's behavior is here...\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index a3a37bd215..f80b8d101a 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n> \t\t * the last part of the input buffer to stdout.\n> \t\t */\n> \t\twhile (input_len) {\n> -\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n> +\t\t\terr = write_in_full(1, input_buffer + input_offset, input_len);\n> \t\t\tif (err <= 0)\n> \t\t\t\tbreak;\n> \t\t\tinput_len -= err;\n> --\n> 2.42.1\n\nThe code above loops while input_len is non-zero, and correctly\ndecrements it by the number of bytes written by xwrite() after each\niteration.\n\nAssuming that xwrite()/write(2) works how I think it does on NonStop,\nI'm not sure I understand why this change is necessary.\n\nThanks,\nTaylor\n"},{"id":"489440","messageId":"026b01da6906$4d96f530$e8c4df90$@nexbridge.com","threadId":"61012","inReplyTo":"Zd0S7aUIG1bhGkaX@nand.local","subject":"RE: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-26T22:51:09Z","receivedAt":"2024-02-26T22:51:17Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, February 26, 2024 5:39 PM, Taylor Blau wrote:\n>On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:\n>> From: \"Randall S. Becker\" \n>>\n>> This change is required because some platforms do not support file\n>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating\n>> the output to the maximum single I/O size possible for the destination device.\n>\n>Hmm. I'm not sure I understand what NonStop's behavior is here...\n>\n>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c index\n>> a3a37bd215..f80b8d101a 100644\n>> --- a/builtin/index-pack.c\n>> +++ b/builtin/index-pack.c\n>> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>> \t\t * the last part of the input buffer to stdout.\n>> \t\t */\n>> \t\twhile (input_len) {\n>> -\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n>> +\t\t\terr = write_in_full(1, input_buffer + input_offset, input_len);\n>> \t\t\tif (err <= 0)\n>> \t\t\t\tbreak;\n>> \t\t\tinput_len -= err;\n>> --\n>> 2.42.1\n>\n>The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each\n>iteration.\n>\n>Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary.\n\nNonStop has a limited SSIZE_MAX. xwrite only handles that much so anything beyond that gets dropped (not in the above code but in other builtins); hence the critical nature of getting this fix out. This particular change probably could be tightened up on a re-roll to just call write_in_full instead of the while loop. I can fix that for v2. The goal suggested by Phillip W was to change xwrite to write_in_full, so I guess I went a little too far. \n\n\n\n"},{"id":"489442","messageId":"026c01da6907$d93297b0$8b97c710$@nexbridge.com","threadId":"61012","inReplyTo":"20240226220539.3494-3-randall.becker@nexbridge.ca","subject":"RE: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-26T23:02:13Z","receivedAt":"2024-02-26T23:02:20Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, February 26, 2024 5:06 PM, I wrote:\n>From: \"Randall S. Becker\" \n>\n>This change is required because some platforms do not support file writes\nof arbitrary sizes (e.g, NonStop). xwrite ends up truncating\n>the output to the maximum single I/O size possible for the destination\ndevice.\n>\n>Signed-off-by: Randall S. Becker \n>---\n> builtin/receive-pack.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n>diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index\ndb65607485..5064f3d300 100644\n>--- a/builtin/receive-pack.c\n>+++ b/builtin/receive-pack.c\n>@@ -455,8 +455,9 @@ static void report_message(const char *prefix, const\nchar *err, va_list params)\n>\n> \tif (use_sideband)\n> \t\tsend_sideband(1, 2, msg, sz, use_sideband);\n>-\telse\n>-\t\txwrite(2, msg, sz);\n>+\telse {\n>+\t\twrite_in_full(2, msg, sz);\n>+\t}\n> }\n>\n> __attribute__((format (printf, 1, 2)))\n>--\n>2.42.1\n\nThis needs to be fixed, so the {} after the else is removed. Will be in v2.\n\n"},{"id":"489444","messageId":"026f01da690b$d1d2a290$7577e7b0$@nexbridge.com","threadId":"61012","inReplyTo":"Zd0S7aUIG1bhGkaX@nand.local","subject":"RE: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-26T23:30:38Z","receivedAt":"2024-02-26T23:30:47Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, February 26, 2024 5:39 PM, Taylor Blau wrote:\n>On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:\n>> From: \"Randall S. Becker\" \n>>\n>> This change is required because some platforms do not support file\n>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating\n>> the output to the maximum single I/O size possible for the destination device.\n>\n>Hmm. I'm not sure I understand what NonStop's behavior is here...\n>\n>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c index\n>> a3a37bd215..f80b8d101a 100644\n>> --- a/builtin/index-pack.c\n>> +++ b/builtin/index-pack.c\n>> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>> \t\t * the last part of the input buffer to stdout.\n>> \t\t */\n>> \t\twhile (input_len) {\n>> -\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n>> +\t\t\terr = write_in_full(1, input_buffer + input_offset, input_len);\n>> \t\t\tif (err <= 0)\n>> \t\t\t\tbreak;\n>> \t\t\tinput_len -= err;\n>> --\n>> 2.42.1\n>\n>The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each\n>iteration.\n>\n>Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary.\n\nAfter thinking about it, I'm going to revert the change in this file, so it will not be in v2. I'm a bit uncomfortable with having the write sizes in global, so will drop this bit.\n\n"},{"id":"489445","messageId":"xmqqa5nmkcuz.fsf@gitster.g","threadId":"61012","inReplyTo":"026b01da6906$4d96f530$e8c4df90$@nexbridge.com","subject":"Re: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T23:46:44Z","receivedAt":"2024-02-26T23:46:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n>>The code above loops while input_len is non-zero, and correctly\n>>decrements it by the number of bytes written by xwrite() after\n>>each iteration.\n>>\n>>Assuming that xwrite()/write(2) works how I think it does on\n>>NonStop, I'm not sure I understand why this change is necessary.\n>\n> NonStop has a limited SSIZE_MAX. xwrite only handles that much so\n> anything beyond that gets dropped (not in the above code but in\n> other builtins)\n\nxwrite() caps a single write attempt to MAX_IO_SIZE and can return a\nshort-write, so anything beyound MAX_IO_SIZE will not even be sent\nto the underlying write(2). There is a heuristic based on the value\nof SSIZE_MAX to define MAX_IO_SIZE in , and if\nthe value given by that heuristics is too large for your platform,\nyou can tweak your own MAX_IO_SIZE (see the comments in that header\nfile).\n\nThe caller of xwrite() must be prepared to see a write return with\nvalue less than the length it used to call the function, either\nbecause of this MAX_IO_SIZE cut-off, or because of the underlying\nwrite(2) returning after a short write. As long as the caller is\nprepared, like Taylor pointed out, I am not sure why you'd need to\nchange it.\n"},{"id":"489446","messageId":"xmqq34tekcoo.fsf@gitster.g","threadId":"61012","inReplyTo":"20240226220539.3494-3-randall.becker@nexbridge.ca","subject":"Re: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T23:50:31Z","receivedAt":"2024-02-26T23:50:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" writes:\n\n> From: \"Randall S. Becker\" \n>\n> This change is required because some platforms do not support file writes of\n> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\n> maximum single I/O size possible for the destination device.\n\nAs msg[] here is 4k on-stack buffer, if the I/O size is small\nenough, the above may happen, and I think write-in-full is warranted\nhere. If your I/O must be done in 1k chunks, it would be very slow\nto run things like writing a pack stream to clone any non-toy\nprojects, though X-<.\n\n> Signed-off-by: Randall S. Becker \n> ---\n> builtin/receive-pack.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index db65607485..5064f3d300 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -455,8 +455,9 @@ static void report_message(const char *prefix, const char *err, va_list params)\n> \n> \tif (use_sideband)\n> \t\tsend_sideband(1, 2, msg, sz, use_sideband);\n> -\telse\n> -\t\txwrite(2, msg, sz);\n> +\telse {\n> +\t\twrite_in_full(2, msg, sz);\n> +\t}\n> }\n> \n> __attribute__((format (printf, 1, 2)))\n"},{"id":"489447","messageId":"xmqqwmqqixx9.fsf@gitster.g","threadId":"61012","inReplyTo":"20240226220539.3494-4-randall.becker@nexbridge.ca","subject":"Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T23:54:42Z","receivedAt":"2024-02-26T23:54:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" writes:\n\n> From: \"Randall S. Becker\" \n>\n> This change is required because some platforms do not support file writes of\n> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\n> maximum single I/O size possible for the destination device. The result of\n> write_in_full() is also passed to the caller, which was previously ignored.\n\nThis one smells more like a theoretical issue than realistic, in\nthat these writes are done only with .hexsz (either 40 or 64) bytes\noid string, or a single byte \"\\n\", for either of which it is hard to\nimagine that it is even remotely close to platform \"maximum single\nI/O size\".\n\nBut we'd need to look for the error return anyway, so switching to\nwrite_in_full() while we are doing so is also good.\n\n> Signed-off-by: Randall S. Becker \n> ---\n> builtin/repack.c | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index ede36328a3..932d24c60b 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,\n> \t\t struct packed_git *pack UNUSED,\n> \t\t uint32_t pos UNUSED, void *data)\n> {\n> +\tint err;\n> \tstruct child_process *cmd = data;\n> \n> \tif (cmd->in == -1) {\n> @@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,\n> \t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n> \t}\n> \n> -\txwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n> -\txwrite(cmd->in, \"\\n\", 1);\n> +\terr = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n> +\tif (err <= 0)\n> +\t\treturn err;\n> +\terr = write_in_full(cmd->in, \"\\n\", 1);\n> +\tif (err <= 0)\n> +\t\treturn err;\n> \treturn 0;\n> }\n"},{"id":"489448","messageId":"xmqqr0gyixuu.fsf@gitster.g","threadId":"61012","inReplyTo":"20240226220539.3494-5-randall.becker@nexbridge.ca","subject":"Re: [PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T23:56:09Z","receivedAt":"2024-02-26T23:56:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" writes:\n\n> From: \"Randall S. Becker\" \n>\n> This change is required because some platforms do not support file writes of\n> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\n> maximum single I/O size possible for the destination device.\n>\n> Signed-off-by: Randall S. Becker \n> ---\n> builtin/unpack-objects.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nThe same comment as [1/4]. Perhaps your MAX_IO_SIZE should be tuned\ndownwards, so that xwrite() works as it was designed to work.\n\n> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> index e0a701f2b3..6935c4574e 100644\n> --- a/builtin/unpack-objects.c\n> +++ b/builtin/unpack-objects.c\n> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n> \n> \t/* Write the last part of the buffer to stdout */\n> \twhile (len) {\n> -\t\tint ret = xwrite(1, buffer + offset, len);\n> +\t\tint ret = write_in_full(1, buffer + offset, len);\n> \t\tif (ret <= 0)\n> \t\t\tbreak;\n> \t\tlen -= ret;\n"},{"id":"489450","messageId":"027001da6911$a727b9d0$f5772d70$@nexbridge.com","threadId":"61012","inReplyTo":"xmqqa5nmkcuz.fsf@gitster.g","subject":"RE: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-27T00:12:24Z","receivedAt":"2024-02-27T00:12:36Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, February 26, 2024 6:47 PM, Junio C Hamano wrote:\n> writes:\n>\n>>>The code above loops while input_len is non-zero, and correctly\n>>>decrements it by the number of bytes written by xwrite() after each\n>>>iteration.\n>>>\n>>>Assuming that xwrite()/write(2) works how I think it does on NonStop,\n>>>I'm not sure I understand why this change is necessary.\n>>\n>> NonStop has a limited SSIZE_MAX. xwrite only handles that much so\n>> anything beyond that gets dropped (not in the above code but in other\n>> builtins)\n>\n>xwrite() caps a single write attempt to MAX_IO_SIZE and can return a\nshort-write, so anything beyound MAX_IO_SIZE will not even be\n>sent to the underlying write(2). There is a heuristic based on the value\nof SSIZE_MAX to define MAX_IO_SIZE in ,\n>and if the value given by that heuristics is too large for your platform,\nyou can tweak your own MAX_IO_SIZE (see the comments in\n>that header file).\n>\n>The caller of xwrite() must be prepared to see a write return with value\nless than the length it used to call the function, either because\n>of this MAX_IO_SIZE cut-off, or because of the underlying\n>write(2) returning after a short write. As long as the caller is prepared,\nlike Taylor pointed out, I am not sure why you'd need to change\n>it.\n\nI understand. I was involved in xwrite() a few years ago. The problem is\nthat users of xwrite() did not account for that and t7704.9 failed as a\nresult. These changes did fix the issue. I am not sure how to proceed based\non the above, however. Continue or recode the callers (which is part of what\nthis does)?\n\n"},{"id":"489451","messageId":"027101da6912$23cf31c0$6b6d9540$@nexbridge.com","threadId":"61012","inReplyTo":"xmqq34tekcoo.fsf@gitster.g","subject":"RE: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-27T00:15:53Z","receivedAt":"2024-02-27T00:16:03Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, February 26, 2024 6:51 PM, Junio C Hamano wrote:\n>\"Randall S. Becker\" writes:\n>\n>> From: \"Randall S. Becker\" \n>>\n>> This change is required because some platforms do not support file\n>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating\n>> the output to the maximum single I/O size possible for the destination\ndevice.\n>\n>As msg[] here is 4k on-stack buffer, if the I/O size is small enough, the\nabove may happen, and I think write-in-full is warranted here. If\n>your I/O must be done in 1k chunks, it would be very slow to run things\nlike writing a pack stream to clone any non-toy projects,\n>though X-<.\n\nOn the x86 platform, we get a size large enough not to trigger the failure\nin t7704. However, on ia64, the limit is 56Kb, which apparently does. I'm\nhoping no one else has a 1Kb limit - although some TCP stacks might\nexperience it. Either way, truncating a package is bad. Fortunately the I/O\nsubsystem on NonStop is very fast (basically DMA) between process memory\nspace.\n\n>\n>> Signed-off-by: Randall S. Becker \n>> ---\n>> builtin/receive-pack.c | 5 +++--\n>> 1 file changed, 3 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index\n>> db65607485..5064f3d300 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -455,8 +455,9 @@ static void report_message(const char *prefix,\n>> const char *err, va_list params)\n>>\n>> \tif (use_sideband)\n>> \t\tsend_sideband(1, 2, msg, sz, use_sideband);\n>> -\telse\n>> -\t\txwrite(2, msg, sz);\n>> +\telse {\n>> +\t\twrite_in_full(2, msg, sz);\n>> +\t}\n>> }\n>>\n>> __attribute__((format (printf, 1, 2)))\n\n"},{"id":"489452","messageId":"027201da6912$872779d0$95766d70$@nexbridge.com","threadId":"61012","inReplyTo":"xmqqr0gyixuu.fsf@gitster.g","subject":"RE: [PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-27T00:18:40Z","receivedAt":"2024-02-27T00:18:49Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, February 26, 2024 6:56 PM, Junio C Hamano wrote:\n>\"Randall S. Becker\" writes:\n>\n>> From: \"Randall S. Becker\" \n>>\n>> This change is required because some platforms do not support file\n>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating\n>> the output to the maximum single I/O size possible for the destination\ndevice.\n>>\n>> Signed-off-by: Randall S. Becker \n>> ---\n>> builtin/unpack-objects.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n>The same comment as [1/4]. Perhaps your MAX_IO_SIZE should be tuned\ndownwards, so that xwrite() works as it was designed to\n>work.\n\nI am considering undoing this one, other than ensuring that the error code\nis checked and returned. The MAX_IO_SIZE is sufficient. I think the actual\nfail was in the original repack.c not this one.\n\n>\n>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index\n>> e0a701f2b3..6935c4574e 100644\n>> --- a/builtin/unpack-objects.c\n>> +++ b/builtin/unpack-objects.c\n>> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char\n>> **argv, const char *prefix UNUSED)\n>>\n>> \t/* Write the last part of the buffer to stdout */\n>> \twhile (len) {\n>> -\t\tint ret = xwrite(1, buffer + offset, len);\n>> +\t\tint ret = write_in_full(1, buffer + offset, len);\n>> \t\tif (ret <= 0)\n>> \t\t\tbreak;\n>> \t\tlen -= ret;\n\n"},{"id":"489470","messageId":"20240227082027.GH3263678@coredump.intra.peff.net","threadId":"61012","inReplyTo":"20240226220539.3494-4-randall.becker@nexbridge.ca","subject":"Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-27T08:20:27Z","receivedAt":"2024-02-27T08:20:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2024 at 05:05:37PM -0500, Randall S. Becker wrote:\n\n> From: \"Randall S. Becker\" \n> \n> This change is required because some platforms do not support file writes of\n> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\n> maximum single I/O size possible for the destination device. The result of\n> write_in_full() is also passed to the caller, which was previously ignored.\n\nThese are going to be tiny compared to single-write() I/O limits, I'd\nthink, but in general we should be on guard for the OS returning short\nreads (this is a pipe and so for most systems PIPE_BUF would guarantee\natomicity, I think, but IMHO it is simpler to just make things\nobviously-correct by looping with write_in_full). So I'd be surprised if\nthis spot was the cause of a visible bug, but I think it's worth\nchanging regardless.\n\nThe error detection is a separate question, though. I think it is good\nto check the result of the write here, as an error here means that the\nchild pack-objects misses some objects we wanted it to pack, which could\nlead to a corrupt repository. But I don't think what you have here is\nquite enough:\n\n> @@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,\n> \t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n> \t}\n> \n> -\txwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n> -\txwrite(cmd->in, \"\\n\", 1);\n> +\terr = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n> +\tif (err <= 0)\n> +\t\treturn err;\n> +\terr = write_in_full(cmd->in, \"\\n\", 1);\n> +\tif (err <= 0)\n> +\t\treturn err;\n> \treturn 0;\n\nOK, so we detect the error and return it to the caller. Who is the\ncaller? The only use of this function is in repack_promisor_objects(),\nwhich calls:\n\n for_each_packed_object(write_oid, &cmd,\n FOR_EACH_OBJECT_PROMISOR_ONLY);\n\nSo when we return the error, now for_each_packed_object() will stop\ntraversing, and propagate that error up to the caller. But as we can see\nabove, the caller ignores it!\n\nSo I think you'd either want to die directly (perhaps using\nwrite_or_die). Or you'd need to additionally check the return from\nfor_each_packed_object(). That would also catch cases where that\nfunction failed to open a pack (I'm not sure how important that is to\nthis code).\n\nBut as it is, your patch just causes a write error to truncate the list\nof oids send to the child process (though that is probably not\nmaterially different from the current behavior, as the subsequent calls\nwould presumably fail, too).\n\n-Peff\n"},{"id":"489472","messageId":"20240227082253.GI3263678@coredump.intra.peff.net","threadId":"61012","inReplyTo":"20240227082027.GH3263678@coredump.intra.peff.net","subject":"Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-27T08:22:53Z","receivedAt":"2024-02-27T08:22:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2024 at 03:20:27AM -0500, Jeff King wrote:\n\n> OK, so we detect the error and return it to the caller. Who is the\n> caller? The only use of this function is in repack_promisor_objects(),\n> which calls:\n> \n> for_each_packed_object(write_oid, &cmd,\n> FOR_EACH_OBJECT_PROMISOR_ONLY);\n> \n> So when we return the error, now for_each_packed_object() will stop\n> traversing, and propagate that error up to the caller. But as we can see\n> above, the caller ignores it!\n\nOh, one other thing I meant to mention: as the test failure you saw was\nrelated to repacking, this seemed like a likely culprit. But the code is\nonly triggered when repacking promisor objects in a partial clone, and\nit didn't look like the test you posted covered that (it was just about\ncruft packs). So I would not expect this code to be run at all in the\nfailing test you saw.\n\n-Peff\n"}]}