Re: [Xen-devel] [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/03/13 10:48, Dave Scott wrote:
> Hi,
>
> This looks fine to me. I assume the console ring is a C string which doesn't 
> contain NULLs in the middle and hence it's sensible to use "caml_copy_string" 
> (the alternative would be to treat it as a raw block of bytes using 
> "caml_alloc_string" and memcpy-- OCaml strings can contain NULLs safely)
>
> Cheers,
> Dave

Hmm - that is a good point.  We expect anything in the console ring to
be regular ASCII text, but already know that ACPI tables from certain
vendors contain non ASCII text.  We certainly cant rule out the presence
of NULL characters.

Furthermore, your suggestion of caml_alloc_string() has given me an idea
to remove another copy of the buffer.  I will respin the patch.

~Andrew

>
>> -----Original Message-----
>> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
>> bounces@xxxxxxxxxxxxx] On Behalf Of Andrew Cooper
>> Sent: 12 March 2013 6:09 PM
>> To: xen-devel@xxxxxxxxxxxxx
>> Cc: Ian Jackson; Keir (Xen.org); Ian Campbell; Jan Beulich; xen-
>> api@xxxxxxxxxxxxx
>> Subject: [Xen-devel] [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix
>> stub_xc_readconsolering()
>>
>> There are two problems with this function:
>>   * The caml_{enter,leave}_blocking_section() calls mean that two threads
>> can
>>     compete for use of the static ring[] buffer.
>>   * It fails to retrieve the entire buffer if the hypervisor has used 32K or
>>     more of its console ring.
>>
>> Rewrite the function using the new xc_consoleringsize() and
>> xc_readconsolering_buffer() functions to efficiently grab the entire console
>> ring.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>
>> --
>> Changes since v2:
>>  * Tweak style and remove redundant variables Changes since v1:
>>  * Convert conring_size to being static to avoid needless hypercalls
>>  * Fix memory due to ordering of failwith_xc() and
>> xc_hypercall_buffer_free()
>>  * Remove useless CAMLreturns()
>>
>> diff -r c7b82dfbec34 -r 7a33b4fdb977 tools/ocaml/libs/xc/xenctrl_stubs.c
>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> @@ -523,27 +523,47 @@ CAMLprim value stub_xc_evtchn_reset(valu
>>      CAMLreturn(Val_unit);
>>  }
>>
>> -
>> -#define RING_SIZE 32768
>> -static char ring[RING_SIZE];
>> -
>>  CAMLprim value stub_xc_readconsolering(value xch)  {
>> -    unsigned int size = RING_SIZE - 1;
>> -    char *ring_ptr = ring;
>> +    static uint64_t conring_size = 0;
>> +    unsigned int nr_chars;
>> +    DECLARE_HYPERCALL_BUFFER(char, ring);
>>      int retval;
>>
>>      CAMLparam1(xch);
>> +    CAMLlocal1(conring);
>> +
>> +    if (!conring_size)
>> +    {
>> +            if (xc_consoleringsize(_H(xch), &conring_size))
>> +                    failwith_xc(_H(xch));
>> +
>> +            /* Round conring_size up to the next page, for NULL
>> terminator
>> +               and slop from a race with printk() in the hypervisor. */
>> +            conring_size = (conring_size + XC_PAGE_SIZE) &
>> XC_PAGE_MASK;
>> +    }
>> +
>> +    nr_chars = conring_size - 1;
>> +    ring = xc_hypercall_buffer_alloc(_H(xch), ring, conring_size);
>> +
>> +    if (!ring)
>> +            caml_raise_out_of_memory();
>>
>>      caml_enter_blocking_section();
>> -    retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
>> +    retval = xc_readconsolering_buffer(_H(xch),
>> HYPERCALL_BUFFER(ring),
>> +                                       &nr_chars, 0, 0, NULL);
>>      caml_leave_blocking_section();
>>
>>      if (retval)
>> +    {
>> +            xc_hypercall_buffer_free(_H(xch), ring);
>>              failwith_xc(_H(xch));
>> +    }
>>
>> -    ring[size] = '\0';
>> -    CAMLreturn(caml_copy_string(ring));
>> +    ring[nr_chars] = '\0';
>> +    conring = caml_copy_string(ring);
>> +    xc_hypercall_buffer_free(_H(xch), ring);
>> +    CAMLreturn(conring);
>>  }
>>
>>  CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> http://lists.xen.org/xen-devel


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