[Xen-devel] [PATCH] cxenstored: correct calculation of data/space in the ring

[Wei Liu <wei.liu2@xxxxxxxxxx>]

The cxenstored implementation can't handle cross ring boundary read and
write. It gets aways with buggy behaviour because upper layer won't
sleep when short-write or short-read occurs.

It would be nice, however, to make the low level routine correct
regardless of what upper layer is doing. This should at least avoid
latent bug should upper layer logic changes.

This patch ports 8a2c11f8 ("tools/ocaml/xb: Correct calculations of
data/space the ring") for oxenstored to cxenstored.

Two functions -- get_{input,output}_chunks -- become unused. Nuke them.
Preserve the behaviour of always sending notification to the other end
when exiting.

Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
---
Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
 tools/xenstore/xenstored_domain.c | 76 +++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c 
b/tools/xenstore/xenstored_domain.c
index dcd6581..4c0b87e 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -82,33 +82,12 @@ static bool check_indexes(XENSTORE_RING_IDX cons, 
XENSTORE_RING_IDX prod)
        return ((prod - cons) <= XENSTORE_RING_SIZE);
 }
 
-static void *get_output_chunk(XENSTORE_RING_IDX cons,
-                             XENSTORE_RING_IDX prod,
-                             char *buf, uint32_t *len)
-{
-       *len = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
-       if ((XENSTORE_RING_SIZE - (prod - cons)) < *len)
-               *len = XENSTORE_RING_SIZE - (prod - cons);
-       return buf + MASK_XENSTORE_IDX(prod);
-}
-
-static const void *get_input_chunk(XENSTORE_RING_IDX cons,
-                                  XENSTORE_RING_IDX prod,
-                                  const char *buf, uint32_t *len)
-{
-       *len = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(cons);
-       if ((prod - cons) < *len)
-               *len = prod - cons;
-       return buf + MASK_XENSTORE_IDX(cons);
-}
-
 static int writechn(struct connection *conn,
-                   const void *data, unsigned int len)
+                   const void *buffer, unsigned int len)
 {
-       uint32_t avail;
-       void *dest;
        struct xenstore_domain_interface *intf = conn->domain->interface;
        XENSTORE_RING_IDX cons, prod;
+       int total_space, space;
 
        /* Must read indexes once, and before anything else, and verified. */
        cons = intf->rsp_cons;
@@ -120,25 +99,39 @@ static int writechn(struct connection *conn,
                return -1;
        }
 
-       dest = get_output_chunk(cons, prod, intf->rsp, &avail);
-       if (avail < len)
-               len = avail;
+       total_space = XENSTORE_RING_SIZE - (prod - cons);
+       if (total_space == 0) {
+               /* No space at all - exit having done nothing. */
+               len = 0;
+               goto exit;
+       } else if (total_space < len)
+               /* Some space - make a partial write */
+               len = total_space;
+
+       space = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
+       if (len < space)
+               /* Message fits inside the remaining part of the ring */
+               memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, len);
+       else {
+               /* Message wraps around the end of ring. Write both halves. */
+               memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, space);
+               memcpy(intf->rsp, buffer + space, len - space);
+       }
 
-       memcpy(dest, data, len);
        xen_mb();
        intf->rsp_prod += len;
 
+exit:
        xc_evtchn_notify(xce_handle, conn->domain->port);
 
        return len;
 }
 
-static int readchn(struct connection *conn, void *data, unsigned int len)
+static int readchn(struct connection *conn, void *buffer, unsigned int len)
 {
-       uint32_t avail;
-       const void *src;
        struct xenstore_domain_interface *intf = conn->domain->interface;
        XENSTORE_RING_IDX cons, prod;
+       int total_data, data;
 
        /* Must read indexes once, and before anything else, and verified. */
        cons = intf->req_cons;
@@ -150,14 +143,29 @@ static int readchn(struct connection *conn, void *data, 
unsigned int len)
                return -1;
        }
 
-       src = get_input_chunk(cons, prod, intf->req, &avail);
-       if (avail < len)
-               len = avail;
+       total_data = prod - cons;
+       if (total_data == 0) {
+               /* No pending data at all. */
+               len = 0;
+               goto exit;
+       } else if (total_data < len)
+               /* Some data - make a partial read. */
+               len = total_data;
+
+       data = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(cons);
+       if (len < data)
+               /* Data within the remaining part of the ring. */
+               memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), len);
+       else {
+               /* Data crosses the ring boundary. Read both halves. */
+               memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), data);
+               memcpy(buffer + data, intf->req, len - data);
+       }
 
-       memcpy(data, src, len);
        xen_mb();
        intf->req_cons += len;
 
+exit:
        xc_evtchn_notify(xce_handle, conn->domain->port);
 
        return len;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel