@@ -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.
@@ -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).
@@ -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().
@@ -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.
@@ -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;
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(). -------------------- 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-}