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

[Juergen Gross <jgross@xxxxxxxx>]

On 23.07.24 14:25, Andrew Cooper wrote:
> 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.
Fine with me.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature