Re: [Libguestfs] [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Fri, Nov 4, 2022 at 11:18 PM Eric Blake wrote:
[...]
> @@ -127,7 +129,10 @@ def __call__(self, parser, namespace, values,
> option_string=None):
> os.environ["LIBNBD_DEBUG"] = "1"
>
> # Create the handle.
> -if not args.n:
> +if args.n:
> +pass
> +else:
>
Why add useless branch?
> +global h
> h = nbd.NBD()
> h.set_handle_name("nbdsh")
>
> @@ -165,21 +170,35 @@ def line(x): lines.append(x)
> def blank(): line("")
> def example(ex, desc): line("%-34s # %s" % (ex, desc))
>
> +connect_hint = False
> +go_hint = False
> blank()
> line("Welcome to nbdsh, the shell for interacting with")
> line("Network Block Device (NBD) servers.")
> blank()
> -if not args.n:
> -line("The ‘nbd’ module has already been imported and there")
> -line("is an open NBD handle called ‘h’.")
> -blank()
> -else:
> +if args.n:
> line("The ‘nbd’ module has already been imported.")
> blank()
> example("h = nbd.NBD()", "Create a new handle.")
> -if False: # args.uri is None:
>
Good that this was removed, but it will be better to remove in the previous
patch.
Nir
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 2/3] nbdsh: Allow -u interleaved with -c
On Fri, Nov 4, 2022 at 11:18 PM Eric Blake wrote
[...]
Sorry but I did not read, but I noticed this:
> @@ -165,7 +177,7 @@ def example(ex, desc): line("%-34s # %s" % (ex, desc))
> line("The ‘nbd’ module has already been imported.")
> blank()
> example("h = nbd.NBD()", "Create a new handle.")
> -if args.uri is None:
> +if False: # args.uri is None:
>
If False will never run, so why not remove the entire branch?
Is this leftover from debugging?
Nir
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH v2 2/3] nbdsh: Allow -u interleaved with -c
Starting with a back-story: I wanted[1] to write:
$ nbdkit --tls=require --tls-psk=$dir/tests/keys.psk null --run \
'nbdsh -u "nbds://alice@localhost/?tls-psk-file='"$dir"'/tests/keys.psk" \
-c "h.get_size()"'
but it fails with the latest libnbd release, due to:
nbdsh: unable to connect to uri
'nbds://alice@localhost/?tls-psk-file=/home/eblake/libnbd/tests/keys.psk':
nbd_connect_uri: local file access (tls-psk-file) is not allowed, call
nbd_set_uri_allow_local_file to enable this: Operation not permitted
But without this patch, this obvious attempt at a fix doesn't work:
$ nbdkit --tls=require --tls-psk=$dir/tests/keys.psk null --run \
'nbdsh -c "h.set_uri_allow_local_file(True)" \
-u "nbds://alice@localhost/?tls-psk-file='"$dir"'/tests/keys.psk" \
-c "h.get_size()"'
because nbdsh was performing h.connect_uri() prior to running any of
the -c strings, where the attempt to inject -c to match the hint from
the original error message is processed too late, even though it
occurred earlier in the command line. The immediate workaround is
thus to spell out the URI with a manual -c:
$ nbdkit --tls=require --tls-psk=$dir/tests/keys.psk null --run \
'nbdsh -c "h.set_uri_allow_local_file(True)" \
-c
"h.connect_uri(\"nbds://alice@localhost/?tls-psk-file='"$dir"'/tests/keys.psk\")"
\
-c "h.get_size()"'
which gets awkward because of the multiple layers of quoting required
(--run, -c, and Python each strip a layer). But we can do better.
Instead of creating separate args.command and args.uri, we can use a
custom argparse Action subclass which will merge various command-line
options into a unified args.snippets. Then with a little bit of
lambda magic, dispatching the various snippets in order looks fairly
easy.
I did have to drop one use of args.uri in the help banner, but it will
be restored in improved form in the next patch.
test-error.sh and test-context.sh have to be updated to match our
saner processing order. nbdkit's testsuite still passes even when
using this update to nbdsh, so I don't think programs in the wild were
relying on insane command-line rearranging.
[1] Actually, I wanted to use -U- and write -u "$uri" instead of -u
"nbds+unix...", but that involves teaching nbdkit how to display a
more-useful URI when TLS is in use. Not this patch's problem.
---
python/nbdsh.py| 76 +++---
sh/test-context.sh | 26
sh/test-error.sh | 37 --
3 files changed, 77 insertions(+), 62 deletions(-)
diff --git a/python/nbdsh.py b/python/nbdsh.py
index 1552ac73..0919c9ec 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -34,15 +34,37 @@ def shell():
parser = argparse.ArgumentParser(prog='nbdsh', description=description,
epilog=epilog)
-parser.set_defaults(command=[])
+# Allow intermixing of various options for replay in command-line order:
+# each option registered with this Action subclass will append a tuple
+# to a single list of snippets
+class SnippetAction(argparse.Action):
+def __init__(self, option_strings, dest, nargs=None,
+ default=argparse.SUPPRESS, **kwargs):
+if nargs not in [0, None]:
+raise ValueError("nargs must be 0 or None")
+super().__init__(option_strings, dest, nargs=nargs,
+ default=default, **kwargs)
+
+def __call__(self, parser, namespace, values, option_string=None):
+dest = self.dest
+if dest != 'command':
+setattr(namespace, 'need_handle',
+getattr(namespace, 'need_handle') + 1)
+elif values == '-':
+dest = 'stdin'
+snippets = getattr(namespace, 'snippets')[:]
+snippets.append((dest, values))
+setattr(namespace, 'snippets', snippets)
+
+parser.set_defaults(need_handle=0, snippets=[])
short_options = []
long_options = []
-parser.add_argument('--base-allocation', action='store_true',
+parser.add_argument('--base-allocation', action=SnippetAction, nargs=0,
help='request the "base:allocation" meta context')
long_options.append("--base-allocation")
-parser.add_argument('-c', '--command', action='append',
+parser.add_argument('-c', '--command', action=SnippetAction,
help="run a Python statement "
"(may be used multiple times)")
short_options.append("-c")
@@ -52,15 +74,17 @@ def shell():
help="do not create the implicit handle 'h'")
short_options.append("-n")
-parser.add_argument('--opt-mode', action='store_true',
+parser.add_argument('--opt-mode', action=SnippetAction, nargs=0,
help='request opt mode during connection')
long_options.append("--opt-mode")
-parser.add_argument('-u', '--uri', help="connect
[Libguestfs] [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
Document all options in --help output. If -n is not in use, then
enhance the banner to print the current state of h, and further tailor
the advice given on useful next steps to take to mention opt_go when
using --opt-mode.
---
python/nbdsh.py | 37 -
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/python/nbdsh.py b/python/nbdsh.py
index 0919c9ec..872b94fc 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -86,11 +86,13 @@ def __call__(self, parser, namespace, values,
option_string=None):
parser.add_argument('--connect', dest='uri', action=SnippetAction,
help=argparse.SUPPRESS)
-parser.add_argument('-v', '--verbose', action='store_true')
+parser.add_argument('-v', '--verbose', action='store_true',
+help="enable verbose debugging")
short_options.append("-v")
long_options.append("--verbose")
-parser.add_argument('-V', '--version', action='store_true')
+parser.add_argument('-V', '--version', action='store_true',
+help="display version information")
short_options.append("-V")
long_options.append("--version")
@@ -127,7 +129,10 @@ def __call__(self, parser, namespace, values,
option_string=None):
os.environ["LIBNBD_DEBUG"] = "1"
# Create the handle.
-if not args.n:
+if args.n:
+pass
+else:
+global h
h = nbd.NBD()
h.set_handle_name("nbdsh")
@@ -165,21 +170,35 @@ def line(x): lines.append(x)
def blank(): line("")
def example(ex, desc): line("%-34s # %s" % (ex, desc))
+connect_hint = False
+go_hint = False
blank()
line("Welcome to nbdsh, the shell for interacting with")
line("Network Block Device (NBD) servers.")
blank()
-if not args.n:
-line("The ‘nbd’ module has already been imported and there")
-line("is an open NBD handle called ‘h’.")
-blank()
-else:
+if args.n:
line("The ‘nbd’ module has already been imported.")
blank()
example("h = nbd.NBD()", "Create a new handle.")
-if False: # args.uri is None:
+connect_hint = True
+else:
+global h
+state = h.connection_state()
+state = state[:state.find(':')]
+line("The ‘nbd’ module has already been imported and there")
+line("is an open NBD handle called ‘h’ in state '%s'." % state)
+blank()
+if h.aio_is_created():
+connect_hint = True
+if h.get_opt_mode():
+go_hint = True
+elif h.aio_is_negotiating():
+go_hint = True
+if connect_hint:
example('h.connect_tcp("remote", "10809")',
"Connect to a remote server.")
+if go_hint:
+example("h.opt_go()", "Finish option negotiation")
example("h.get_size()", "Get size of the remote disk.")
example("buf = h.pread(512, 0)", "Read the first sector.")
example("exit() or Ctrl-D", "Quit the shell")
--
2.38.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH v2 1/3] nbdsh: Refactor handling of -c
By telling argparse to always supply a list for args.command, it
becomes easier to sink all logic related to going interactive to after
all command line options have been processed. This in turn will make
future patches that preserve command line ordering easier to manage.
---
python/nbdsh.py | 47 +++
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/python/nbdsh.py b/python/nbdsh.py
index a98a9113..1552ac73 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -1,5 +1,5 @@
# NBD client library in userspace
-# Copyright (C) 2013-2021 Red Hat Inc.
+# Copyright (C) 2013-2022 Red Hat Inc.
#
# This library is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
@@ -33,6 +33,8 @@ def shell():
epilog = '''Please read the nbdsh(1) manual page for full usage.'''
parser = argparse.ArgumentParser(prog='nbdsh', description=description,
epilog=epilog)
+
+parser.set_defaults(command=[])
short_options = []
long_options = []
@@ -98,10 +100,6 @@ def shell():
print("\n".join(long_options))
exit(0)
-# Create the banner and prompt.
-banner = make_banner(args)
-sys.ps1 = "nbd> "
-
# If verbose, set LIBNBD_DEBUG=1
if args.verbose:
os.environ["LIBNBD_DEBUG"] = "1"
@@ -126,26 +124,27 @@ def shell():
(args.uri, ex.string), file=sys.stderr)
sys.exit(1)
-# If there are no -c or --command parameters, go interactive,
-# otherwise we run the commands and exit.
-if not args.command:
-code.interact(banner=banner, local=locals(), exitmsg='')
-else:
-# https://stackoverflow.com/a/11754346
-d = dict(locals(), **globals())
-try:
-for c in args.command:
-if c != '-':
-exec(c, d, d)
-else:
-exec(sys.stdin.read(), d, d)
-except nbd.Error as ex:
-if nbd.NBD().get_debug():
-traceback.print_exc()
+# Run all -c snippets
+# https://stackoverflow.com/a/11754346
+d = dict(locals(), **globals())
+try:
+for c in args.command:
+if c != '-':
+exec(c, d, d)
else:
-print("nbdsh: command line script failed: %s" % ex.string,
- file=sys.stderr)
-sys.exit(1)
+exec(sys.stdin.read(), d, d)
+except nbd.Error as ex:
+if nbd.NBD().get_debug():
+traceback.print_exc()
+else:
+print("nbdsh: command line script failed: %s" % ex.string,
+ file=sys.stderr)
+sys.exit(1)
+
+# If there are no explicit -c or --command parameters, go interactive.
+if len(args.command) == 0:
+sys.ps1 = "nbd> "
+code.interact(banner=make_banner(args), local=locals(), exitmsg='')
def make_banner(args):
--
2.38.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH v2 0/3] Improve nbdsh -u handling
v1 was here: https://listman.redhat.com/archives/libguestfs/2022-October/030216.html Since then, I've incorporated changes based on Rich's feedback: swap order of patches 2 and 3 less change in patch 1 (including no unsafe eval(%s) for --uri) in patch 2, include -c in list of snippets to store, and use dict of lambdas to map back to the desired action Eric Blake (3): nbdsh: Refactor handling of -c nbdsh: Allow -u interleaved with -c nbdsh: Improve --help and initial banner contents. python/nbdsh.py| 142 +++-- sh/test-context.sh | 26 - sh/test-error.sh | 37 +++- 3 files changed, 119 insertions(+), 86 deletions(-) -- 2.38.1 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd 2/2] ocaml: Simplify the sockaddr test
On Fri, Nov 04, 2022 at 12:45:32PM +, Richard W.M. Jones wrote:
> The sockaddr test added in commit 50500aade9 ("ocaml: Implement
> sockaddr type") was quite complicated because it had to run nbdkit as
> a subprocess, manage the socket, check for a PID file etc.
>
> The simpler way of doing this is to make it work like the Python test
> (python/test-aio-connect-unix.sh) where we run
> nbdkit -U - ... --run './test $unixsocket'
>
> The straightforward way would be to a separate shell script etc to
> ocaml/tests/Makefile.am:TESTS which is orthogonal to how the existing
> tests work. So instead make the test exec nbdkit when called first
> time (without the $unixsocket parameter) and then run the test when
> called the second time.
>
> Updates: commit 50500aade9f32899eddd2a7b89ae5c596674f95c
> ---
> ocaml/tests/test_580_aio_connect.ml | 69 +
> 1 file changed, 30 insertions(+), 39 deletions(-)
Series:
Acked-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [nbdkit PATCH] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}
On Wed, Nov 02, 2022 at 01:43:13PM +, Richard W.M. Jones wrote: > On Tue, Nov 01, 2022 at 02:56:09PM -0500, Eric Blake wrote: > ... > > +=item S<6> > > + > > +Triggers a call to the C function C with C > > +set to false, which requests a soft disconnect of the current client > > +(future client requests are rejected with C without calling > > +into the plugin, but current requests may complete). Since the client > > +will likely get the response to this command, nbdkit then treats empty > > +stderr as success for the current callback, and parses non-empty > > +stderr as if the script had exited with code S<1>. > > OK .. seems like complicated behaviour, but I can sort of see the > reasoning behind it. > > I do wonder if we just want to use separate codes for the "soft > disconnect + OK" and the "soft disconnect + error" cases. We have > reserved more so we won't run out :-) Given that both you and Laszlo requested it, I will respin along those lines. > > > +=item 7-15 > > + > > +These exit codes are reserved for future use. Note that versions of > > +nbdkit E 1.34 documented that codes S<8> through S<15> behaved > > This is actually a case where S<> around the nbdkit E 1.34 makes > sense. But it should be removed around S<8> etc and in the next line > below. I'll fix the pre-existing inconsistent use of S<> as part of the respin. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd 2/2] ocaml: Simplify the sockaddr test
The sockaddr test added in commit 50500aade9 ("ocaml: Implement
sockaddr type") was quite complicated because it had to run nbdkit as
a subprocess, manage the socket, check for a PID file etc.
The simpler way of doing this is to make it work like the Python test
(python/test-aio-connect-unix.sh) where we run
nbdkit -U - ... --run './test $unixsocket'
The straightforward way would be to a separate shell script etc to
ocaml/tests/Makefile.am:TESTS which is orthogonal to how the existing
tests work. So instead make the test exec nbdkit when called first
time (without the $unixsocket parameter) and then run the test when
called the second time.
Updates: commit 50500aade9f32899eddd2a7b89ae5c596674f95c
---
ocaml/tests/test_580_aio_connect.ml | 69 +
1 file changed, 30 insertions(+), 39 deletions(-)
diff --git a/ocaml/tests/test_580_aio_connect.ml
b/ocaml/tests/test_580_aio_connect.ml
index 43c6fd0f7a..5a94ac0c9b 100644
--- a/ocaml/tests/test_580_aio_connect.ml
+++ b/ocaml/tests/test_580_aio_connect.ml
@@ -17,51 +17,42 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*)
+(* This test is unusual because we want to run it under nbdkit
+ * rather than having the test run nbdkit as a subprocess.
+ *
+ * Therefore we detect if a $unixsocket parameter is passed
+ * in Sys.argv. If not then we exec nbdkit:
+ * nbdkit -U - ... --run '$argv0 \$unixsocket'
+ * If the $unixsocket parameter is present then we run the test.
+ *)
+
open Unix
open Printf
let () =
- let nbd = NBD.create () in
+ match Array.length Sys.argv with
+ | 1 ->(* exec nbdkit *)
+ let argv0 = Sys.argv.(0) in
+ let runcmd = sprintf "%s $unixsocket" (Filename.quote argv0) in
+ execvp "nbdkit" [| "nbdkit"; "-U"; "-"; "--exit-with-parent";
+"memory"; "size=512";
+"--run"; runcmd |]
- (* Unlike other tests, we're going to run nbdkit as a subprocess
- * by hand and have it listening on a randomly named socket
- * that we create.
- *)
- let sock = Filename.temp_file "580-" ".sock" in
- unlink sock;
- let pidfile = Filename.temp_file "580-" ".pid" in
- unlink pidfile;
- let cmd =
-sprintf "nbdkit -U %s -P %s --exit-with-parent memory size=512 &"
- (Filename.quote sock) (Filename.quote pidfile) in
- if Sys.command cmd <> 0 then
-failwith "nbdkit command failed";
- let rec loop i =
-if i > 60 then
- failwith "nbdkit subcommand did not start up";
-if not (Sys.file_exists pidfile) then (
- sleep 1;
- loop (i+1)
-)
- in
- loop 0;
+ | 2 ->(* run the test *)
+ let unixsocket = Sys.argv.(1) in
+ let nbd = NBD.create () in
- (* Connect to the subprocess using a Unix.sockaddr. *)
- let sa = ADDR_UNIX sock in
- NBD.aio_connect nbd sa;
- while NBD.aio_is_connecting nbd do
-ignore (NBD.poll nbd 1)
- done;
- assert (NBD.aio_is_ready nbd);
- NBD.close nbd;
+ (* Connect to the subprocess using a Unix.sockaddr. *)
+ let sa = ADDR_UNIX unixsocket in
+ NBD.aio_connect nbd sa;
+ while NBD.aio_is_connecting nbd do
+ ignore (NBD.poll nbd 1)
+ done;
+ assert (NBD.aio_is_ready nbd);
+ assert (NBD.get_size nbd = 512_L);
+ NBD.close nbd
- (* Kill the nbdkit subprocess. *)
- let chan = open_in pidfile in
- let pid = int_of_string (input_line chan) in
- kill pid Sys.sigterm;
-
- (* Clean up files. *)
- unlink sock;
- unlink pidfile
+ | _ ->
+ failwith "unexpected test parameters"
let () = Gc.compact ()
--
2.37.0.rc2
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd 0/2] ocaml: Simplify the sockaddr test
Not as bad as I thought it would be! Rich. ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd 1/2] python: Rename the aio-connect test
This is a test, but it had a confusing name, so rename it to test-*.sh.
Updates: commit b8d0dd8e8d15219161dad9d029ece0b49fcbb565
---
python/Makefile.am| 4 ++--
.../{python-aio-connect-unix.sh => test-aio-connect-unix.sh} | 0
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/python/Makefile.am b/python/Makefile.am
index f51d40e16a..f1e701d03c 100644
--- a/python/Makefile.am
+++ b/python/Makefile.am
@@ -36,7 +36,7 @@ EXTRA_DIST = \
$(generator_built) \
nbdsh.py \
pycodestyle.sh \
- python-aio-connect-unix.sh \
+ test-aio-connect-unix.sh \
$(srcdir)/t/*.py \
$(NULL)
@@ -85,8 +85,8 @@ TESTS_ENVIRONMENT = \
$(NULL)
LOG_COMPILER = $(top_builddir)/run
TESTS += \
- python-aio-connect-unix.sh \
run-python-tests \
+ test-aio-connect-unix.sh \
$(NULL)
endif HAVE_NBDKIT
diff --git a/python/python-aio-connect-unix.sh b/python/test-aio-connect-unix.sh
similarity index 100%
rename from python/python-aio-connect-unix.sh
rename to python/test-aio-connect-unix.sh
--
2.37.0.rc2
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd] ocaml: Implement sockaddr type
On Fri, Nov 04, 2022 at 08:22:59AM +0100, Laszlo Ersek wrote: > On 11/03/22 16:16, Richard W.M. Jones wrote: > > On Thu, Nov 03, 2022 at 04:03:42PM +0100, Laszlo Ersek wrote: > >> On 11/02/22 22:10, Richard W.M. Jones wrote: > >>> +open Unix > >>> +open Printf > >>> + > >>> +let () = > >>> + let nbd = NBD.create () in > >>> + > >>> + (* Unlike other tests, we're going to run nbdkit as a subprocess > >>> + * by hand and have it listening on a randomly named socket > >>> + * that we create. > >>> + *) > >>> + let sock = Filename.temp_file "580-" ".sock" in > >>> + unlink sock; > >>> + let pidfile = Filename.temp_file "580-" ".pid" in > >>> + unlink pidfile; > >>> + let cmd = > >>> +sprintf "nbdkit -U %s -P %s --exit-with-parent memory size=512 &" > >>> + (Filename.quote sock) (Filename.quote pidfile) in > >>> + if Sys.command cmd <> 0 then > >>> +failwith "nbdkit command failed"; > >>> + let rec loop i = > >>> +if i > 60 then > >>> + failwith "nbdkit subcommand did not start up"; > >>> +if not (Sys.file_exists pidfile) then ( > >>> + sleep 1; > >>> + loop (i+1) > >>> +) > >>> + in > >>> + loop 0; > >>> + > >>> + (* Connect to the subprocess using a Unix.sockaddr. *) > >>> + let sa = ADDR_UNIX sock in > >>> + NBD.aio_connect nbd sa; > >>> + while NBD.aio_is_connecting nbd do > >>> +ignore (NBD.poll nbd 1) > >>> + done; > >>> + assert (NBD.aio_is_ready nbd); > >>> + NBD.close nbd; > >>> + > >>> + (* Kill the nbdkit subprocess. *) > >>> + let chan = open_in pidfile in > >>> + let pid = int_of_string (input_line chan) in > >>> + kill pid Sys.sigint; > >> > >> I think it's more customary to send SIGTERM in such situations; SIGINT > >> is more like an interactive interrupt signal (usually sent by the > >> terminal driver when the INTR character is entered on the terminal). > >> POSIX calls SIGINT "Terminal interrupt signal", and SIGTERM "Termination > >> signal". But it's really tangential. > > > > I changed it to SIGTERM now (commit eb13699a75). > > > > The whole test is very unsatisfactory though. Compare it to the > > elegance of the equivalent Python test: > > > > https://gitlab.com/nbdkit/libnbd/-/blob/master/python/python-aio-connect-unix.sh > > Can you perhaps introduce the test not under ML_TESTS but TESTS (called > "test_580_aio_connect.sh")? Then use the "captive nbdkit" pattern > (--run) just like in the python example. > > Now, ocaml doesn't support "-c" (I think), so the "$PYTHON -c" idea > won't work identically, but if you also introduced > "test_580_aio_connect.ml", and began that with an "ocaml shebang", that > should work, shouldn't it? > > nbdkit [...] --run 'test_580_aio_connect.ml "$unixsocket"' > > (Not sure if it's worth the churn though.) I was thinking about a complicated scheme with re-execing the test under nbdkit, but the above might well work too. I'll see what I can do. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd] ocaml: Implement sockaddr type
On 11/03/22 16:16, Richard W.M. Jones wrote: > On Thu, Nov 03, 2022 at 04:03:42PM +0100, Laszlo Ersek wrote: >> On 11/02/22 22:10, Richard W.M. Jones wrote: >>> +open Unix >>> +open Printf >>> + >>> +let () = >>> + let nbd = NBD.create () in >>> + >>> + (* Unlike other tests, we're going to run nbdkit as a subprocess >>> + * by hand and have it listening on a randomly named socket >>> + * that we create. >>> + *) >>> + let sock = Filename.temp_file "580-" ".sock" in >>> + unlink sock; >>> + let pidfile = Filename.temp_file "580-" ".pid" in >>> + unlink pidfile; >>> + let cmd = >>> +sprintf "nbdkit -U %s -P %s --exit-with-parent memory size=512 &" >>> + (Filename.quote sock) (Filename.quote pidfile) in >>> + if Sys.command cmd <> 0 then >>> +failwith "nbdkit command failed"; >>> + let rec loop i = >>> +if i > 60 then >>> + failwith "nbdkit subcommand did not start up"; >>> +if not (Sys.file_exists pidfile) then ( >>> + sleep 1; >>> + loop (i+1) >>> +) >>> + in >>> + loop 0; >>> + >>> + (* Connect to the subprocess using a Unix.sockaddr. *) >>> + let sa = ADDR_UNIX sock in >>> + NBD.aio_connect nbd sa; >>> + while NBD.aio_is_connecting nbd do >>> +ignore (NBD.poll nbd 1) >>> + done; >>> + assert (NBD.aio_is_ready nbd); >>> + NBD.close nbd; >>> + >>> + (* Kill the nbdkit subprocess. *) >>> + let chan = open_in pidfile in >>> + let pid = int_of_string (input_line chan) in >>> + kill pid Sys.sigint; >> >> I think it's more customary to send SIGTERM in such situations; SIGINT >> is more like an interactive interrupt signal (usually sent by the >> terminal driver when the INTR character is entered on the terminal). >> POSIX calls SIGINT "Terminal interrupt signal", and SIGTERM "Termination >> signal". But it's really tangential. > > I changed it to SIGTERM now (commit eb13699a75). > > The whole test is very unsatisfactory though. Compare it to the > elegance of the equivalent Python test: > > https://gitlab.com/nbdkit/libnbd/-/blob/master/python/python-aio-connect-unix.sh Can you perhaps introduce the test not under ML_TESTS but TESTS (called "test_580_aio_connect.sh")? Then use the "captive nbdkit" pattern (--run) just like in the python example. Now, ocaml doesn't support "-c" (I think), so the "$PYTHON -c" idea won't work identically, but if you also introduced "test_580_aio_connect.ml", and began that with an "ocaml shebang", that should work, shouldn't it? nbdkit [...] --run 'test_580_aio_connect.ml "$unixsocket"' (Not sure if it's worth the churn though.) Laszlo ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
