There aren't too many places that we make calls to xwrite(), so
let's audit all.
I'll show something that looks like patch, but I won't be applying
any of them myself, at least not as a part of this message.
First, some ground rules:
* Calling xwrite() relieves us from worrying about interrupted
system call returning with EINTR or EAGAIN without writing
anything. If we were using write(2), we need to be calling it
again with the same arguments, but xwrite() retries that call for
us. That is the _only_ thing it does.
* Specifically, when write(2) returns after writing less than what
it was asked to (called "short write"), xwrite() returns the
number of bytes written, just like the underlying write(2) does,
and it is not an error.
* Errors xwrite() internally handles are only the ones that are
related to an interrupted system call that needs retrying. Other
errors are responsibility of the caller to handle. If a platform
raises an error when passed a very large buffer to write out,
instead of just making a short write and returning the number of
writes written, the platform port should protect itself from such
an error by limiting MAX_IO_SIZE, so that xwrite() does not even
attempt to make such a large write(2) call.
Now, I'll go through "git grep -W -e 'xwrite('" output and annotate
it.
--------------------
builtin/index-pack.c:final() is prepared to see and handle a short
write(2) itself. It is a correct implementation that does not need
to be touched, but we could replace the loop with a single call to
write_in_full().
diff --git c/builtin/index-pack.c w/builtin/index-pack.c
index a3a37bd215..a54cb07a35 100644
--- c/builtin/index-pack.c
+++ w/builtin/index-pack.c
@@ -1570,13 +1570,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
* Let's just mimic git-unpack-objects here and write
* the last part of the input buffer to stdout.
*/
- while (input_len) {
- err = xwrite(1, input_buffer + input_offset, input_len);
- if (err <= 0)
- break;
- input_len -= err;
- input_offset += err;
- }
+ write_in_full(1, input_buffer + input_offset, input_len);
}
strbuf_release(&rev_index_name);
--------------------
builtin/receive-pack.c:report_message() has a call to xwrite() that
will lead to message truncation if the underlying write(2) returns
early.
diff --git c/builtin/receive-pack.c w/builtin/receive-pack.c
index 56d8a77ed7..4d8ed3f7a3 100644
--- c/builtin/receive-pack.c
+++ w/builtin/receive-pack.c
@@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
if (use_sideband)
send_sideband(1, 2, msg, sz, use_sideband);
else
- xwrite(2, msg, sz);
+ write_in_full(2, msg, sz);
}
__attribute__((format (printf, 1, 2)))
--------------------
builtin/repack.c:write_oid() feeds a line with an object name on it
to a subprocess, but it can suffer from a short write(2).
diff --git c/builtin/repack.c w/builtin/repack.c
index ede36328a3..1a516a6bed 100644
--- c/builtin/repack.c
+++ w/builtin/repack.c
@@ -314,8 +314,10 @@ 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);
+ if (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) ||
+ write_in_full(cmd->in, "\n", 1))
+ die(_("failed to feed promisor objects to pack-objects"));
+
return 0;
}
--------------------
builtin/unpack-objects.c:cmd_unpack_objects() has the same xwrite()
loop as we saw in builtin/index-pack.c:final(). It is a correct
implementation that does not need to be touched, but we could
replace the loop with a single call to write_in_full().
diff --git i/builtin/unpack-objects.c w/builtin/unpack-objects.c
index e0a701f2b3..f1c85a00ae 100644
--- i/builtin/unpack-objects.c
+++ w/builtin/unpack-objects.c
@@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
use(the_hash_algo->rawsz);
/* Write the last part of the buffer to stdout */
- while (len) {
- int ret = xwrite(1, buffer + offset, len);
- if (ret <= 0)
- break;
- len -= ret;
- offset += ret;
- }
+ write_in_full(1, buffer + offset, len);
/* All done */
return has_errors;
--------------------
http.c:fwrite_sha1_file() has xwrite() loop that is prepared to see
a short write(2). The loop could be replaced with write_in_full() but
the semantics of the value returned upon failure could become different,
so I'd rather not to touch it.
http.c- do {
http.c: ssize_t retval = xwrite(freq->localfile,
http.c- (char *) ptr + posn, size - posn);
http.c- if (retval < 0)
http.c- return posn / eltsize;
http.c- posn += retval;
http.c- } while (posn < size);
--------------------
sideband.c:demultiplex_sideband() uses xwrite() that does not retry
for its progress eye-candy. write_in_full() may be overkill for
them, I suspect.
sideband.c- strbuf_addch(scratch, *brk);
sideband.c: xwrite(2, scratch->buf, scratch->len);
sideband.c- strbuf_reset(scratch);
sideband.c ...
sideband.c- if (scratch->len) {
sideband.c- strbuf_addch(scratch, '\n');
sideband.c: xwrite(2, scratch->buf, scratch->len);
sideband.c- }
--------------------
streaming.c:stream_blob_to_fd() keeps track of the "hole" at the end
of the output file by refraining from writing series of NULs, and
instead uses lseek() to move the write pointer to 1-byte before the
desired end of file, and then uses xwrite() to write a single NUL at
the end. This should not result in a short write(2), and it handles
write error correctly already, but it would be OK to update it to
write_in_full() for consistency.
diff --git c/streaming.c w/streaming.c
index 10adf625b2..38925ea9fd 100644
--- c/streaming.c
+++ w/streaming.c
@@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter
goto close_and_exit;
}
if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
- xwrite(fd, "", 1) != 1))
+ write_in_full(fd, "", 1) != 1))
goto close_and_exit;
result = 0;
--------------------
t/helper/test-genzeros.c:cmd__genzeros() runs xwrite() and dies if
it got an error. It does handle short write(2) correctly.
(1) If asked to write an infinite stream of NUL, it just feeds
zeros[] array without caring how many NUL bytes have actually been
written by a single xwrite() call. It just stops when it sees an
error.
t/helper/test-genzeros.c- /* Writing out individual NUL bytes is slow... */
t/helper/test-genzeros.c- while (count < 0)
t/helper/test-genzeros.c: if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
t/helper/test-genzeros.c- die_errno("write error");
(2) If asked to write a specific number of NUL bytes, it does a loop
that handles short write(2) correctly.
t/helper/test-genzeros.c- while (count > 0) {
t/helper/test-genzeros.c: n = xwrite(1, zeros,
t/helper/test-genzeros.c- count < ARRAY_SIZE(zeros)
t/helper/test-genzeros.c- ? count : ARRAY_SIZE(zeros));
t/helper/test-genzeros.c-
t/helper/test-genzeros.c- if (n < 0)
t/helper/test-genzeros.c- die_errno("write error");
t/helper/test-genzeros.c-
t/helper/test-genzeros.c- count -= n;
t/helper/test-genzeros.c- }
--------------------
transport-helper.c:disconnect_helper() has a call to xwrite() of a
single byte that does not care about errors, whose rationale is
given in a well written comment.
--------------------
transport-helper.c:udt_do_write() has a call to xwrite() that makes
a non-zero progress per a call to it, and its caller is prepared to
call it until the buffer is drained fully, prepared to handle a
short write(2) correctly. There is nothing to fix there.
--------------------
upload-pack.c:send_client_data() calls unchecked xwrite() on fd #2
to show an error message, but this happens only when the sideband is
not in use, so I do not know offhand how much this matters these
days. When the sideband is in use, the data is passed to
send_sideband() that does write_or_die(), so I guess for consistency
it might want to become write_in_full().
HOWEVER, there is a comment that reads "are we happy to lose stuff
here?" left by 93822c22 (short i/o: fix calls to write to use xwrite
or write_in_full, 2007-01-08), which was an earlier audit of this
exact issue that turned many unchecked xwrite() into write_in_full(),
so, I dunno.
upload-pack.c- if (use_sideband) {
upload-pack.c- send_sideband(1, fd, data, sz, use_sideband);
upload-pack.c- return;
upload-pack.c- }
upload-pack.c- if (fd == 3)
upload-pack.c- /* emergency quit */
upload-pack.c- fd = 2;
upload-pack.c- if (fd == 2) {
upload-pack.c- /* XXX: are we happy to lose stuff here? */
upload-pack.c: xwrite(fd, data, sz);
upload-pack.c- return;
upload-pack.c- }
upload-pack.c- write_or_die(fd, data, sz);
upload-pack.c-}
