Re: [Libguestfs] [nbdkit PATCH] ocaml: Add support for dynamic .thread_model
On 8/15/19 4:52 PM, Eric Blake wrote:
> We do not promise API stability for non-C languages; this is an API
> break as follows: instead of calling 'NBDKit.register_plugin model
> plugin' with a static model, you can now add .thread_model :(unit ->
> thread_model) to plugin or default to PARALLEL.
>
> Since all existing OCaml plugins will have already thought about
> thread models, they can convert their existing model into the new
> plugin field (and thus, I don't feel too bad making PARALLEL the
> default, even if it is not always the safest).
>
> Signed-off-by: Eric Blake
> ---
>
> I'm still looking at two followups:
> 1) ./nbdkit doesn't set LD_LIBRARY_PATH=plugins/ocaml/.libs:$LD_LIBRARY_PATH
> (making ./nbdkit --dump-plugin tests/test-ocaml-plugin.so fail to load
> when the system nbdkit is too old)
> 2) although --dump-plugin shows thread model, ./nbdkit -v log does not;
> I need to add a debug() statement for that in server/locks.c
Doing this proved I had a copy-paste bug:
> +static int
> +thread_model_wrapper (void)
> +{
> + CAMLparam0 ();
> + CAMLlocal1 (rv);
> +
> + caml_leave_blocking_section ();
> +
> + rv = caml_callback_exn (config_complete_fn, Val_unit);
This needs to call thread_model_fn, not config_complete_fn; otherwise
the thread model is wrongly chosen between 0 or 1, and in the
--dump-plugin case I get an assertion in tests/test_ocaml_plugin.ml when
not providing the expected config parameters.
I've now pushed fixes for the two noted improvements, as well as this
patch as fixed.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [nbdkit PATCH] ocaml: Add support for dynamic .thread_model
On Thu, Aug 15, 2019 at 04:52:00PM -0500, Eric Blake wrote:
> We do not promise API stability for non-C languages; this is an API
> break as follows: instead of calling 'NBDKit.register_plugin model
> plugin' with a static model, you can now add .thread_model :(unit ->
> thread_model) to plugin or default to PARALLEL.
>
> Since all existing OCaml plugins will have already thought about
> thread models, they can convert their existing model into the new
> plugin field (and thus, I don't feel too bad making PARALLEL the
> default, even if it is not always the safest).
>
> Signed-off-by: Eric Blake
Patch looks OK. It would crash pretty early if there was something
wrong in the bindings, so ACK.
> I'm still looking at two followups:
> 1) ./nbdkit doesn't set LD_LIBRARY_PATH=plugins/ocaml/.libs:$LD_LIBRARY_PATH
> (making ./nbdkit --dump-plugin tests/test-ocaml-plugin.so fail to load
> when the system nbdkit is too old)
Yes this is surely a bug. After making that change the line can also
be removed from tests/Makefile.am.
> 2) although --dump-plugin shows thread model, ./nbdkit -v log does not;
> I need to add a debug() statement for that in server/locks.c
Yes I guess it's a good idea to add it to the debug output. Helps
with checking that we really got the intended thread model in all
circumstances.
Thanks,
Rich.
> But I was quite pleased that I got this working in under 3 hours (I'm
> getting better at OCaml).
>
> plugins/ocaml/nbdkit-ocaml-plugin.pod | 13 ++-
> plugins/ocaml/ocaml.c | 33 +--
> plugins/ocaml/NBDKit.ml | 28 ++-
> plugins/ocaml/NBDKit.mli | 19 ---
> plugins/ocaml/example.ml | 9 +---
> tests/test_ocaml_plugin.ml| 5 ++--
> 6 files changed, 69 insertions(+), 38 deletions(-)
>
> diff --git a/plugins/ocaml/nbdkit-ocaml-plugin.pod
> b/plugins/ocaml/nbdkit-ocaml-plugin.pod
> index a66cf26e..4b349612 100644
> --- a/plugins/ocaml/nbdkit-ocaml-plugin.pod
> +++ b/plugins/ocaml/nbdkit-ocaml-plugin.pod
> @@ -36,12 +36,11 @@ Your OCaml code should call C
> like this:
> open_connection = Some myplugin_open;
> get_size= Some myplugin_get_size;
> pread = Some myplugin_pread;
> + thread_model= Some (fun () ->
> NBDKit.THREAD_MODEL_SERIALIZE_CONNECTIONS);
> (* etc *)
> }
>
> - let thread_model = NBDKit.THREAD_MODEL_SERIALIZE_CONNECTIONS
> -
> - let () = NBDKit.register_plugin thread_model plugin
> + let () = NBDKit.register_plugin plugin
>
> Your plugin must call C exactly once when the plugin
> is loaded.
> @@ -108,9 +107,11 @@ to control this.
>
> =head2 Threads
>
> -The first parameter of C is the thread model,
> -which can be one of the values in the table below. For more
> -information on thread models, see L. Note
> +One of the members in the plugin record passed to
> +C is C, which must return one of
> +the values in the table below. For more information on thread models,
> +see L. If this optional function is not
> +provided, the thread model defaults to THREAD_MODEL_PARALLEL. Note
> that because of the garbage collector lock in OCaml, callbacks are
> never truly concurrent.
>
> diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c
> index f664a7fb..01f4448f 100644
> --- a/plugins/ocaml/ocaml.c
> +++ b/plugins/ocaml/ocaml.c
> @@ -72,6 +72,7 @@ static void remove_roots (void);
> static struct nbdkit_plugin plugin = {
>._struct_size = sizeof (plugin),
>._api_version = NBDKIT_API_VERSION,
> + ._thread_model = NBDKIT_THREAD_MODEL_PARALLEL,
>
>/* The following field is used as a canary to detect whether the
> * OCaml code started up and called us back successfully. If it's
> @@ -131,6 +132,8 @@ static value extents_fn;
> static value can_cache_fn;
> static value cache_fn;
>
> +static value thread_model_fn;
> +
> /*--*/
> /* Wrapper functions that translate calls from C (ie. nbdkit) to OCaml. */
>
> @@ -683,18 +686,30 @@ cache_wrapper (void *h, uint32_t count, uint64_t
> offset, uint32_t flags)
>CAMLreturnT (int, 0);
> }
>
> +static int
> +thread_model_wrapper (void)
> +{
> + CAMLparam0 ();
> + CAMLlocal1 (rv);
> +
> + caml_leave_blocking_section ();
> +
> + rv = caml_callback_exn (config_complete_fn, Val_unit);
> + if (Is_exception_result (rv)) {
> +nbdkit_error ("%s", caml_format_exception (Extract_exception (rv)));
> +caml_enter_blocking_section ();
> +CAMLreturnT (int, -1);
> + }
> +
> + caml_enter_blocking_section ();
> + CAMLreturnT (int, Int_val (rv));
> +}
> +
> /*--*/
> /* set_* functions called from OCaml code at load time to initialize
> * fields in the plugin struct.
> */
>
> -value
> -ocaml_nbdkit_set_thread_model (value modelv)
> -{
> - plugin._thread_model =
[Libguestfs] [nbdkit PATCH] ocaml: Add support for dynamic .thread_model
We do not promise API stability for non-C languages; this is an API
break as follows: instead of calling 'NBDKit.register_plugin model
plugin' with a static model, you can now add .thread_model :(unit ->
thread_model) to plugin or default to PARALLEL.
Since all existing OCaml plugins will have already thought about
thread models, they can convert their existing model into the new
plugin field (and thus, I don't feel too bad making PARALLEL the
default, even if it is not always the safest).
Signed-off-by: Eric Blake
---
I'm still looking at two followups:
1) ./nbdkit doesn't set LD_LIBRARY_PATH=plugins/ocaml/.libs:$LD_LIBRARY_PATH
(making ./nbdkit --dump-plugin tests/test-ocaml-plugin.so fail to load
when the system nbdkit is too old)
2) although --dump-plugin shows thread model, ./nbdkit -v log does not;
I need to add a debug() statement for that in server/locks.c
But I was quite pleased that I got this working in under 3 hours (I'm
getting better at OCaml).
plugins/ocaml/nbdkit-ocaml-plugin.pod | 13 ++-
plugins/ocaml/ocaml.c | 33 +--
plugins/ocaml/NBDKit.ml | 28 ++-
plugins/ocaml/NBDKit.mli | 19 ---
plugins/ocaml/example.ml | 9 +---
tests/test_ocaml_plugin.ml| 5 ++--
6 files changed, 69 insertions(+), 38 deletions(-)
diff --git a/plugins/ocaml/nbdkit-ocaml-plugin.pod
b/plugins/ocaml/nbdkit-ocaml-plugin.pod
index a66cf26e..4b349612 100644
--- a/plugins/ocaml/nbdkit-ocaml-plugin.pod
+++ b/plugins/ocaml/nbdkit-ocaml-plugin.pod
@@ -36,12 +36,11 @@ Your OCaml code should call C like
this:
open_connection = Some myplugin_open;
get_size= Some myplugin_get_size;
pread = Some myplugin_pread;
+ thread_model= Some (fun () ->
NBDKit.THREAD_MODEL_SERIALIZE_CONNECTIONS);
(* etc *)
}
- let thread_model = NBDKit.THREAD_MODEL_SERIALIZE_CONNECTIONS
-
- let () = NBDKit.register_plugin thread_model plugin
+ let () = NBDKit.register_plugin plugin
Your plugin must call C exactly once when the plugin
is loaded.
@@ -108,9 +107,11 @@ to control this.
=head2 Threads
-The first parameter of C is the thread model,
-which can be one of the values in the table below. For more
-information on thread models, see L. Note
+One of the members in the plugin record passed to
+C is C, which must return one of
+the values in the table below. For more information on thread models,
+see L. If this optional function is not
+provided, the thread model defaults to THREAD_MODEL_PARALLEL. Note
that because of the garbage collector lock in OCaml, callbacks are
never truly concurrent.
diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c
index f664a7fb..01f4448f 100644
--- a/plugins/ocaml/ocaml.c
+++ b/plugins/ocaml/ocaml.c
@@ -72,6 +72,7 @@ static void remove_roots (void);
static struct nbdkit_plugin plugin = {
._struct_size = sizeof (plugin),
._api_version = NBDKIT_API_VERSION,
+ ._thread_model = NBDKIT_THREAD_MODEL_PARALLEL,
/* The following field is used as a canary to detect whether the
* OCaml code started up and called us back successfully. If it's
@@ -131,6 +132,8 @@ static value extents_fn;
static value can_cache_fn;
static value cache_fn;
+static value thread_model_fn;
+
/*--*/
/* Wrapper functions that translate calls from C (ie. nbdkit) to OCaml. */
@@ -683,18 +686,30 @@ cache_wrapper (void *h, uint32_t count, uint64_t offset,
uint32_t flags)
CAMLreturnT (int, 0);
}
+static int
+thread_model_wrapper (void)
+{
+ CAMLparam0 ();
+ CAMLlocal1 (rv);
+
+ caml_leave_blocking_section ();
+
+ rv = caml_callback_exn (config_complete_fn, Val_unit);
+ if (Is_exception_result (rv)) {
+nbdkit_error ("%s", caml_format_exception (Extract_exception (rv)));
+caml_enter_blocking_section ();
+CAMLreturnT (int, -1);
+ }
+
+ caml_enter_blocking_section ();
+ CAMLreturnT (int, Int_val (rv));
+}
+
/*--*/
/* set_* functions called from OCaml code at load time to initialize
* fields in the plugin struct.
*/
-value
-ocaml_nbdkit_set_thread_model (value modelv)
-{
- plugin._thread_model = Int_val (modelv);
- return Val_unit;
-}
-
value
ocaml_nbdkit_set_name (value namev)
{
@@ -775,6 +790,8 @@ SET(extents)
SET(can_cache)
SET(cache)
+SET(thread_model)
+
#undef SET
static void
@@ -817,6 +834,8 @@ remove_roots (void)
REMOVE (can_cache);
REMOVE (cache);
+ REMOVE (thread_model);
+
#undef REMOVE
}
diff --git a/plugins/ocaml/NBDKit.ml b/plugins/ocaml/NBDKit.ml
index 02aa2001..57e57a46 100644
--- a/plugins/ocaml/NBDKit.ml
+++ b/plugins/ocaml/NBDKit.ml
@@ -1,3 +1,4 @@
+(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* nbdkit OCaml interface
* Copyright (C) 2014-2019 Red Hat Inc.
*
@@ -39,6 +40,12 @@ type fua_flag = FuaNone |
