Libguestfs - Jul 2019 - Re: [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 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