Re: [PATCH v1.1 5/6] tools/libxs: Use writev()/sendmsg() instead of write()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/07/2024 10:37 am, Jürgen Groß wrote:
> On 22.07.24 18:25, Andrew Cooper wrote:
>> With the input data now conveniently arranged, use writev()/sendmsg()
>> instead
>> of decomposing it into write() calls.
>>
>> This causes all requests to be submitted with a single system call,
>> rather
>> than at least two.  While in principle short writes can occur, the
>> chances of
>> it happening are slim given that most xenbus comms are only a handful of
>> bytes.
>>
>> Nevertheless, provide {writev,sendmsg}_exact() wrappers which take
>> care of
>> resubmitting on EINTR or short write.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> CC: Juergen Gross <jgross@xxxxxxxx>
>> CC: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
>>
>> v1.1:
>>   * Fix iov overread, spotted by Frediano.  Factor the common
>> updating logic
>>     out into update_iov().
>> ---
>>   tools/libs/store/xs.c | 94 +++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 91 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>> index e820cccc2314..f80ac7558cbe 100644
>> --- a/tools/libs/store/xs.c
>> +++ b/tools/libs/store/xs.c
>> @@ -563,6 +563,95 @@ static void *read_reply(
>>       return body;
>>   }
>>   +/*
>> + * Update an iov/nr pair after an incomplete writev()/sendmsg().
>> + *
>> + * Awkwardly, nr has different widths and signs between writev() and
>> + * sendmsg(), so we take it and return it by value, rather than by
>> pointer.
>> + */
>> +static size_t update_iov(struct iovec **p_iov, size_t nr, size_t res)
>> +{
>> +    struct iovec *iov = *p_iov;
>> +
>> +        /* Skip fully complete elements, including empty elements. */
>> +        while (nr && res >= iov->iov_len) {
>> +                res -= iov->iov_len;
>> +                nr--;
>> +                iov++;
>> +        }
>> +
>> +        /* Partial element, adjust base/len. */
>> +        if (res) {
>> +                iov->iov_len  -= res;
>> +                iov->iov_base += res;
>> +        }
>> +
>> +        *p_iov = iov;
>> +
>> +    return nr;
>> +}
>> +
>> +/*
>> + * Wrapper around sendmsg() to resubmit on EINTR or short write. 
>> Returns
>> + * @true if all data was transmitted, or @false with errno for an
>> error.
>> + * Note: May alter @iov in place on resubmit.
>> + */
>> +static bool sendmsg_exact(int fd, struct iovec *iov, unsigned int nr)
>> +{
>> +    struct msghdr hdr = {
>> +        .msg_iov = iov,
>> +        .msg_iovlen = nr,
>> +    };
>> +
>> +    /* Sanity check first element isn't empty */
>> +    assert(iov->iov_len == sizeof(struct xsd_sockmsg));
>
> Can you please move this assert() into write_request(), avoiding to have
> 2 copies of it?

It was more relevant before update_iov() was split out.

But, there's exactly the same assertion in the write_request()'s caller,
so I'd prefer to simply drop it if that's ok?

The writev()/sendmsg() won't malfunction if the first element is 0, and
update_iov() will now cope too, so I don't think it's necessary.

~Andrew