On 26/02/2024 19:02, rsbecker@xxxxxxxxxxxxx wrote:
On Monday, February 26, 2024 11:00 AM, Phillip Wood wrote:
On 26/02/2024 15:32, Phillip Wood wrote:
Hi Randal
[cc'ing Patrick for the reftable writer]
On 25/02/2024 20:36, rsbecker@xxxxxxxxxxxxx wrote:
On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@xxxxxxxxxxxxx wrote:
On Sunday, February 25, 2024 1:45 PM, I wrote:
To: git@xxxxxxxxxxxxxxx
But I think that this should be used:
write_in_full()
My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
although a bit redundant since xwrite() does similar things and is
used by write_in_full().
Note that unlike write_in_full(), xwrite() does not guarantee to write
the whole buffer passed to it. In general unless a caller is writing a
single byte or writing less than PIPE_BUF bytes to a pipe it should
use write_in_full().
The question is which call is bad? The cruft stuff is relatively new
and I don't know the code.
I should have been clearer that I do not think any of these calls are the likely
problem for the cruft pack code. I do think the reftable writers are worth looking at
though for git in general.
For the cruft pack problem you might want to look for suspect xwrite() calls where
the caller does not handle a short write correctly for example under builtin/ we have
builtin/index-pack.c: err = xwrite(1, input_buffer +
input_offset, input_len);
builtin/receive-pack.c: xwrite(2, msg, sz);
builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid),
the_hash_algo->hexsz);
builtin/repack.c: xwrite(cmd->in, "\n", 1);
builtin/unpack-objects.c: int ret = xwrite(1, buffer +
offset, len);
Best Wishes
Phillip
reftable/writer.c: int n = w->write(w->write_arg,
zeroed,
w->pending_padding);
reftable/writer.c: n = w->write(w->write_arg, data, len);
Neither of these appear to check for short writes and
reftable_fd_write() is a thin wrapper around write(). Maybe
reftable_fd_write() should be using write_in_full()?
run-command.c: len = write(io->fd, io->u.out.buf,
This call to write() looks correct as it is in the io pump loop.
t/helper/test-path-utils.c: if (write(1,
buffer,
count)
< 0) >>> t/helper/test-windows-named-pipe.c: write(1,
buf, nbr);
t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
In principle these all look like they are prone to short writes.
trace2/tr2_dst.c: bytes = write(fd, buf_line->buf,
buf_line->len);
This caller explicitly says it prefers short writes over retrying
Replacing xwrite with write_in_full the above worked correctly. Do you want it or should I write this up?
I'm glad that worked for you. If you are able to write it up that would
be great. There are some dodgy looking xwrite() calls outside builtin/
as well. It is probably worth checking the return value of
write_in_full() when you do the conversion for the sites where we ignore
errors at the moment.
Best Wishes
Phillip
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a3a37bd215..f80b8d101a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
* the last part of the input buffer to stdout.
*/
while (input_len) {
- err = xwrite(1, input_buffer + input_offset, input_len);
+ err = write_in_full(1, input_buffer + input_offset, input_len);
if (err <= 0)
break;
input_len -= err;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index db65607485..4277c63d08 100644
--- a/builtin/receive-pack.c
+++ b/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)))
diff --git a/builtin/repack.c b/builtin/repack.c
index ede36328a3..4916870992 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -314,8 +314,8 @@ 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);
+ write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
+ write_in_full(cmd->in, "\n", 1);
return 0;
}
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;
