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 <[email protected]> Thanks, Laszlo _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
