Andrew Cooper
2013-Mar-13 12:52 UTC
[PATCH 4 of 5 v4] 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 v3:
* Use caml_alloc_string/memcpy in preference to caml_copy_string to allow
for the possibility of NULL characters in the middle of the conring
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 5bd08f35b362 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));
+ conring = caml_alloc_string(nr_chars);
+ memcpy(String_val(conring), ring, nr_chars);
+ xc_hypercall_buffer_free(_H(xch), ring);
+ CAMLreturn(conring);
}
CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
Dave Scott
2013-Mar-13 15:19 UTC
Re: [PATCH 4 of 5 v4] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()
This looks good to me. Acked-by: Dave Scott <dave.scott@eu.citrix.com>> -----Original Message----- > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- > bounces@lists.xen.org] On Behalf Of Andrew Cooper > Sent: 13 March 2013 12:52 PM > To: xen-devel@lists.xen.org > Cc: Ian Jackson; Keir (Xen.org); Ian Campbell; Jan Beulich; xen- > api@lists.xen.org > Subject: [Xen-devel] [PATCH 4 of 5 v4] 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 v3: > * Use caml_alloc_string/memcpy in preference to caml_copy_string to > allow > for the possibility of NULL characters in the middle of the conring 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 5bd08f35b362 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)); > + conring = caml_alloc_string(nr_chars); > + memcpy(String_val(conring), ring, nr_chars); > + 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@lists.xen.org > http://lists.xen.org/xen-devel