[PATCH libnbd] api: New nbd_kill_command API for sending a signal to the command subprocess.
Reverts commit 387cbe67c3db27e8a61117fedb6e7fad76e409ef. --- generator/generator | 18 +++++++++++++++++- lib/handle.c | 28 +++++++++++++++++++++++++++- tests/closure-lifetimes.c | 4 +++- 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/generator/generator b/generator/generator index 2cd83f1..25e4aa5 100755 --- a/generator/generator +++ b/generator/generator @@ -1191,7 +1191,9 @@ has been made."; longdesc = "\ Run the command as a subprocess and connect to it over stdin/stdout. This is for use with NBD servers which can -behave like inetd clients, such as C<nbdkit --single>."; +behave like inetd clients, such as C<nbdkit --single>. + +See also C<nbd_kill_command>."; }; "read_only", { @@ -2244,6 +2246,20 @@ The release number is incremented for each release along a particular branch."; }; + "kill_command", { + default_call with + args = [ Int "signal" ]; ret = RErr; + shortdesc = "kill server running as a subprocess"; + longdesc = "\ +This call may be used to kill the server running as a subprocess +that was previously created using C<nbd_connect_command>. You +do not need to use this call. It is only needed if the server +does not exit when the socket is closed. + +The C<signal> flag is the optional signal number to send +(see L<signal(7)>). If signal is C<0> then C<SIGTERM> is sent."; + }; + "supports_tls", { default_call with args = []; ret = RBool; is_locked = false; may_set_error = false; diff --git a/lib/handle.c b/lib/handle.c index 1fe4467..ccfea42 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -22,6 +22,8 @@ #include <stdlib.h> #include <unistd.h> #include <errno.h> +#include <signal.h> +#include <assert.h> #include <netdb.h> #include <sys/types.h> #include <sys/wait.h> @@ -124,7 +126,7 @@ nbd_close (struct nbd_handle *h) freeaddrinfo (h->result); if (h->sock) h->sock->ops->close (h->sock); - if (h->pid >= 0) /* XXX kill it? */ + if (h->pid > 0) waitpid (h->pid, NULL, 0); free (h->export_name); @@ -215,6 +217,30 @@ nbd_unlocked_get_version (struct nbd_handle *h) return PACKAGE_VERSION; } +int +nbd_unlocked_kill_command (struct nbd_handle *h, int signal) +{ + if (h->pid == -1) { + set_error (ESRCH, "no subprocess exists"); + return -1; + } + assert (h->pid > 0); + + if (signal == 0) + signal = SIGTERM; + if (signal < 0) { + set_error (EINVAL, "invalid signal number: %d", signal); + return -1; + } + + if (kill (h->pid, signal) == -1) { + set_error (errno, "kill"); + return -1; + } + + return 0; +} + /* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_supports_tls (struct nbd_handle *h) diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c index 910bdf4..3ddf4c1 100644 --- a/tests/closure-lifetimes.c +++ b/tests/closure-lifetimes.c @@ -35,7 +35,7 @@ static char *nbdkit_delay[] = { "nbdkit", "-s", "--exit-with-parent", "-v", "--filter=delay", "null", "size=512", - "delay-read=3", + "delay-read=10", NULL }; static unsigned debug_fn_valid; @@ -134,6 +134,7 @@ main (int argc, char *argv[]) assert (read_cb_free == 1); assert (completion_cb_free == 1); + nbd_kill_command (nbd, 0); nbd_close (nbd); /* Test command callbacks are freed if the handle is closed without @@ -149,6 +150,7 @@ main (int argc, char *argv[]) read_cb, NULL, completion_cb, NULL, 0); if (cookie == -1) NBD_ERROR; + nbd_kill_command (nbd, 0); nbd_close (nbd); assert (read_cb_free == 1); -- 2.22.0
On Thu, Jul 25, 2019 at 08:15:00PM +0100, Richard W.M. Jones wrote:
+The C<signal> flag is the optional signal number to send
Sorry, that should say "parameter" instead of "flag". Fixed in my local copy. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 7/25/19 2:15 PM, Richard W.M. Jones wrote:
Reverts commit 387cbe67c3db27e8a61117fedb6e7fad76e409ef. --- generator/generator | 18 +++++++++++++++++- lib/handle.c | 28 +++++++++++++++++++++++++++- tests/closure-lifetimes.c | 4 +++- 3 files changed, 47 insertions(+), 3 deletions(-)
tests/server-death.c can be simplified (no longer has to use --pidfile to learn where to send its kill); could be done on top.
+ "kill_command", { + default_call with + args = [ Int "signal" ]; ret = RErr; + shortdesc = "kill server running as a subprocess"; + longdesc = "\ +This call may be used to kill the server running as a subprocess +that was previously created using C<nbd_connect_command>. You +do not need to use this call. It is only needed if the server +does not exit when the socket is closed. + +The C<signal> flag is the optional signal number to send +(see L<signal(7)>). If signal is C<0> then C<SIGTERM> is sent.";
[1]
+int +nbd_unlocked_kill_command (struct nbd_handle *h, int signal) +{
Is gcc -Wshadow going to annoy us by this choice of naming? I'd use signum, if we're worried.
@@ -149,6 +150,7 @@ main (int argc, char *argv[]) read_cb, NULL, completion_cb, NULL, 0); if (cookie == -1) NBD_ERROR; + nbd_kill_command (nbd, 0);
This looks a bit funny until I read the docs at [1]. When using kill(2), I'm used to the function call 'kill(pid, 0)' probing for process existence. But on the command line, kill(1) has the behavior of sending SIGTERM by default when you omit a signal number (and not serving as a process existence probe). I guess our decision on what to do with the client requesting signal=0 boils down to whether we think nbd_is_dead() (for checking that the socket has gone away, but not necessarily if the subprocess still exists) and the knowledge that nbd_close() does wait() on the subprocess is sufficient for avoiding the need to do any direct probing. Thus, ACK with your wording fix. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Jul 25, 2019 at 03:19:23PM -0500, Eric Blake wrote:
This looks a bit funny until I read the docs at [1]. When using kill(2), I'm used to the function call 'kill(pid, 0)' probing for process existence. But on the command line, kill(1) has the behavior of sending SIGTERM by default when you omit a signal number (and not serving as a process existence probe).
The reason for this choice is that in some language bindings we might not have the SIG* symbols available, but there ought to still be a way to send a default. Hopefully people will read the docs :-) I pushed it with s/signal/signum/. Are we waiting on any other ABI breaks or would now be a good time to do another 0.x release? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On 7/25/19 4:11 PM, Richard W.M. Jones wrote:
On Thu, Jul 25, 2019 at 03:19:23PM -0500, Eric Blake wrote:
This looks a bit funny until I read the docs at [1]. When using kill(2), I'm used to the function call 'kill(pid, 0)' probing for process existence. But on the command line, kill(1) has the behavior of sending SIGTERM by default when you omit a signal number (and not serving as a process existence probe).
The reason for this choice is that in some language bindings we might not have the SIG* symbols available, but there ought to still be a way to send a default. Hopefully people will read the docs :-)
Your explanation is good enough for me :)
I pushed it with s/signal/signum/.
Are we waiting on any other ABI breaks or would now be a good time to do another 0.x release?
I know we've debated about a flag to nbd_shutdown to give the user more control over whether to wait on in-flight commands to settle, but without code for that, I don't see it stalling another 0.x release. In the meantime, we've definitely broken API/ABI, so another release makes it easier to test against. Still, I'd appreciate a few minutes to post my possible optimization of VALID|FREE in a few more spots, though. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Jul 25, 2019 at 04:23:12PM -0500, Eric Blake wrote:
On 7/25/19 4:11 PM, Richard W.M. Jones wrote:
On Thu, Jul 25, 2019 at 03:19:23PM -0500, Eric Blake wrote:
This looks a bit funny until I read the docs at [1]. When using kill(2), I'm used to the function call 'kill(pid, 0)' probing for process existence. But on the command line, kill(1) has the behavior of sending SIGTERM by default when you omit a signal number (and not serving as a process existence probe).
The reason for this choice is that in some language bindings we might not have the SIG* symbols available, but there ought to still be a way to send a default. Hopefully people will read the docs :-)
Your explanation is good enough for me :)
I pushed it with s/signal/signum/.
Are we waiting on any other ABI breaks or would now be a good time to do another 0.x release?
I know we've debated about a flag to nbd_shutdown to give the user more control over whether to wait on in-flight commands to settle, but without code for that, I don't see it stalling another 0.x release. In the meantime, we've definitely broken API/ABI, so another release makes it easier to test against. Still, I'd appreciate a few minutes to post my possible optimization of VALID|FREE in a few more spots, though.
Sure that's fine, I'll wait til tomorrow. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 7/25/19 3:19 PM, Eric Blake wrote:
On 7/25/19 2:15 PM, Richard W.M. Jones wrote:
Reverts commit 387cbe67c3db27e8a61117fedb6e7fad76e409ef. --- generator/generator | 18 +++++++++++++++++- lib/handle.c | 28 +++++++++++++++++++++++++++- tests/closure-lifetimes.c | 4 +++- 3 files changed, 47 insertions(+), 3 deletions(-)
tests/server-death.c can be simplified (no longer has to use --pidfile to learn where to send its kill); could be done on top.
Done now: From a9c42e136ef52361a13a7a2f2e7eae6cadb36421 Mon Sep 17 00:00:00 2001 From: Eric Blake <eblake@redhat.com> Date: Thu, 25 Jul 2019 19:58:32 -0500 Subject: [libnbd PATCH] tests: Simplify server-death with nbd_kill_command Now that libnbd can forward our kill request, we don't need to manage a pidfile ourselves. And we don't even have to ignore SIGCHLD to avoid waiting for a zombie; all we really need is our existing nbd_aio_poll loop to detect that the connection dies. --- tests/server-death.c | 100 ++++++++++++++----------------------------- 1 file changed, 31 insertions(+), 69 deletions(-) diff --git a/tests/server-death.c b/tests/server-death.c index cd58190..56e7e51 100644 --- a/tests/server-death.c +++ b/tests/server-death.c @@ -25,7 +25,6 @@ #include <stdlib.h> #include <string.h> #include <errno.h> -#include <unistd.h> #include <signal.h> #include <libnbd.h> @@ -56,69 +55,46 @@ main (int argc, char *argv[]) const char *msg; char buf[512]; int64_t cookie; - char pidfile[] = "/tmp/libnbd-test-disconnectXXXXXX"; - int fd; - pid_t pid; int r; - int delay = 0; - const char *cmd[] = { "nbdkit", "--pidfile", pidfile, "-s", - "--exit-with-parent", "--filter=delay", "memory", - "size=1m", "delay-read=15", "delay-trim=15", NULL }; + const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent", + "--filter=delay", "memory", "size=1m", + "delay-read=15", "delay-trim=15", NULL }; progname = argv[0]; - /* We're going to kill the child, but don't want to wait for a zombie */ - if (signal (SIGCHLD, SIG_IGN) == SIG_ERR) { - fprintf (stderr, "%s: signal: %s\n", argv[0], strerror (errno)); - exit (EXIT_FAILURE); - } - - fd = mkstemp (pidfile); - if (fd < 0) { - fprintf (stderr, "%s: mkstemp: %s\n", argv[0], strerror (errno)); - exit (EXIT_FAILURE); - } - nbd = nbd_create (); if (nbd == NULL) { fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); - goto fail; + exit (EXIT_FAILURE); } /* Connect to a slow server. */ if (nbd_connect_command (nbd, (char **) cmd) == -1) { fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); - goto fail; - } - if (read (fd, buf, sizeof buf) == -1) { - fprintf (stderr, "%s: read: %s\n", argv[0], strerror (errno)); - goto fail; - } - pid = atoi (buf); - if (pid <= 0) { - fprintf (stderr, "%s: unable to parse server's pid\n", argv[0]); - goto fail; + exit (EXIT_FAILURE); } /* Issue a read and trim that should not complete yet. Set up the * trim to auto-retire via callback. */ if ((cookie = nbd_aio_pread (nbd, buf, sizeof buf, 0, 0)) == -1) { - fprintf (stderr, "%s: test failed: nbd_aio_pread\n", argv[0]); - goto fail; + fprintf (stderr, "%s: test failed: nbd_aio_pread: %s\n", argv[0], + nbd_get_error ()); + exit (EXIT_FAILURE); } if (nbd_aio_trim_callback (nbd, sizeof buf, 0, callback, NULL, 0) == -1) { - fprintf (stderr, "%s: test failed: nbd_aio_trim_callback\n", argv[0]); - goto fail; + fprintf (stderr, "%s: test failed: nbd_aio_trim_callback: %s\n", argv[0], + nbd_get_error ()); + exit (EXIT_FAILURE); } if (nbd_aio_peek_command_completed (nbd) != 0) { fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n", argv[0]); - goto fail; + exit (EXIT_FAILURE); } if (nbd_aio_command_completed (nbd, cookie) != 0) { fprintf (stderr, "%s: test failed: nbd_aio_command_completed\n", argv[0]); - goto fail; + exit (EXIT_FAILURE); } /* Kill the server forcefully (SIGINT is not always strong enough, @@ -126,47 +102,39 @@ main (int argc, char *argv[]) * actually exiting), although it's a race whether our signal * arrives while nbdkit has a pending transaction. */ - if (kill (pid, SIGKILL) == -1) { - fprintf (stderr, "%s: kill: %s\n", argv[0], strerror (errno)); - goto fail; - } - /* Wait up to 10 seconds, 100 ms at a time */ - while (kill (pid, 0) == 0) { - if (delay++ > 100) { - fprintf (stderr, "%s: kill taking too long\n", argv[0]); - goto fail; - } - usleep (100 * 1000); + if (nbd_kill_command (nbd, SIGKILL) == -1) { + fprintf (stderr, "%s: test failed: nbd_kill_command: %s\n", argv[0], + nbd_get_error ()); + exit (EXIT_FAILURE); } - /* This part is somewhat racy if we don't wait for the process death - * above - depending on load and timing, nbd_poll may succeed or - * fail, and we may transition to either CLOSED (the state machine - * saw a clean EOF) or DEAD (the state machine saw a stranded - * transaction or POLLERR). + /* This part is somewhat racy depending on how fast we run before + * the server process exits - depending on load and timing, nbd_poll + * may succeed or fail, and we may transition to either CLOSED (the + * state machine saw a clean EOF) or DEAD (the state machine saw a + * stranded transaction or POLLERR). */ while ((r = nbd_poll (nbd, 1000)) == 1) if (nbd_aio_is_dead (nbd) || nbd_aio_is_closed (nbd)) break; if (!(nbd_aio_is_dead (nbd) || nbd_aio_is_closed (nbd))) { fprintf (stderr, "%s: test failed: server death not detected\n", argv[0]); - goto fail; + exit (EXIT_FAILURE); } /* Detection of the dead server completes all remaining in-flight commands */ if (nbd_aio_in_flight (nbd) != 0) { - fprintf (stderr, "%s: test failed: nbd_aio_in_flight\n", - argv[0]); - goto fail; + fprintf (stderr, "%s: test failed: nbd_aio_in_flight\n", argv[0]); + exit (EXIT_FAILURE); } if (nbd_aio_peek_command_completed (nbd) != cookie) { fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n", argv[0]); - goto fail; + exit (EXIT_FAILURE); } if (nbd_aio_command_completed (nbd, cookie) != -1) { fprintf (stderr, "%s: test failed: nbd_aio_command_completed\n", argv[0]); - goto fail; + exit (EXIT_FAILURE); } msg = nbd_get_error (); err = nbd_get_errno (); @@ -175,19 +143,19 @@ main (int argc, char *argv[]) if (err != ENOTCONN) { fprintf (stderr, "%s: test failed: unexpected errno %d (%s)\n", argv[0], err, strerror (err)); - goto fail; + exit (EXIT_FAILURE); } /* With all commands retired, no further command should be pending */ if (!trim_retired) { fprintf (stderr, "%s: test failed: nbd_aio_trim_callback not retired\n", argv[0]); - goto fail; + exit (EXIT_FAILURE); } if (nbd_aio_peek_command_completed (nbd) != -1) { fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n", argv[0]); - goto fail; + exit (EXIT_FAILURE); } msg = nbd_get_error (); err = nbd_get_errno (); @@ -196,15 +164,9 @@ main (int argc, char *argv[]) if (err != EINVAL) { fprintf (stderr, "%s: test failed: unexpected errno %d (%s)\n", argv[0], err, strerror (err)); - goto fail; + exit (EXIT_FAILURE); } - close (fd); - unlink (pidfile); nbd_close (nbd); exit (EXIT_SUCCESS); - - fail: - unlink (pidfile); - exit (EXIT_FAILURE); } -- 2.20.1 -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones