[PATCH 1 of 2] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

[PATCH 1 of 2] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

[Andrew Cooper]

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.

Unfortunately, fixing the code in a sensible is rather difficult.  The Ocaml
caller expects to get the entire console ring in a single buffer.  There are
no mechanisms to ask Xen for the size of the console ring (which is fixed
after boot).  Even if there was a way to get the size of the console ring, the
XEN_SYSCTL_readconsole hypercall itself races with printk(), leading to
possibly discontinuities if the ring is full.  The requirement for a NULL
terminating string means that a full console ring will appear to fail when use
the correct power of two, forcing us to double up again.  Furthermore, libxc
will bounce the allocated buffer, as will caml_copy_string().

On the other hand, this is very definitely for debugging and testing, so lets
do the best we can to get the entire buffer, and accept the inefficiencies.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 63594ce1708f -r 1daa30509f1c 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,55 @@ 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)
 CAMLprim value stub_xc_readconsolering(value xch)
 {
-	unsigned int size = RING_SIZE - 1;
-	char *ring_ptr = ring;
-	int retval;
+	/* 32K starting size (adj for loop doubling at start) */
+	unsigned int ring_size = 1U << 14;
+	unsigned int nr_chars = ring_size;
+	char * ring = NULL;
+	int r;
 
 	CAMLparam1(xch);
+	CAMLlocal1(conring);
 
-	caml_enter_blocking_section();
-	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
-	caml_leave_blocking_section();
+	do
+	{
+		ring_size *= 2;
+		if ( ring_size > MAX_CONSOLE_RING_SIZE )
+		{
+			caml_failwith("Console ring too large");
+			CAMLreturn(Val_unit);
+		}
+		nr_chars = ring_size;
 
-	if (retval)
-		failwith_xc(_H(xch));
+		free(ring);
+		if ( ! ( ring = malloc(ring_size) ) )
+		{
+			caml_failwith("Out of memory");
+			CAMLreturn(Val_unit);
+		}
 
-	ring[size] = '\0';
-	CAMLreturn(caml_copy_string(ring));
+		caml_enter_blocking_section();
+		r = xc_readconsolering(_H(xch), ring, &nr_chars, 0, 0, NULL);
+		caml_leave_blocking_section();
+
+		if ( r )
+		{
+			failwith_xc(_H(xch));
+			free(ring);
+			CAMLreturn(Val_unit);
+		}
+
+		/* If nr_chars == ring_size, we have not read the entire ring.
+		 * Try again with a larger buffer. */
+	} while ( nr_chars >= ring_size );
+
+	ring[nr_chars] = '\0';
+	conring = caml_copy_string(ring);
+	free(ring);
+	CAMLreturn(conring);
 }
 
 CAMLprim value stub_xc_send_debug_keys(value xch, value keys)

[PATCH 2 of 2] tools/ocaml: libxc bindings: Fix failwith_xc()

[Andrew Cooper]

The static error_str[] buffer is not thread-safe, and 1024 bytes is
unreasonably large.  Reduce to 256 bytes (which is still much larger than any
current use), and move it to being a stack variable.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 1daa30509f1c -r 39ae53386eb2 tools/ocaml/libs/xc/xenctrl_stubs.c
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -51,21 +51,22 @@
 	i1 = (uint32_t) Int64_val(Field(input, 0)); \
 	i2 = ((Field(input, 1) == Val_none) ? 0xffffffff : (uint32_t) Int64_val(Field(Field(input, 1), 0)));
 
-#define ERROR_STRLEN 1024
 void failwith_xc(xc_interface *xch)
 {
-	static char error_str[ERROR_STRLEN];
+	char error_str[256];
 	if (xch) {
 		const xc_error *error = xc_get_last_error(xch);
 		if (error->code == XC_ERROR_NONE)
-                	snprintf(error_str, ERROR_STRLEN, "%d: %s", errno, strerror(errno));
+                	snprintf(error_str, sizeof(error_str),
+				 "%d: %s", errno, strerror(errno));
 		else
-			snprintf(error_str, ERROR_STRLEN, "%d: %s: %s",
-				 error->code,
+			snprintf(error_str, sizeof(error_str),
+				 "%d: %s: %s", error->code,
 				 xc_error_code_to_desc(error->code),
 				 error->message);
 	} else {
-		snprintf(error_str, ERROR_STRLEN, "Unable to open XC interface");
+		snprintf(error_str, sizeof(error_str),
+			 "Unable to open XC interface");
 	}
 	caml_raise_with_string(*caml_named_value("xc.error"), error_str);
 }

Re: [PATCH 2 of 2] tools/ocaml: libxc bindings: Fix failwith_xc()

[Ian Campbell]

On Thu, 2013-02-14 at 19:29 +0000, Andrew Cooper wrote:
> The static error_str[] buffer is not thread-safe, and 1024 bytes is
> unreasonably large.  Reduce to 256 bytes (which is still much larger than any
> current use), and move it to being a stack variable.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Looks plausible to me, but please could you cc xen-api@ for ocaml
changes so someone who knows ocaml can comment.

> 
> diff -r 1daa30509f1c -r 39ae53386eb2 tools/ocaml/libs/xc/xenctrl_stubs.c
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -51,21 +51,22 @@
>  	i1 = (uint32_t) Int64_val(Field(input, 0)); \
>  	i2 = ((Field(input, 1) == Val_none) ? 0xffffffff : (uint32_t) Int64_val(Field(Field(input, 1), 0)));
>  
> -#define ERROR_STRLEN 1024
>  void failwith_xc(xc_interface *xch)
>  {
> -	static char error_str[ERROR_STRLEN];
> +	char error_str[256];
>  	if (xch) {
>  		const xc_error *error = xc_get_last_error(xch);
>  		if (error->code == XC_ERROR_NONE)
> -                	snprintf(error_str, ERROR_STRLEN, "%d: %s", errno, strerror(errno));
> +                	snprintf(error_str, sizeof(error_str),
> +				 "%d: %s", errno, strerror(errno));
>  		else
> -			snprintf(error_str, ERROR_STRLEN, "%d: %s: %s",
> -				 error->code,
> +			snprintf(error_str, sizeof(error_str),
> +				 "%d: %s: %s", error->code,
>  				 xc_error_code_to_desc(error->code),
>  				 error->message);
>  	} else {
> -		snprintf(error_str, ERROR_STRLEN, "Unable to open XC interface");
> +		snprintf(error_str, sizeof(error_str),
> +			 "Unable to open XC interface");
>  	}
>  	caml_raise_with_string(*caml_named_value("xc.error"), error_str);
>  }

Re: [PATCH 1 of 2] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

[Ian Campbell]

On Thu, 2013-02-14 at 19:29 +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.
> 
> Unfortunately, fixing the code in a sensible is rather difficult.  The Ocaml
> caller expects to get the entire console ring in a single buffer.  There are
> no mechanisms to ask Xen for the size of the console ring (which is fixed
> after boot).

It would be easy to add a sysctl to get this

>   Even if there was a way to get the size of the console ring, the
> XEN_SYSCTL_readconsole hypercall itself races with printk(), leading to
> possibly discontinuities if the ring is full.

Right, that one is tricky, but as you say we could accept the
inefficiencies here.

>   The requirement for a NULL
> terminating string means that a full console ring will appear to fail when use
> the correct power of two, forcing us to double up again.

Double? Or just allocate buffersize + 1? We could also consider fixing
the XEN_SYSCTL_readconsole interface to remove this issue.

> Furthermore, libxc will bounce the allocated buffer, as will caml_copy_string().

You could avoid the first of these by using xc_hypercall_buffer_alloc or
xc_hypercall_buffer_alloc_pages in the caller, these are exposed from
libxc exactly to allow us to avoid bouncing of larger objects.


> On the other hand, this is very definitely for debugging and testing, so lets
> do the best we can to get the entire buffer, and accept the inefficiencies.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r 63594ce1708f -r 1daa30509f1c 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,55 @@ 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)
>  CAMLprim value stub_xc_readconsolering(value xch)
>  {
> -	unsigned int size = RING_SIZE - 1;
> -	char *ring_ptr = ring;
> -	int retval;
> +	/* 32K starting size (adj for loop doubling at start) */
> +	unsigned int ring_size = 1U << 14;
> +	unsigned int nr_chars = ring_size;
> +	char * ring = NULL;
> +	int r;
>  
>  	CAMLparam1(xch);
> +	CAMLlocal1(conring);
>  
> -	caml_enter_blocking_section();
> -	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
> -	caml_leave_blocking_section();
> +	do
> +	{
> +		ring_size *= 2;
> +		if ( ring_size > MAX_CONSOLE_RING_SIZE )
> +		{
> +			caml_failwith("Console ring too large");
> +			CAMLreturn(Val_unit);
> +		}
> +		nr_chars = ring_size;
>  
> -	if (retval)
> -		failwith_xc(_H(xch));
> +		free(ring);
> +		if ( ! ( ring = malloc(ring_size) ) )
> +		{
> +			caml_failwith("Out of memory");
> +			CAMLreturn(Val_unit);
> +		}
>  
> -	ring[size] = '\0';
> -	CAMLreturn(caml_copy_string(ring));
> +		caml_enter_blocking_section();
> +		r = xc_readconsolering(_H(xch), ring, &nr_chars, 0, 0, NULL);
> +		caml_leave_blocking_section();
> +
> +		if ( r )
> +		{
> +			failwith_xc(_H(xch));
> +			free(ring);
> +			CAMLreturn(Val_unit);
> +		}
> +
> +		/* If nr_chars == ring_size, we have not read the entire ring.
> +		 * Try again with a larger buffer. */
> +	} while ( nr_chars >= ring_size );
> +
> +	ring[nr_chars] = '\0';
> +	conring = caml_copy_string(ring);
> +	free(ring);
> +	CAMLreturn(conring);
>  }
>  
>  CAMLprim value stub_xc_send_debug_keys(value xch, value keys)

Re: [PATCH 1 of 2] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

[Andrew Cooper]

On 15/02/13 14:26, Ian Campbell wrote:
> On Thu, 2013-02-14 at 19:29 +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.
>>
>> Unfortunately, fixing the code in a sensible is rather difficult.  The Ocaml
>> caller expects to get the entire console ring in a single buffer.  There are
>> no mechanisms to ask Xen for the size of the console ring (which is fixed
>> after boot).
> It would be easy to add a sysctl to get this

Yes.  I didn't because of a combination of the reasons below, and
because I needed a fix for our automated testing, but I think I have had
a cunning idea.

>
>>   Even if there was a way to get the size of the console ring, the
>> XEN_SYSCTL_readconsole hypercall itself races with printk(), leading to
>> possibly discontinuities if the ring is full.
> Right, that one is tricky, but as you say we could accept the
> inefficiencies here.
>
>>   The requirement for a NULL
>> terminating string means that a full console ring will appear to fail when use
>> the correct power of two, forcing us to double up again.
> Double? Or just allocate buffersize + 1? We could also consider fixing
> the XEN_SYSCTL_readconsole interface to remove this issue.

Requires buffersize + 1 + nr_of_chars_from_a_raced_printk to get any
indication back from the hypercall that we have indeed read the full ring.

Choosing buffersize rounded up to the next PAGE_SIZE will suffice except
in extreme circumstances.

>
>> Furthermore, libxc will bounce the allocated buffer, as will caml_copy_string().
> You could avoid the first of these by using xc_hypercall_buffer_alloc or
> xc_hypercall_buffer_alloc_pages in the caller, these are exposed from
> libxc exactly to allow us to avoid bouncing of larger objects.

I will investigate using these.

~Andrew

>
>
>> On the other hand, this is very definitely for debugging and testing, so lets
>> do the best we can to get the entire buffer, and accept the inefficiencies.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 63594ce1708f -r 1daa30509f1c 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,55 @@ 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)
>>  CAMLprim value stub_xc_readconsolering(value xch)
>>  {
>> -	unsigned int size = RING_SIZE - 1;
>> -	char *ring_ptr = ring;
>> -	int retval;
>> +	/* 32K starting size (adj for loop doubling at start) */
>> +	unsigned int ring_size = 1U << 14;
>> +	unsigned int nr_chars = ring_size;
>> +	char * ring = NULL;
>> +	int r;
>>  
>>  	CAMLparam1(xch);
>> +	CAMLlocal1(conring);
>>  
>> -	caml_enter_blocking_section();
>> -	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
>> -	caml_leave_blocking_section();
>> +	do
>> +	{
>> +		ring_size *= 2;
>> +		if ( ring_size > MAX_CONSOLE_RING_SIZE )
>> +		{
>> +			caml_failwith("Console ring too large");
>> +			CAMLreturn(Val_unit);
>> +		}
>> +		nr_chars = ring_size;
>>  
>> -	if (retval)
>> -		failwith_xc(_H(xch));
>> +		free(ring);
>> +		if ( ! ( ring = malloc(ring_size) ) )
>> +		{
>> +			caml_failwith("Out of memory");
>> +			CAMLreturn(Val_unit);
>> +		}
>>  
>> -	ring[size] = '\0';
>> -	CAMLreturn(caml_copy_string(ring));
>> +		caml_enter_blocking_section();
>> +		r = xc_readconsolering(_H(xch), ring, &nr_chars, 0, 0, NULL);
>> +		caml_leave_blocking_section();
>> +
>> +		if ( r )
>> +		{
>> +			failwith_xc(_H(xch));
>> +			free(ring);
>> +			CAMLreturn(Val_unit);
>> +		}
>> +
>> +		/* If nr_chars == ring_size, we have not read the entire ring.
>> +		 * Try again with a larger buffer. */
>> +	} while ( nr_chars >= ring_size );
>> +
>> +	ring[nr_chars] = '\0';
>> +	conring = caml_copy_string(ring);
>> +	free(ring);
>> +	CAMLreturn(conring);
>>  }
>>  
>>  CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
>