| Message ID | 20240226220539.3494-2-randall.becker@nexbridge.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Change xwrite() to write_in_full() in builtins. | expand |
On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote: > From: "Randall S. Becker" <rsbecker@nexbridge.com> > > This change is required because some platforms do not support file writes of > arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the > maximum single I/O size possible for the destination device. Hmm. I'm not sure I understand what NonStop's behavior is here... > 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; > -- > 2.42.1 The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each iteration. Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary. Thanks, Taylor
On Monday, February 26, 2024 5:39 PM, Taylor Blau wrote: >On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote: >> From: "Randall S. Becker" <rsbecker@nexbridge.com> >> >> This change is required because some platforms do not support file >> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating >> the output to the maximum single I/O size possible for the destination device. > >Hmm. I'm not sure I understand what NonStop's behavior is here... > >> 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; >> -- >> 2.42.1 > >The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each >iteration. > >Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary. NonStop 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.
On Monday, February 26, 2024 5:39 PM, Taylor Blau wrote: >On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote: >> From: "Randall S. Becker" <rsbecker@nexbridge.com> >> >> This change is required because some platforms do not support file >> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating >> the output to the maximum single I/O size possible for the destination device. > >Hmm. I'm not sure I understand what NonStop's behavior is here... > >> 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; >> -- >> 2.42.1 > >The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each >iteration. > >Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary. After 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.
<rsbecker@nexbridge.com> writes: >>The code above loops while input_len is non-zero, and correctly >>decrements it by the number of bytes written by xwrite() after >>each iteration. >> >>Assuming that xwrite()/write(2) works how I think it does on >>NonStop, I'm not sure I understand why this change is necessary. > > NonStop 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) xwrite() caps a single write attempt to MAX_IO_SIZE and can return a short-write, so anything beyound MAX_IO_SIZE will not even be sent to the underlying write(2). There is a heuristic based on the value of SSIZE_MAX to define MAX_IO_SIZE in <git-compat-util.h>, and if the value given by that heuristics is too large for your platform, you can tweak your own MAX_IO_SIZE (see the comments in that header file). The caller of xwrite() must be prepared to see a write return with value less than the length it used to call the function, either because of this MAX_IO_SIZE cut-off, or because of the underlying write(2) returning after a short write. As long as the caller is prepared, like Taylor pointed out, I am not sure why you'd need to change it.
On Monday, February 26, 2024 6:47 PM, Junio C Hamano wrote: ><rsbecker@nexbridge.com> writes: > >>>The code above loops while input_len is non-zero, and correctly >>>decrements it by the number of bytes written by xwrite() after each >>>iteration. >>> >>>Assuming that xwrite()/write(2) works how I think it does on NonStop, >>>I'm not sure I understand why this change is necessary. >> >> NonStop 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) > >xwrite() caps a single write attempt to MAX_IO_SIZE and can return a short-write, so anything beyound MAX_IO_SIZE will not even be >sent to the underlying write(2). There is a heuristic based on the value of SSIZE_MAX to define MAX_IO_SIZE in <git-compat-util.h>, >and if the value given by that heuristics is too large for your platform, you can tweak your own MAX_IO_SIZE (see the comments in >that header file). > >The caller of xwrite() must be prepared to see a write return with value less than the length it used to call the function, either because >of this MAX_IO_SIZE cut-off, or because of the underlying >write(2) returning after a short write. As long as the caller is prepared, like Taylor pointed out, I am not sure why you'd need to change >it. I understand. I was involved in xwrite() a few years ago. The problem is that users of xwrite() did not account for that and t7704.9 failed as a result. These changes did fix the issue. I am not sure how to proceed based on the above, however. Continue or recode the callers (which is part of what this does)?
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;