7:12 a.m.
On 11/17/21 21:13, Eric Blake wrote:
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.
I'd slightly prefer long lines to be wrapped, wherever that's easy;
OTOH, I don't feel strongly enough about them to actually *request*
updates. :) I'm fine either way.
Acked-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks,
Laszlo