[Libguestfs] [PATCH nbdkit v2 2/2] ocaml: Simplify NBDKit.set_error

[Eric Blake]

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