Re: [Xen-devel] [PATCH RFC] tools/ocaml/xb: Correct calculations of data/space the ring

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Andrew Cooper, le Wed 28 Oct 2015 16:05:36 +0000, a écrit :
> @@ -62,22 +62,32 @@ CAMLprim value ml_interface_read(value ml_interface,
>  
>       xen_mb();
>  
> -     if ((prod - cons) > XENSTORE_RING_SIZE)
> +     if (((prod - cons) > XENSTORE_RING_SIZE) ||
> +            ((cons - prod) < -XENSTORE_RING_SIZE))

prod and cons are uint32_t, so the difference will still be unsigned.
IIRC the test is not bogus even when prod wraps around, (prod - cons)
will still correctly be the difference between both, modulo 2^32.

Indeed, the read side also needs the same two-memcpy fix.

> +     /* Check for any pending data at all. */
> +     total_data = prod - cons;
> +     if (total_data == 0) {
...
> +     else if (total_data > len)
> +             /* Some data - make a partial read. */
> +             len = total_data;
> +
> +     /* Check whether data crosses the end of the ring. */
> +     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);
> +     }
> +

That looks right and nice-looking to me.

>       xen_mb();
>       intf->req_cons += len;
>       result = len;
> @@ -100,7 +110,7 @@ CAMLprim value ml_interface_write(value ml_interface,
>  
>       struct xenstore_domain_interface *intf = interface->addr;
>       XENSTORE_RING_IDX cons, prod;
> -     int can_write;
> +     int total_space, space;
>       uint32_t connection;
>  
>       cons = *(volatile uint32_t*)&intf->rsp_cons;
> @@ -111,17 +121,33 @@ CAMLprim value ml_interface_write(value ml_interface,
>               caml_raise_constant(*caml_named_value("Xb.Reconnect"));
>  
>       xen_mb();
> -     if ( (prod - cons) >= XENSTORE_RING_SIZE ) {
> +
> +     if (((prod - cons) > XENSTORE_RING_SIZE) ||
> +            ((cons - prod) < -XENSTORE_RING_SIZE))

Ditto.

> +     /* Check for space to write the full message. */
> +     total_space = prod - cons;
> +     if (total_space == 0) {

Shouldn't that be total_space == XENSTORE_RING_SIZE?

> +             /* No space at all - exit having done nothing. */
>               result = 0;
>               goto exit;
>       }
> +     else if (total_space < len)
> +             /* Some space - make a partial write. */
> +             len = total_space;
> +
> +     /* Check for space until the ring wraps. */
> +     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 the ring. Write both halves. 
> */
> +             memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, space);
> +             memcpy(intf->rsp, buffer + space, len - space);
> +     }

That looks right and nicer-looking than my attempt :)

Samuel

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