From: phillip.wood123@gmail.com
Date: Mon, 26 Feb 2024 19:45:03 GMT
Subject: Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
Message-ID: <1eba5cd0-67d1-461a-b525-05d2c1b8a55d@gmail.com>
In-Reply-To: <025301da68e6$68956940$39c03bc0$@nexbridge.com>

On 26/02/2024 19:02, rsbecker@nexbridge.com 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@nexbridge.com wrote:
>>>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>>>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
>>>>>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>>>>>>> To: git@vger.kernel.org
>>>>> 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;
> 
>