Xen devel - Feb 2013 - [PATCH 4 of 5 v2] 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@citrix.com>

--
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 1e6c7f7cec6f -r 1774a72fde4a 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,50 @@ 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;
-	int retval;
+	static int have_conring_size = 0;
+	static uint64_t conring_size;
+
+	unsigned int nr_chars;
+	DECLARE_HYPERCALL_BUFFER(char, ring);
+	int r;
 
 	CAMLparam1(xch);
+	CAMLlocal1(conring);
+
+	if ( ! have_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;
+		have_conring_size = 0;
+	}
+
+	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);
+	r = xc_readconsolering_buffer(_H(xch), HYPERCALL_BUFFER(ring),
+				      &nr_chars, 0, 0, NULL);
 	caml_leave_blocking_section();
 
-	if (retval)
+	if ( r )
+	{
+		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)

On Thu, 2013-02-21 at 15:46 +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>
> 
> --
> 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 1e6c7f7cec6f -r 1774a72fde4a 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,50 @@ 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;
> -	int retval;
> +	static int have_conring_size = 0;
> +	static uint64_t conring_size;
Checking if conring_size == 0 (or ~0 if you prefer) is functionally
equivalent to the additional variable I think.
> +
> +	unsigned int nr_chars;
> +	DECLARE_HYPERCALL_BUFFER(char, ring);
> +	int r;
>  
>  	CAMLparam1(xch);
> +	CAMLlocal1(conring);
> +
> +	if ( ! have_conring_size )
This overuse of spaces is neither Xen style nor the prevailing style in
this file AFAICT. The prevailing style appears to be Linux-ish, I
suggest to stick with that.
> +	{
> +		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;
> +		have_conring_size = 0;
If you do want to do things this way then surely this has to be = 1 ?
> +	}
> +
> +	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);
> +	r = xc_readconsolering_buffer(_H(xch), HYPERCALL_BUFFER(ring),
> +				      &nr_chars, 0, 0, NULL);
>  	caml_leave_blocking_section();
>  
> -	if (retval)
> +	if ( r )
The s/retval/r/ seems a bit unneeded.
> +	{
> +		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)