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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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

> +
> +     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...


> +             xc_hypercall_buffer_free(_H(xch), ring);
> +             CAMLreturn(Val_unit);

I think CAMLnoreturn here.

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