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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/10/15 16:18, Samuel Thibault wrote:
> 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.

The RHS will also be promoted to unsigned, as int has insufficient range
for the LHS.

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

(prod - cons) >= XENSTORE_RING_SIZE checks for the prod getting more
than a ring's worth ahead of cons.

(cons - prod) < -XENSTORE_RING_SIZE is supposed to check for cons
getting ahead of prod (the naive (cons > prod) fails when the ring
wraps), although I notice it still buggy in the case where cons == prod.

>
> 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?

Yes - I think it should.  (I should probably kill my pending tests -
they are not going to get very far...)

~Andrew

>
>> +            /* 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