{"thread":{"id":"61020","subject":"Uses of xwrite() in the codebase","startedAt":"2024-02-27T23:07:28Z","lastAt":"2024-02-27T23:07:28Z","messageCount":1,"participants":["Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"489581","messageId":"xmqqil29o6ag.fsf@gitster.g","threadId":"61020","inReplyTo":null,"subject":"Uses of xwrite() in the codebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T23:07:19Z","receivedAt":"2024-02-27T23:07:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There aren't too many places that we make calls to xwrite(), so\nlet's audit all.\n\nI'll show something that looks like patch, but I won't be applying\nany of them myself, at least not as a part of this message.\n\nFirst, some ground rules:\n\n * Calling xwrite() relieves us from worrying about interrupted\n   system call returning with EINTR or EAGAIN without writing\n   anything.  If we were using write(2), we need to be calling it\n   again with the same arguments, but xwrite() retries that call for\n   us.  That is the _only_ thing it does.\n\n * Specifically, when write(2) returns after writing less than what\n   it was asked to (called \"short write\"), xwrite() returns the\n   number of bytes written, just like the underlying write(2) does,\n   and it is not an error.\n\n * Errors xwrite() internally handles are only the ones that are\n   related to an interrupted system call that needs retrying.  Other\n   errors are responsibility of the caller to handle.  If a platform\n   raises an error when passed a very large buffer to write out,\n   instead of just making a short write and returning the number of\n   writes written, the platform port should protect itself from such\n   an error by limiting MAX_IO_SIZE, so that xwrite() does not even\n   attempt to make such a large write(2) call.\n\nNow, I'll go through \"git grep -W -e 'xwrite('\" output and annotate\nit.\n\n--------------------\nbuiltin/index-pack.c:final() is prepared to see and handle a short\nwrite(2) itself.  It is a correct implementation that does not need\nto be touched, but we could replace the loop with a single call to\nwrite_in_full().\n\ndiff --git c/builtin/index-pack.c w/builtin/index-pack.c\nindex a3a37bd215..a54cb07a35 100644\n--- c/builtin/index-pack.c\n+++ w/builtin/index-pack.c\n@@ -1570,13 +1570,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t * Let's just mimic git-unpack-objects here and write\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\tif (err <= 0)\n-\t\t\t\tbreak;\n-\t\t\tinput_len -= err;\n-\t\t\tinput_offset += err;\n-\t\t}\n+\t\twrite_in_full(1, input_buffer + input_offset, input_len);\n \t}\n \n \tstrbuf_release(&rev_index_name);\n\n\n--------------------\nbuiltin/receive-pack.c:report_message() has a call to xwrite() that\nwill lead to message truncation if the underlying write(2) returns\nearly.\n\ndiff --git c/builtin/receive-pack.c w/builtin/receive-pack.c\nindex 56d8a77ed7..4d8ed3f7a3 100644\n--- c/builtin/receive-pack.c\n+++ w/builtin/receive-pack.c\n@@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params)\n \tif (use_sideband)\n \t\tsend_sideband(1, 2, msg, sz, use_sideband);\n \telse\n-\t\txwrite(2, msg, sz);\n+\t\twrite_in_full(2, msg, sz);\n }\n \n __attribute__((format (printf, 1, 2)))\n\n\n\n--------------------\nbuiltin/repack.c:write_oid() feeds a line with an object name on it\nto a subprocess, but it can suffer from a short write(2).\n\ndiff --git c/builtin/repack.c w/builtin/repack.c\nindex ede36328a3..1a516a6bed 100644\n--- c/builtin/repack.c\n+++ w/builtin/repack.c\n@@ -314,8 +314,10 @@ 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+\tif (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) ||\n+\t    write_in_full(cmd->in, \"\\n\", 1))\n+\t\tdie(_(\"failed to feed promisor objects to pack-objects\"));\n+\n \treturn 0;\n }\n \n\n--------------------\nbuiltin/unpack-objects.c:cmd_unpack_objects() has the same xwrite()\nloop as we saw in builtin/index-pack.c:final().  It is a correct\nimplementation that does not need to be touched, but we could\nreplace the loop with a single call to write_in_full().\n\ndiff --git i/builtin/unpack-objects.c w/builtin/unpack-objects.c\nindex e0a701f2b3..f1c85a00ae 100644\n--- i/builtin/unpack-objects.c\n+++ w/builtin/unpack-objects.c\n@@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n \tuse(the_hash_algo->rawsz);\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\tif (ret <= 0)\n-\t\t\tbreak;\n-\t\tlen -= ret;\n-\t\toffset += ret;\n-\t}\n+\twrite_in_full(1, buffer + offset, len);\n \n \t/* All done */\n \treturn has_errors;\n\n\n--------------------\nhttp.c:fwrite_sha1_file() has xwrite() loop that is prepared to see\na short write(2).  The loop could be replaced with write_in_full() but\nthe semantics of the value returned upon failure could become different,\nso I'd rather not to touch it.\n\nhttp.c-\tdo {\nhttp.c:\t\tssize_t retval = xwrite(freq->localfile,\nhttp.c-\t\t\t\t\t(char *) ptr + posn, size - posn);\nhttp.c-\t\tif (retval < 0)\nhttp.c-\t\t\treturn posn / eltsize;\nhttp.c-\t\tposn += retval;\nhttp.c-\t} while (posn < size);\n\n--------------------\nsideband.c:demultiplex_sideband() uses xwrite() that does not retry\nfor its progress eye-candy.  write_in_full() may be overkill for\nthem, I suspect.\n\nsideband.c-\t\t\tstrbuf_addch(scratch, *brk);\nsideband.c:\t\t\txwrite(2, scratch->buf, scratch->len);\nsideband.c-\t\t\tstrbuf_reset(scratch);\nsideband.c ...\nsideband.c-\tif (scratch->len) {\nsideband.c-\t\tstrbuf_addch(scratch, '\\n');\nsideband.c:\t\txwrite(2, scratch->buf, scratch->len);\nsideband.c-\t}\n\n\n--------------------\nstreaming.c:stream_blob_to_fd() keeps track of the \"hole\" at the end\nof the output file by refraining from writing series of NULs, and\ninstead uses lseek() to move the write pointer to 1-byte before the\ndesired end of file, and then uses xwrite() to write a single NUL at\nthe end.  This should not result in a short write(2), and it handles\nwrite error correctly already, but it would be OK to update it to\nwrite_in_full() for consistency.\n\ndiff --git c/streaming.c w/streaming.c\nindex 10adf625b2..38925ea9fd 100644\n--- c/streaming.c\n+++ w/streaming.c\n@@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter\n \t\t\tgoto close_and_exit;\n \t}\n \tif (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||\n-\t\t     xwrite(fd, \"\", 1) != 1))\n+\t\t     write_in_full(fd, \"\", 1) != 1))\n \t\tgoto close_and_exit;\n \tresult = 0;\n \n--------------------\nt/helper/test-genzeros.c:cmd__genzeros() runs xwrite() and dies if\nit got an error.  It does handle short write(2) correctly.\n\n(1) If asked to write an infinite stream of NUL, it just feeds\nzeros[] array without caring how many NUL bytes have actually been\nwritten by a single xwrite() call.  It just stops when it sees an\nerror.\n\nt/helper/test-genzeros.c-\t/* Writing out individual NUL bytes is slow... */\nt/helper/test-genzeros.c-\twhile (count < 0)\nt/helper/test-genzeros.c:\t\tif (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)\nt/helper/test-genzeros.c-\t\t\tdie_errno(\"write error\");\n\n(2) If asked to write a specific number of NUL bytes, it does a loop\nthat handles short write(2) correctly.\n\nt/helper/test-genzeros.c-\twhile (count > 0) {\nt/helper/test-genzeros.c:\t\tn = xwrite(1, zeros,\nt/helper/test-genzeros.c-\t\t\t   count < ARRAY_SIZE(zeros)\nt/helper/test-genzeros.c-\t\t\t   ? count : ARRAY_SIZE(zeros));\nt/helper/test-genzeros.c-\nt/helper/test-genzeros.c-\t\tif (n < 0)\nt/helper/test-genzeros.c-\t\t\tdie_errno(\"write error\");\nt/helper/test-genzeros.c-\nt/helper/test-genzeros.c-\t\tcount -= n;\nt/helper/test-genzeros.c-\t}\n\n--------------------\ntransport-helper.c:disconnect_helper() has a call to xwrite() of a\nsingle byte that does not care about errors, whose rationale is\ngiven in a well written comment.\n\n--------------------\ntransport-helper.c:udt_do_write() has a call to xwrite() that makes\na non-zero progress per a call to it, and its caller is prepared to\ncall it until the buffer is drained fully, prepared to handle a\nshort write(2) correctly.  There is nothing to fix there.\n\n--------------------\nupload-pack.c:send_client_data() calls unchecked xwrite() on fd #2\nto show an error message, but this happens only when the sideband is\nnot in use, so I do not know offhand how much this matters these\ndays.  When the sideband is in use, the data is passed to\nsend_sideband() that does write_or_die(), so I guess for consistency\nit might want to become write_in_full().  \n\nHOWEVER, there is a comment that reads \"are we happy to lose stuff\nhere?\" left by 93822c22 (short i/o: fix calls to write to use xwrite\nor write_in_full, 2007-01-08), which was an earlier audit of this\nexact issue that turned many unchecked xwrite() into write_in_full(),\nso, I dunno.\n\nupload-pack.c-\tif (use_sideband) {\nupload-pack.c-\t\tsend_sideband(1, fd, data, sz, use_sideband);\nupload-pack.c-\t\treturn;\nupload-pack.c-\t}\nupload-pack.c-\tif (fd == 3)\nupload-pack.c-\t\t/* emergency quit */\nupload-pack.c-\t\tfd = 2;\nupload-pack.c-\tif (fd == 2) {\nupload-pack.c-\t\t/* XXX: are we happy to lose stuff here? */\nupload-pack.c:\t\txwrite(fd, data, sz);\nupload-pack.c-\t\treturn;\nupload-pack.c-\t}\nupload-pack.c-\twrite_or_die(fd, data, sz);\nupload-pack.c-}\n"}]}