Richard W.M. Jones
2021-Nov-17 17:58 UTC
[Libguestfs] [PATCH nbdkit v2 2/2] ocaml: Simplify NBDKit.set_error
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" \
source.ml
OCaml plugin scripts can be created using this trick:
@@ -97,7 +97,7 @@ OCaml plugin scripts can be created using this trick:
(*/.)>/dev/null 2>&1
exec nbdkit cc "$0" \
CC=ocamlopt \
- CFLAGS="-output-obj -runtime-variant _pic NBDKit.cmx -cclib
-lnbdkitocaml" \
+ CFLAGS="-output-obj -runtime-variant _pic unix.cmxa NBDKit.cmx
-cclib -lnbdkitocaml" \
"$@"
*)
(* followed by OCaml code for the plugin here *)
diff --git a/plugins/ocaml/nbdkit-ocaml-plugin.pod
b/plugins/ocaml/nbdkit-ocaml-plugin.pod
index 293f8143..efeb2240 100644
--- a/plugins/ocaml/nbdkit-ocaml-plugin.pod
+++ b/plugins/ocaml/nbdkit-ocaml-plugin.pod
@@ -53,7 +53,7 @@ using this command:
ocamlopt.opt -output-obj -runtime-variant _pic \
-o nbdkit-myplugin-plugin.so \
- NBDKit.cmx myplugin.ml \
+ unix.cmxa NBDKit.cmx myplugin.ml \
-cclib -lnbdkitocaml
You can then use C<nbdkit-myplugin-plugin.so> as an nbdkit plugin (see
diff --git a/plugins/ocaml/Makefile.am b/plugins/ocaml/Makefile.am
index 1082fc0a..fcf3396d 100644
--- a/plugins/ocaml/Makefile.am
+++ b/plugins/ocaml/Makefile.am
@@ -81,7 +81,7 @@ noinst_SCRIPTS = nbdkit-ocamlexample-plugin.so
nbdkit-ocamlexample-plugin.so: example.cmx libnbdkitocaml.la NBDKit.cmi
NBDKit.cmx
$(OCAMLOPT) $(OCAMLOPTFLAGS) \
-output-obj -runtime-variant _pic -o $@ \
- NBDKit.cmx $< \
+ unix.cmxa NBDKit.cmx $< \
-cclib -L.libs -cclib -lnbdkitocaml
example.cmx: example.ml NBDKit.cmi NBDKit.cmx
$(OCAMLOPT) $(OCAMLOPTFLAGS) -c $< -o $@
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 07b09207..912e8b3f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1067,6 +1067,7 @@ EXTRA_DIST += test-zero.sh
if HAVE_OCAML
LIBGUESTFS_TESTS += test-ocaml
+LIBNBD_TESTS += test-ocaml-errorcodes
test_ocaml_SOURCES = test-ocaml.c test.h
test_ocaml_CFLAGS = \
@@ -1075,15 +1076,30 @@ test_ocaml_CFLAGS = \
$(NULL)
test_ocaml_LDADD = libtest.la $(LIBGUESTFS_LIBS)
-check_SCRIPTS += test-ocaml-plugin.so
+test_ocaml_errorcodes_SOURCES = test-ocaml-errorcodes.c
+test_ocaml_errorcodes_CFLAGS = $(WARNINGS_CFLAGS) $(LIBNBD_CFLAGS)
+test_ocaml_errorcodes_LDADD = $(LIBNBD_LIBS)
+
+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
$(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
+ $(OCAMLOPT) $(OCAMLOPTFLAGS) -I ../plugins/ocaml \
+ -output-obj -runtime-variant _pic -o $@ \
+ unix.cmxa NBDKit.cmx $< \
+ -cclib -L../plugins/ocaml/.libs -cclib -lnbdkitocaml
+test_ocaml_errorcodes_plugin.cmx: test_ocaml_errorcodes_plugin.ml
../plugins/ocaml/NBDKit.cmi ../plugins/ocaml/NBDKit.cmx
+ $(OCAMLOPT) $(OCAMLOPTFLAGS) -I ../plugins/ocaml -c $< -o $@
+
endif HAVE_OCAML
EXTRA_DIST += \
diff --git a/plugins/ocaml/NBDKit.ml b/plugins/ocaml/NBDKit.ml
index c9ce31b5..d28992c8 100644
--- a/plugins/ocaml/NBDKit.ml
+++ 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]
external parse_size : string -> int64 = "ocaml_nbdkit_parse_size"
external parse_bool : string -> bool = "ocaml_nbdkit_parse_bool"
external read_password : string -> string =
"ocaml_nbdkit_read_password"
diff --git a/plugins/ocaml/bindings.c b/plugins/ocaml/bindings.c
index a6d57084..ba95fb4a 100644
--- a/plugins/ocaml/bindings.c
+++ b/plugins/ocaml/bindings.c
@@ -42,6 +42,7 @@
#include <caml/memory.h>
#include <caml/mlvalues.h>
#include <caml/threads.h>
+#include <caml/unixsupport.h>
#define NBDKIT_API_VERSION 2
#include <nbdkit-plugin.h>
@@ -54,26 +55,7 @@
NBDKIT_DLL_PUBLIC value
ocaml_nbdkit_set_error (value nv)
{
- int err;
-
- switch (Int_val (nv)) {
- /* Host errno values that will map to NBD protocol values */
- case 1: err = EPERM; break;
- case 2: err = EIO; break;
- case 3: err = ENOMEM; break;
- case 4: err = EINVAL; break;
- case 5: err = ENOSPC; break;
- case 6: err = ESHUTDOWN; break;
- case 7: err = EOVERFLOW; break;
- case 8: err = EOPNOTSUPP; break;
- /* Other errno values that server/protocol.c treats specially */
- case 9: err = EROFS; break;
- case 10: err = EFBIG; break;
- default: abort ();
- }
-
- nbdkit_set_error (err);
-
+ nbdkit_set_error (code_of_unix_error (nv));
return Val_unit;
}
diff --git a/tests/test-cc-ocaml.sh b/tests/test-cc-ocaml.sh
index e47bf26f..9ba7ee98 100755
--- a/tests/test-cc-ocaml.sh
+++ 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" \
--run 'nbdinfo --size $uri' > $out
test "$(cat $out)" -eq $((512 * 2048))
diff --git a/tests/cc_shebang.ml b/tests/cc_shebang.ml
index 05036284..a6c79039 100755
--- a/tests/cc_shebang.ml
+++ b/tests/cc_shebang.ml
@@ -4,7 +4,7 @@
# shell as an impossible command which is ignored. The line below is
# run by the shell and ignored by OCaml.
-exec nbdkit cc "$0" CC=ocamlopt CFLAGS="-output-obj
-runtime-variant _pic NBDKit.cmx -cclib -lnbdkitocaml" "$@"
+exec nbdkit cc "$0" CC=ocamlopt CFLAGS="-output-obj
-runtime-variant _pic unix.cmxa NBDKit.cmx -cclib -lnbdkitocaml"
"$@"
*)
open Printf
diff --git a/tests/test-ocaml-errorcodes.c b/tests/test-ocaml-errorcodes.c
new file mode 100644
index 00000000..a7a3f125
--- /dev/null
+++ b/tests/test-ocaml-errorcodes.c
@@ -0,0 +1,95 @@
+/* nbdkit
+ * Copyright (C) 2013-2021 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <assert.h>
+
+#include <libnbd.h>
+
+/* This test checks the conversion from OCaml Unix.error to errno (in
+ * the plugin) to NBD_E* (over the wire) and back to errno (in
+ * libnbd).
+ *
+ * Reading at various sector offsets in the associated plugin
+ * (test_ocaml_errorcodes_plugin.ml) produces predictable error codes.
+ */
+static struct { uint64_t offset; int expected_errno; } tests[] = {
+ { 1*512, EPERM },
+ { 2*512, EIO },
+ { 3*512, ENOMEM },
+ { 4*512, ESHUTDOWN },
+ { 5*512, EINVAL },
+ { 0 }
+};
+
+int
+main (int argc, char *argv[])
+{
+ struct nbd_handle *nbd;
+ char buf[512];
+ size_t i;
+ int actual_errno;
+
+ nbd = nbd_create ();
+ if (nbd == NULL) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
+ if (nbd_connect_command (nbd,
+ (char *[]) {
+ "nbdkit", "-s",
"--exit-with-parent",
+ "./test-ocaml-errorcodes-plugin.so",
+ NULL }) == -1) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
+ assert (nbd_pread (nbd, buf, 512, 0, 0) == 0);
+
+ for (i = 0; tests[i].offset != 0; ++i) {
+ assert (nbd_pread (nbd, buf, 512, tests[i].offset, 0) == -1);
+ actual_errno = nbd_get_errno ();
+ if (actual_errno != tests[i].expected_errno) {
+ fprintf (stderr, "%s: FAIL: actual errno = %d expected errno =
%d\n",
+ argv[0], actual_errno, tests[i].expected_errno);
+ exit (EXIT_FAILURE);
+ }
+ }
+
+ nbd_close (nbd);
+ exit (EXIT_SUCCESS);
+}
diff --git a/tests/test_ocaml_errorcodes_plugin.ml
b/tests/test_ocaml_errorcodes_plugin.ml
new file mode 100644
index 00000000..4b128846
--- /dev/null
+++ b/tests/test_ocaml_errorcodes_plugin.ml
@@ -0,0 +1,32 @@
+open Unix
+
+let sector_size = 512
+
+let open_connection _ = ()
+
+let get_size () = Int64.of_int (6 * sector_size)
+
+let pread () count offset _ + (* Depending on the sector requested (offset),
return a different
+ * error code.
+ *)
+ match (Int64.to_int offset) / sector_size with
+ | 0 -> (* good, return data *) String.make (Int32.to_int count)
'\000'
+ | 1 -> NBDKit.set_error EPERM; failwith "EPERM"
+ | 2 -> NBDKit.set_error EIO; failwith "EIO"
+ | 3 -> NBDKit.set_error ENOMEM; failwith "ENOMEM"
+ | 4 -> NBDKit.set_error ESHUTDOWN; failwith "ESHUTDOWN"
+ | 5 -> NBDKit.set_error EINVAL; failwith "EINVAL"
+ | _ -> assert false
+
+let plugin = {
+ NBDKit.default_callbacks with
+ NBDKit.name = "test-ocaml-errorcodes";
+ version = NBDKit.version ();
+
+ open_connection = Some open_connection;
+ get_size = Some get_size;
+ pread = Some pread;
+}
+
+let () = NBDKit.register_plugin plugin
diff --git a/.gitignore b/.gitignore
index 91cbc810..4e2ae75d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -146,6 +146,7 @@ plugins/*/*.3
/tests/test-nbd
/tests/test-null
/tests/test-ocaml
+/tests/test-ocaml-errorcodes
/tests/test-offset
/tests/test-oldstyle
/tests/test-old-plugins-*.sh
--
2.32.0
Eric Blake
2021-Nov-17 20:13 UTC
[Libguestfs] [PATCH nbdkit v2 2/2] ocaml: Simplify NBDKit.set_error
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.cmxLess 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