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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/02/13 10:14, Ian Campbell wrote:
> On Wed, 2013-02-20 at 18:05 +0000, Andrew Cooper wrote:
>> 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@citrix.com>
>>
>> diff -r 6b8c513cff4f -r a73d0c5a5b24 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,61 @@ CAMLprim value stub_xc_evtchn_reset(valu
>>  	CAMLreturn(Val_unit);
>>  }
>>  
>> -
>> -#define RING_SIZE 32768
>> -static char ring[RING_SIZE];
>> -
>> +/* Maximum size of console ring we are prepared to read, 16MB */
>> +#define MAX_CONSOLE_RING_SIZE (1U << 24)
> A bit arbitrary, why not just let it handle whatever the hypervisor
> throws at it. If there is to be a limit it probably belongs on the
> hypervisor side, but in the meantime someone who passes consring=1G can
> keep all the pieces ;-)
>
>>  CAMLprim value stub_xc_readconsolering(value xch)
>>  {
>> -	unsigned int size = RING_SIZE - 1;
>> -	char *ring_ptr = ring;
>> -	int retval;
>> +	uint64_t conring_size;
>> +	DECLARE_HYPERCALL_BUFFER(char, ring);
>> +	unsigned int nr_chars;
>> +	int r;
>>  
>>  	CAMLparam1(xch);
>> +	CAMLlocal1(conring);
>> +
>> +	r = xc_consoleringsize(_H(xch), &conring_size);
> static int consoleringsize and only call this once the first time?

One of the point of this patch was to remove the static buffer for
thread safety reasons.  Having said that, even if you do race on this
static variable, it should be filled in with the same value, so is
probably safe?

>
>> +
>> +	if ( r )
>> +	{
>> +		failwith_xc(_H(xch));
>> +		CAMLreturn(Val_unit);
>> +	}
>> +
>> +	/* 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;
>> +
>> +	if ( conring_size > MAX_CONSOLE_RING_SIZE )
>> +	{
>> +		caml_failwith("Console ring too large");
>> +		CAMLreturn(Val_unit);
>> +	}
>> +
>> +	ring = xc_hypercall_buffer_alloc(_H(xch), ring, conring_size);
>> +
>> +	if ( ! ring )
>> +	{
>> +		failwith_xc(_H(xch));
>> +		CAMLreturn(Val_unit);
>> +	}
>>  
>>  	caml_enter_blocking_section();
>> -	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
>> +	r = xc_readconsolering_buffer(_H(xch), HYPERCALL_BUFFER(ring),
>> +				      &nr_chars, 0, 0, NULL);
>>  	caml_leave_blocking_section();
>>  
>> -	if (retval)
>> +	if ( r )
>> +	{
>>  		failwith_xc(_H(xch));
> Doesn't this (due to caml_raise...) end up exiting synchronously via a
> longjmp back to the runtime? i.e. it leaks the buffer which is only
> free'd after.
>
> It's a good idea to cc xen-api@ with ocaml changes, for this sort of
> reason...

I will double check that.  If so, then I have some memory leaks to fix
up in other bits of Ocaml/C bindings, which I was using as an example
here...

I will cc xen-api@ for the next submission of this series.

>
>> +		xc_hypercall_buffer_free(_H(xch), ring);
>> +		CAMLreturn(Val_unit);
> I think CAMLnoreturn here.

Ok.

~Andrew

>
>> +	}
>>  
>> -	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)
>