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

[Jürgen Groß]

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.coop...@citrix.com>
---
CC: Anthony PERARD <anthony.per...@vates.tech>
CC: Juergen Gross <jgr...@suse.com>
CC: Frediano Ziglio <frediano.zig...@cloud.com>

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?

With that:

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen