On Wed, Nov 17, 2021 at 05:58:53PM +0000, Richard W.M. Jones wrote:
Using the function code_of_unix_error from <caml/unixsupport.h> we can greatly simplify this function. code_of_unix_error was added in OCaml 4.01 which is ≤ 4.03 that we currently require.
See also: https://github.com/ocaml/ocaml/issues/4812
This does require a small change ot how OCaml plugins are linked -- we now need to link them with the standard OCaml Unix library (unix.cmxa).
This commit also adds a comprehensive end-to-end test of error codes. --- plugins/cc/nbdkit-cc-plugin.pod | 4 +- plugins/ocaml/nbdkit-ocaml-plugin.pod | 2 +- plugins/ocaml/Makefile.am | 2 +- tests/Makefile.am | 20 +++++- plugins/ocaml/NBDKit.ml | 25 +------ plugins/ocaml/bindings.c | 22 +------ tests/test-cc-ocaml.sh | 2 +- tests/cc_shebang.ml | 2 +- tests/test-ocaml-errorcodes.c | 95 +++++++++++++++++++++++++++ tests/test_ocaml_errorcodes_plugin.ml | 32 +++++++++ .gitignore | 1 + 11 files changed, 155 insertions(+), 52 deletions(-)
diff --git a/plugins/cc/nbdkit-cc-plugin.pod b/plugins/cc/nbdkit-cc-plugin.pod index 0fe0d9ea..be4019f9 100644 --- a/plugins/cc/nbdkit-cc-plugin.pod +++ b/plugins/cc/nbdkit-cc-plugin.pod @@ -89,7 +89,7 @@ C<CC=g++> as a parameter to exec nbdkit. =head2 Using this plugin with OCaml
nbdkit cc CC=ocamlopt \ - CFLAGS="-output-obj -runtime-variant _pic NBDKit.cmx -cclib -lnbdkitocaml" \ + CFLAGS="-output-obj -runtime-variant _pic unix.cmxa NBDKit.cmx -cclib -lnbdkitocaml" \
Worth adding another \-newline split on what is now a long line?
+++ b/plugins/ocaml/Makefile.am
+check_SCRIPTS += \ + test-ocaml-plugin.so \ + test-ocaml-errorcodes-plugin.so + test-ocaml-plugin.so: test_ocaml_plugin.cmx ../plugins/ocaml/libnbdkitocaml.la ../plugins/ocaml/NBDKit.cmi ../plugins/ocaml/NBDKit.cmx
Less important that the doc long line, but another place where we may want to consider line splitting or the use of a convenience Makefile variable to reduce the line length...
$(OCAMLOPT) $(OCAMLOPTFLAGS) -I ../plugins/ocaml \ -output-obj -runtime-variant _pic -o $@ \ - NBDKit.cmx $< \ + unix.cmxa NBDKit.cmx $< \ -cclib -L../plugins/ocaml/.libs -cclib -lnbdkitocaml test_ocaml_plugin.cmx: test_ocaml_plugin.ml ../plugins/ocaml/NBDKit.cmi ../plugins/ocaml/NBDKit.cmx $(OCAMLOPT) $(OCAMLOPTFLAGS) -I ../plugins/ocaml -c $< -o $@
+test-ocaml-errorcodes-plugin.so: test_ocaml_errorcodes_plugin.cmx ../plugins/ocaml/libnbdkitocaml.la ../plugins/ocaml/NBDKit.cmi ../plugins/ocaml/NBDKit.cmx
...particularly since you are repeating some of the same prerequisites in multiple targets. Maybe: TEST_OCAML_OBJS = \ ../plugins/ocaml/libnbdkitocaml.la \ ../plugins/ocaml/NBDKit.cmi \ ../plugins/ocaml/NBDKit.cmx \ $(NULL) test-ocaml-errorcodes-plugin.so: test_ocaml_errorcodes_plugin.cmx $(TEST_OCAML_OBJS)
+++ b/plugins/ocaml/NBDKit.ml @@ -220,30 +220,7 @@ let register_plugin plugin =
(* Bindings to nbdkit server functions. *)
-external _set_error : int -> unit = "ocaml_nbdkit_set_error" [@@noalloc] - -let set_error unix_error = - (* There's an awkward triple translation going on here, because - * OCaml Unix.error codes, errno on the host system, and NBD_* - * errnos are not all the same integer value. Plus we cannot - * read the host system errno values from OCaml. - *) - let nbd_error = - match unix_error with - | Unix.EPERM -> 1 - | Unix.EIO -> 2 - | Unix.ENOMEM -> 3 - | Unix.EINVAL -> 4 - | Unix.ENOSPC -> 5 - | Unix.ESHUTDOWN -> 6 - | Unix.EOVERFLOW -> 7 - | Unix.EOPNOTSUPP -> 8 - | Unix.EROFS -> 9 - | Unix.EFBIG -> 10 - | _ -> 4 (* EINVAL *) in - - _set_error nbd_error - +external set_error : Unix.error -> unit = "ocaml_nbdkit_set_error" [@@noalloc]
Yep, that's simpler.
+++ b/tests/test-cc-ocaml.sh @@ -58,6 +58,6 @@ cleanup_fn rm -f $out rm -f $out
nbdkit -U - cc $script a=1 b=2 c=3 \ - CC="$OCAMLOPT" CFLAGS="-output-obj -runtime-variant _pic -I $SRCDIR/../plugins/ocaml NBDKit.cmx -cclib -lnbdkitocaml" \ + CC="$OCAMLOPT" CFLAGS="-output-obj -runtime-variant _pic -I $SRCDIR/../plugins/ocaml unix.cmxa NBDKit.cmx -cclib -lnbdkitocaml" \
Here, it's harder to split a long line, as you really do have CC="lots of text" Series looks good to me, once you decide how you want to handle long lines. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org