Libguestfs - Nov 2017 - [nbdkit PATCH 0/2] Add nbd forwarder test coverage

To avoid bitrot, any new feature needs testsuite coverage ;)

Still to come: once I get my work on parallel nbd finished,
I will add a test-parallel-nbd.sh that closely mirrors what
my other series added in test-parallel-file.sh.

If desired, it might be a fun exercise to tweak test-nbd into
using a for-loop of user-controlled depth for how deep you
want to nest the forwarding tree, to see where the bottlenecks
like in trying to improve nbdkit performance.

Eric Blake (2):
  tests: Prepare for running multiple nbdkits in one test
  tests: Test nbd forwarder plugin

 .gitignore        |   1 +
 tests/Makefile.am |   8 ++++
 tests/test-nbd.c  | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/test.c      | 118 +++++++++++++++++++++++++++++++++++------------------
 tests/test.h      |   4 +-
 5 files changed, 209 insertions(+), 41 deletions(-)
 create mode 100644 tests/test-nbd.c

-- 
2.13.6

The obvious way to test the nbd plugin is to wrap yet another
nbdkit server.  The testsuite already has nice cleanup of an
nbdkit server, which we want to reuse while multiple servers
are running.  So rework global variables to instead belong to
a struct which is allocated per invocation, with the cleanup
function destroying servers in LIFO order.

To minimize changes to existing tests, existing global variables
are changed to track the details of the most-recently created
server.  A new global sock variable will also prove useful.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test.c | 118 +++++++++++++++++++++++++++++++++++++++--------------------
 tests/test.h |   4 +-
 2 files changed, 81 insertions(+), 41 deletions(-)

diff --git a/tests/test.c b/tests/test.c
index 961b918..5dc2f64 100644
--- a/tests/test.c
+++ b/tests/test.c
@@ -42,70 +42,101 @@
 #include <sys/wait.h>
 #include <signal.h>
 #include <errno.h>
+#include <assert.h>

 #include "test.h"

 /* 'test_start_nbdkit' below makes assumptions about the format of
  * these strings.
  */
-static char tmpdir[] =            "/tmp/nbdkitXXXXXX";
-static char sockpath[] =          "/tmp/nbdkitXXXXXX/sock";
-static char unixsockpath[] = "unix:/tmp/nbdkitXXXXXX/sock";
-static char pidpath[] =           "/tmp/nbdkitXXXXXX/pid";
+#define TEST_NBDKIT_TEMPLATE "/tmp/nbdkitXXXXXX"
+struct test_nbdkit {
+  char tmpdir[          17     + 1]; /*          template,          NUL */
+  char sockpath[        17 + 5 + 1]; /*          template, "/sock",
NUL */
+  char unixsockpath[5 + 17 + 5 + 1]; /* "unix:", template,
"/sock", NUL */
+  char pidpath[         17 + 4 + 1]; /*          template, "/pid", 
NUL */
+  pid_t pid;
+  struct test_nbdkit *next;
+};
+const struct test_nbdkit template = {
+  .tmpdir =               TEST_NBDKIT_TEMPLATE,
+  .sockpath =             TEST_NBDKIT_TEMPLATE "/sock",
+  .unixsockpath = "unix:" TEST_NBDKIT_TEMPLATE "/sock",
+  .pidpath =              TEST_NBDKIT_TEMPLATE "/pid",
+};
+
+static struct test_nbdkit *head;

 pid_t pid = 0;
-const char *server[2] = { unixsockpath, NULL };
+const char *sock = NULL;
+const char *server[2] = { NULL, NULL };

 static void
 cleanup (void)
 {
   int status;
+  struct test_nbdkit *next;

-  if (pid > 0) {
-    kill (pid, SIGTERM);
+  while (head) {
+    if (head->pid > 0) {
+      assert (!pid || pid == head->pid);
+      pid = 0;
+      kill (head->pid, SIGTERM);

-    /* Check the status of nbdkit is normal on exit. */
-    if (waitpid (pid, &status, 0) == -1) {
-      perror ("waitpid");
-      _exit (EXIT_FAILURE);
-    }
-    if (WIFEXITED (status) && WEXITSTATUS (status) != 0) {
-      _exit (WEXITSTATUS (status));
-    }
-    if (WIFSIGNALED (status)) {
-      /* Note that nbdkit is supposed to catch the signal we send and
-       * exit cleanly, so the following shouldn't happen.
-       */
-      fprintf (stderr, "nbdkit terminated by signal %d\n", WTERMSIG
(status));
-      _exit (EXIT_FAILURE);
-    }
-    if (WIFSTOPPED (status)) {
-      fprintf (stderr, "nbdkit stopped by signal %d\n", WSTOPSIG
(status));
-      _exit (EXIT_FAILURE);
+      /* Check the status of nbdkit is normal on exit. */
+      if (waitpid (head->pid, &status, 0) == -1) {
+        perror ("waitpid");
+        _exit (EXIT_FAILURE);
+      }
+      if (WIFEXITED (status) && WEXITSTATUS (status) != 0) {
+        _exit (WEXITSTATUS (status));
+      }
+      if (WIFSIGNALED (status)) {
+        /* Note that nbdkit is supposed to catch the signal we send and
+         * exit cleanly, so the following shouldn't happen.
+         */
+        fprintf (stderr, "nbdkit terminated by signal %d\n", WTERMSIG
(status));
+        _exit (EXIT_FAILURE);
+      }
+      if (WIFSTOPPED (status)) {
+        fprintf (stderr, "nbdkit stopped by signal %d\n", WSTOPSIG
(status));
+        _exit (EXIT_FAILURE);
+      }
     }
+
+    unlink (head->pidpath);
+    unlink (head->sockpath);
+    rmdir (head->tmpdir);
+
+    next = head->next;
+    free (head);
+    head = next;
   }
-
-  unlink (pidpath);
-  unlink (sockpath);
-  rmdir (tmpdir);
 }

 int
 test_start_nbdkit (const char *arg, ...)
 {
   size_t i, len;
+  struct test_nbdkit *kit = malloc (sizeof *kit);

-  if (mkdtemp (tmpdir) == NULL) {
+  if (!kit) {
+    perror ("malloc");
+    return -1;
+  }
+  *kit = template;
+  if (mkdtemp (kit->tmpdir) == NULL) {
     perror ("mkdtemp");
+    free (kit);
     return -1;
   }
-  len = strlen (tmpdir);
-  memcpy (sockpath, tmpdir, len);
-  memcpy (unixsockpath+5, tmpdir, len);
-  memcpy (pidpath, tmpdir, len);
+  len = strlen (kit->tmpdir);
+  memcpy (kit->sockpath, kit->tmpdir, len);
+  memcpy (kit->unixsockpath+5, kit->tmpdir, len);
+  memcpy (kit->pidpath, kit->tmpdir, len);

-  pid = fork ();
-  if (pid == 0) {               /* Child (nbdkit). */
+  kit->pid = fork ();
+  if (kit->pid == 0) {               /* Child (nbdkit). */
     const char *p;
     const int MAX_ARGS = 64;
     const char *argv[MAX_ARGS+1];
@@ -113,9 +144,9 @@ test_start_nbdkit (const char *arg, ...)

     argv[0] = "nbdkit";
     argv[1] = "-U";
-    argv[2] = sockpath;
+    argv[2] = kit->sockpath;
     argv[3] = "-P";
-    argv[4] = pidpath;
+    argv[4] = kit->pidpath;
     argv[5] = "-f";
     argv[6] = "-v";
     argv[7] = arg;
@@ -139,7 +170,14 @@ test_start_nbdkit (const char *arg, ...)
   /* Ensure nbdkit is killed and temporary files are deleted when the
    * main program exits.
    */
-  atexit (cleanup);
+  if (head)
+    kit->next = head;
+  else
+    atexit (cleanup);
+  head = kit;
+  pid = kit->pid;
+  sock = kit->sockpath;
+  server[0] = kit->unixsockpath;

   /* Wait for the pidfile to turn up, which indicates that nbdkit has
    * started up successfully and is ready to serve requests.  However
@@ -162,7 +200,7 @@ test_start_nbdkit (const char *arg, ...)
       perror ("kill");
     }

-    if (access (pidpath, F_OK) == 0)
+    if (access (kit->pidpath, F_OK) == 0)
       break;

     sleep (1);
diff --git a/tests/test.h b/tests/test.h
index ba00bba..dae3afc 100644
--- a/tests/test.h
+++ b/tests/test.h
@@ -39,9 +39,11 @@

 #define NBDKIT_START_TIMEOUT 30 /* seconds */

-extern pid_t pid;               /* PID of nbdkit process. */
+extern pid_t pid;               /* PID of most recent nbdkit process */
+extern const char *sock;        /* socket of most recent nbdkit process */
 extern const char *server[2];   /* server parameter for add_drive */

+/* Can be called more than once (useful for nbd plugin) */
 extern int test_start_nbdkit (const char *arg, ...);

 /* Declare program_name. */
-- 
2.13.6

I'd be remiss if I didn't enhance the testsuite to exercise the
nbd plugin (after all, the best way to make sure something doesn't
regress is to cover it in the testsuite).  Borrows heavily from
test-file, with the main difference being that I wrap three nbdkit
processes instead of one.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 .gitignore        |   1 +
 tests/Makefile.am |   8 ++++
 tests/test-nbd.c  | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 128 insertions(+)
 create mode 100644 tests/test-nbd.c

diff --git a/.gitignore b/.gitignore
index 4baaaad..17b1688 100644
--- a/.gitignore
+++ b/.gitignore
@@ -49,6 +49,7 @@ Makefile.in
 /tests/test-file
 /tests/test-gzip
 /tests/test-newstyle
+/tests/test-nbd
 /tests/test-ocaml
 /tests/test-ocaml-plugin.so
 /tests/test-oldstyle
diff --git a/tests/Makefile.am b/tests/Makefile.am
index c5429cb..aefb0ca 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -210,6 +210,14 @@ test_file_SOURCES = test-file.c test.h
 test_file_CFLAGS = $(WARNINGS_CFLAGS) $(LIBGUESTFS_CFLAGS)
 test_file_LDADD = libtest.la $(LIBGUESTFS_LIBS)

+# nbd plugin test.
+check_PROGRAMS += test-nbd
+TESTS += test-nbd
+
+test_nbd_SOURCES = test-nbd.c test.h
+test_nbd_CFLAGS = $(WARNINGS_CFLAGS) $(LIBGUESTFS_CFLAGS)
+test_nbd_LDADD = libtest.la $(LIBGUESTFS_LIBS)
+
 # gzip plugin test.
 if HAVE_ZLIB
 if HAVE_GUESTFISH
diff --git a/tests/test-nbd.c b/tests/test-nbd.c
new file mode 100644
index 0000000..646c0c3
--- /dev/null
+++ b/tests/test-nbd.c
@@ -0,0 +1,119 @@
+/* nbdkit
+ * Copyright (C) 2017 Red Hat Inc.
+ * All rights reserved.
+ *
+ * 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 <stdint.h>
+#include <inttypes.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <guestfs.h>
+
+#include "test.h"
+
+int
+main (int argc, char *argv[])
+{
+  guestfs_h *g;
+  int r;
+  char *data;
+  char *sockarg = NULL;
+  size_t i, size;
+
+  /* If wrapping once is good, why not do it twice!  Shows that we can
+   * convert between either style of server options. */
+  if (test_start_nbdkit ("-o", "file",
"file=file-data", NULL) == -1)
+    exit (EXIT_FAILURE);
+
+  if (asprintf (&sockarg, "socket=%s", sock) < 0) {
+    perror ("asprintf");
+    exit (EXIT_FAILURE);
+  }
+  if (test_start_nbdkit ("-e", "wrap", "nbd",
sockarg, NULL) == -1)
+    exit (EXIT_FAILURE);
+  free (sockarg);
+
+  if (asprintf (&sockarg, "socket=%s", sock) < 0) {
+    perror ("asprintf");
+    exit (EXIT_FAILURE);
+  }
+  if (test_start_nbdkit ("-o", "nbd", sockarg,
"export=wrap", NULL) == -1)
+    exit (EXIT_FAILURE);
+  free (sockarg);
+
+  g = guestfs_create ();
+  if (g == NULL) {
+    perror ("guestfs_create");
+    exit (EXIT_FAILURE);
+  }
+
+  r = guestfs_add_drive_opts (g, "",
+                              GUESTFS_ADD_DRIVE_OPTS_FORMAT, "raw",
+                              GUESTFS_ADD_DRIVE_OPTS_PROTOCOL, "nbd",
+                              GUESTFS_ADD_DRIVE_OPTS_SERVER, server,
+                              -1);
+  if (r == -1)
+    exit (EXIT_FAILURE);
+
+  if (guestfs_launch (g) == -1)
+    exit (EXIT_FAILURE);
+
+  /* Check the data in the file is \x01-\x08 repeated 512 times. */
+  data = guestfs_pread_device (g, "/dev/sda", 8 * 512, 0, &size);
+  if (!data)
+    exit (EXIT_FAILURE);
+  if (size != 8 * 512) {
+    fprintf (stderr, "%s FAILED: unexpected size (actual: %zu, expected:
512)\n",
+             program_name, size);
+    exit (EXIT_FAILURE);
+  }
+
+  for (i = 0; i < 512 * 8; i += 8) {
+    if (data[i] != 1 || data[i+1] != 2 ||
+        data[i+2] != 3 || data[i+3] != 4 ||
+        data[i+4] != 5 || data[i+5] != 6 ||
+        data[i+6] != 7 || data[i+7] != 8) {
+      fprintf (stderr, "%s FAILED: unexpected data returned at offset
%zu\n",
+               program_name, i);
+      exit (EXIT_FAILURE);
+    }
+  }
+
+  free (data);
+
+  guestfs_close (g);
+  exit (EXIT_SUCCESS);
+}
-- 
2.13.6

ACK this series too.

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