[PATCH nbdkit v2 0/2] ocaml: Simplify NBDKit.set_error - Libguestfs - Libguestfs List Archives

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

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

specifying a standard VGA video...

[PATCH libnbd 0/6] Enhance...

Richard W.M. Jones

Wednesday, 17 November 2021 Wed, 17 Nov '21

11:58 a.m.

v1 was here: https://listman.redhat.com/archives/libguestfs/2021-November/msg00176.html This version addresses all or very nearly all of the feedback. I already posted v2 1/2, but including it again here since it is necessary for the 2/2 to make sense. Rich.

Reply

Show replies by date

Richard W.M. Jones

Wednesday, 17 November Wed, 17 Nov

11:58 a.m.

New subject: [PATCH nbdkit v2 1/2] tests: Use compound literal array for nbd_connect_command parameter

Instead of having a separate char *args[] local variable, we can use a compound literal array (C99 feature). This change is just refactoring. --- tests/README.tests | 7 ++++--- tests/test-connect.c | 8 ++++---- tests/test-curl-cookie-script.c | 21 ++++++++++----------- tests/test-curl-header-script.c | 21 ++++++++++----------- tests/test-delay.c | 13 ++++++------- tests/test-layers.c | 27 ++++++++++++++------------- tests/test-newstyle.c | 10 +++++----- tests/test-null.c | 8 +++++--- tests/test-oldstyle.c | 10 +++++----- tests/test-pause.c | 11 ++++++----- tests/test-random.c | 9 +++++---- tests/test-split.c | 12 ++++++------ 12 files changed, 80 insertions(+), 77 deletions(-) diff --git a/tests/README.tests b/tests/README.tests index 595c3c19..a55e6958 100644 --- a/tests/README.tests +++ b/tests/README.tests @@ -65,9 +65,10 @@ To test a plugin using libnbd Open a libnbd handle, and configure it using: - char *args[] = { "nbdkit", "-s", "--exit-with-parent", - "plugin", <plugin args ...>, NULL }; - nbd_connect_command (nbd, args); + nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "plugin", <plugin args ...>, NULL }); Perform tests via libnbd functions. diff --git a/tests/test-connect.c b/tests/test-connect.c index f6b494ac..13143f46 100644 --- a/tests/test-connect.c +++ b/tests/test-connect.c @@ -53,10 +53,10 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "example1", NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "example1", NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-curl-cookie-script.c b/tests/test-curl-cookie-script.c index 481207b5..45e3d136 100644 --- a/tests/test-curl-cookie-script.c +++ b/tests/test-curl-cookie-script.c @@ -104,17 +104,16 @@ main (int argc, char *argv[]) perror ("asprintf"); exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "-v", - "curl", - "-D", "curl.verbose=1", - "http://localhost/disk", - "cookie-script=" SCRIPT, - "cookie-script-renew=1", - usp_param, /* unix-socket-path=... */ - NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", "-v", + "curl", + "-D", "curl.verbose=1", + "http://localhost/disk", + "cookie-script=" SCRIPT, + "cookie-script-renew=1", + usp_param, /* unix-socket-path=... */ + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-curl-header-script.c b/tests/test-curl-header-script.c index a151af05..afe16591 100644 --- a/tests/test-curl-header-script.c +++ b/tests/test-curl-header-script.c @@ -126,17 +126,16 @@ main (int argc, char *argv[]) perror ("asprintf"); exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "-v", - "curl", - "-D", "curl.verbose=1", - "http://localhost/disk", - "header-script=" SCRIPT, - "header-script-renew=1", - usp_param, /* unix-socket-path=... */ - NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", "-v", + "curl", + "-D", "curl.verbose=1", + "http://localhost/disk", + "header-script=" SCRIPT, + "header-script-renew=1", + usp_param, /* unix-socket-path=... */ + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-delay.c b/tests/test-delay.c index 736de529..b0e6f8de 100644 --- a/tests/test-delay.c +++ b/tests/test-delay.c @@ -56,13 +56,12 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", - "--filter", "delay", - "memory", "1M", - "wdelay=10", NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "--filter", "delay", + "memory", "1M", + "wdelay=10", NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-layers.c b/tests/test-layers.c index 13c82289..0083b733 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -157,19 +157,20 @@ main (int argc, char *argv[]) } /* Start nbdkit. */ - char *args[] = { - "nbdkit", "--exit-with-parent", "-fvns", - /* Because of asynchronous shutdown with threads, finalize - * isn't reliably called unless we disable parallel. - */ - "-t", "1", - "--filter", ".libs/test-layers-filter3." SOEXT, - "--filter", ".libs/test-layers-filter2." SOEXT, - "--filter", ".libs/test-layers-filter1." SOEXT, - ".libs/test-layers-plugin." SOEXT, - "foo=bar", - NULL}; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "--exit-with-parent", "-fvns", + /* Because of asynchronous shutdown with + * threads, finalize isn't reliably + * called unless we disable parallel. + */ + "-t", "1", + "--filter", ".libs/test-layers-filter3." SOEXT, + "--filter", ".libs/test-layers-filter2." SOEXT, + "--filter", ".libs/test-layers-filter1." SOEXT, + ".libs/test-layers-plugin." SOEXT, + "foo=bar", + NULL }) == -1) { dprintf (orig_stderr, "nbd_connect_command: %s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-newstyle.c b/tests/test-newstyle.c index d96c7e44..1f5d1ca3 100644 --- a/tests/test-newstyle.c +++ b/tests/test-newstyle.c @@ -49,11 +49,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", - "--newstyle", "file", "file-data", NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "--newstyle", "file", "file-data", + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-null.c b/tests/test-null.c index 2205934f..d220999a 100644 --- a/tests/test-null.c +++ b/tests/test-null.c @@ -52,9 +52,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = - { "nbdkit", "-s", "--exit-with-parent", "null", "100M", NULL }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "null", "100M", + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-oldstyle.c b/tests/test-oldstyle.c index 5147768a..0afe868f 100644 --- a/tests/test-oldstyle.c +++ b/tests/test-oldstyle.c @@ -49,11 +49,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", - "--oldstyle", "file", "file-data", NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "--oldstyle", "file", "file-data", + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-pause.c b/tests/test-pause.c index 3c8ee520..e5a970d6 100644 --- a/tests/test-pause.c +++ b/tests/test-pause.c @@ -78,11 +78,12 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "--filter", "pause", - "example1", "pause-control=" SOCKET, NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "--filter", "pause", + "example1", "pause-control=" SOCKET, + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-random.c b/tests/test-random.c index 4316d2b3..b7716efa 100644 --- a/tests/test-random.c +++ b/tests/test-random.c @@ -72,10 +72,11 @@ main (int argc, char *argv[]) } snprintf (sizearg, sizeof sizearg, "%d", SIZE); - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "random", sizearg, NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "random", sizearg, + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-split.c b/tests/test-split.c index be53590d..657e5b5a 100644 --- a/tests/test-split.c +++ b/tests/test-split.c @@ -51,12 +51,12 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", - "split", "split1", "split2", "file=split3" /* leave file= to test */, - NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "split", "split1", "split2", + "file=split3" /* leave file= to test */, + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } -- 2.32.0

Reply

Laszlo Ersek

Thursday, 18 November Thu, 18 Nov

7:08 a.m.

New subject: [PATCH nbdkit v2 1/2] tests: Use compound literal array for nbd_connect_command parameter

On 11/17/21 18:58, Richard W.M. Jones wrote:

Instead of having a separate char *args[] local variable, we can use a compound literal array (C99 feature). This change is just refactoring. --- tests/README.tests | 7 ++++--- tests/test-connect.c | 8 ++++---- tests/test-curl-cookie-script.c | 21 ++++++++++----------- tests/test-curl-header-script.c | 21 ++++++++++----------- tests/test-delay.c | 13 ++++++------- tests/test-layers.c | 27 ++++++++++++++------------- tests/test-newstyle.c | 10 +++++----- tests/test-null.c | 8 +++++--- tests/test-oldstyle.c | 10 +++++----- tests/test-pause.c | 11 ++++++----- tests/test-random.c | 9 +++++---- tests/test-split.c | 12 ++++++------ 12 files changed, 80 insertions(+), 77 deletions(-)

This is the kind of refactoring that I can no longer meticulously review, so if you were malicious, you could drive a truck through it... But if you were malicious, this refactoring wouldn't be our biggest problem! :) Acked-by: Laszlo Ersek <lersek(a)redhat.com> Thanks, Laszlo

diff --git a/tests/README.tests b/tests/README.tests index 595c3c19..a55e6958 100644 --- a/tests/README.tests +++ b/tests/README.tests @@ -65,9 +65,10 @@ To test a plugin using libnbd Open a libnbd handle, and configure it using: - char *args[] = { "nbdkit", "-s", "--exit-with-parent", - "plugin", <plugin args ...>, NULL }; - nbd_connect_command (nbd, args); + nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "plugin", <plugin args ...>, NULL }); Perform tests via libnbd functions. diff --git a/tests/test-connect.c b/tests/test-connect.c index f6b494ac..13143f46 100644 --- a/tests/test-connect.c +++ b/tests/test-connect.c @@ -53,10 +53,10 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "example1", NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "example1", NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-curl-cookie-script.c b/tests/test-curl-cookie-script.c index 481207b5..45e3d136 100644 --- a/tests/test-curl-cookie-script.c +++ b/tests/test-curl-cookie-script.c @@ -104,17 +104,16 @@ main (int argc, char *argv[]) perror ("asprintf"); exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "-v", - "curl", - "-D", "curl.verbose=1", - "http://localhost/disk", - "cookie-script=" SCRIPT, - "cookie-script-renew=1", - usp_param, /* unix-socket-path=... */ - NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", "-v", + "curl", + "-D", "curl.verbose=1", + "http://localhost/disk", + "cookie-script=" SCRIPT, + "cookie-script-renew=1", + usp_param, /* unix-socket-path=... */ + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-curl-header-script.c b/tests/test-curl-header-script.c index a151af05..afe16591 100644 --- a/tests/test-curl-header-script.c +++ b/tests/test-curl-header-script.c @@ -126,17 +126,16 @@ main (int argc, char *argv[]) perror ("asprintf"); exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "-v", - "curl", - "-D", "curl.verbose=1", - "http://localhost/disk", - "header-script=" SCRIPT, - "header-script-renew=1", - usp_param, /* unix-socket-path=... */ - NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", "-v", + "curl", + "-D", "curl.verbose=1", + "http://localhost/disk", + "header-script=" SCRIPT, + "header-script-renew=1", + usp_param, /* unix-socket-path=... */ + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-delay.c b/tests/test-delay.c index 736de529..b0e6f8de 100644 --- a/tests/test-delay.c +++ b/tests/test-delay.c @@ -56,13 +56,12 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", - "--filter", "delay", - "memory", "1M", - "wdelay=10", NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "--filter", "delay", + "memory", "1M", + "wdelay=10", NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-layers.c b/tests/test-layers.c index 13c82289..0083b733 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -157,19 +157,20 @@ main (int argc, char *argv[]) } /* Start nbdkit. */ - char *args[] = { - "nbdkit", "--exit-with-parent", "-fvns", - /* Because of asynchronous shutdown with threads, finalize - * isn't reliably called unless we disable parallel. - */ - "-t", "1", - "--filter", ".libs/test-layers-filter3." SOEXT, - "--filter", ".libs/test-layers-filter2." SOEXT, - "--filter", ".libs/test-layers-filter1." SOEXT, - ".libs/test-layers-plugin." SOEXT, - "foo=bar", - NULL}; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "--exit-with-parent", "-fvns", + /* Because of asynchronous shutdown with + * threads, finalize isn't reliably + * called unless we disable parallel. + */ + "-t", "1", + "--filter", ".libs/test-layers-filter3." SOEXT, + "--filter", ".libs/test-layers-filter2." SOEXT, + "--filter", ".libs/test-layers-filter1." SOEXT, + ".libs/test-layers-plugin." SOEXT, + "foo=bar", + NULL }) == -1) { dprintf (orig_stderr, "nbd_connect_command: %s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-newstyle.c b/tests/test-newstyle.c index d96c7e44..1f5d1ca3 100644 --- a/tests/test-newstyle.c +++ b/tests/test-newstyle.c @@ -49,11 +49,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", - "--newstyle", "file", "file-data", NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "--newstyle", "file", "file-data", + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-null.c b/tests/test-null.c index 2205934f..d220999a 100644 --- a/tests/test-null.c +++ b/tests/test-null.c @@ -52,9 +52,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = - { "nbdkit", "-s", "--exit-with-parent", "null", "100M", NULL }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "null", "100M", + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-oldstyle.c b/tests/test-oldstyle.c index 5147768a..0afe868f 100644 --- a/tests/test-oldstyle.c +++ b/tests/test-oldstyle.c @@ -49,11 +49,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", - "--oldstyle", "file", "file-data", NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "--oldstyle", "file", "file-data", + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-pause.c b/tests/test-pause.c index 3c8ee520..e5a970d6 100644 --- a/tests/test-pause.c +++ b/tests/test-pause.c @@ -78,11 +78,12 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "--filter", "pause", - "example1", "pause-control=" SOCKET, NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "--filter", "pause", + "example1", "pause-control=" SOCKET, + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-random.c b/tests/test-random.c index 4316d2b3..b7716efa 100644 --- a/tests/test-random.c +++ b/tests/test-random.c @@ -72,10 +72,11 @@ main (int argc, char *argv[]) } snprintf (sizearg, sizeof sizearg, "%d", SIZE); - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", "random", sizearg, NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "random", sizearg, + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/test-split.c b/tests/test-split.c index be53590d..657e5b5a 100644 --- a/tests/test-split.c +++ b/tests/test-split.c @@ -51,12 +51,12 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - char *args[] = { - "nbdkit", "-s", "--exit-with-parent", - "split", "split1", "split2", "file=split3" /* leave file= to test */, - NULL - }; - if (nbd_connect_command (nbd, args) == -1) { + if (nbd_connect_command (nbd, + (char *[]) { + "nbdkit", "-s", "--exit-with-parent", + "split", "split1", "split2", + "file=split3" /* leave file= to test */, + NULL }) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); }

Reply

Richard W.M. Jones

Wednesday, 17 November Wed, 17 Nov

11:58 a.m.

New subject: [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

Reply

Eric Blake

2:13 p.m.

New subject: [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.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

Reply

Laszlo Ersek

Thursday, 18 November Thu, 18 Nov

7:12 a.m.

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

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

Reply

1100

days inactive

1101

days old

guestfs@lists.libguestfs.org

Manage subscription

5 comments

3 participants

tags (0)

participants (3)

Eric Blake
Laszlo Ersek
Richard W.M. Jones