Libguestfs - Aug 2020 - [PATCH nbdkit 1/2] server: Call .get_ready before redirecting stdin/stdout to /dev/null.

VDDK plugin + --run was broken because of the following sequence of
events:

(1) After .config_complete, server redirects stdin/stdout to /dev/null.

(2) Server then calls vddk_get_ready which reexecs.

(3) We restart the server from the top, but with stdin/stdout
    redirected to /dev/null.  So saved_stdin/saved_stdout save
    /dev/null.

(4) run_command is called which "restores" /dev/null as stdin/stdout.

(5) The output of the --run option is sent to /dev/null.

In addition to fixing this problem with VDDK, it also makes general
sense not to redirect stdin/stdout before calling .get_ready since
this callback is supposed to be the last chance before the server
daemonizes, and redirecting stdin/stdout is part of daemonization.

Before this commit:

  $ ./nbdkit vddk libdir=tests/.libs /dev/null --run 'qemu-img info
$nbd'
  [no output]

After this commit:

  $ ./nbdkit vddk libdir=tests/.libs /dev/null --run 'qemu-img info
$nbd'
  image: nbd://localhost:10809
  file format: raw
  virtual size: 512 KiB (524288 bytes)
  disk size: unavailable

One test (test-stdio.sh) made the assumption that stdin/stdout had
already been redirected to /dev/null in .get_ready.  I adjusted this
test so it checks for redirection in .after_fork instead.
---
 server/main.c             | 10 +++++-----
 tests/test-stdio-plugin.c |  9 +++++++++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/server/main.c b/server/main.c
index 2f0aaf07..11c1614b 100644
--- a/server/main.c
+++ b/server/main.c
@@ -693,6 +693,11 @@ main (int argc, char *argv[])
   /* Select the correct thread model based on config. */
   lock_init_thread_model ();
 
+  /* Tell the plugin that we are about to start serving.  This must be
+   * called before we change user, fork, or open any sockets.
+   */
+  top->get_ready (top);
+
   /* Sanitize stdin/stdout to /dev/null, after saving the originals
    * when needed.  We are still single-threaded at this point, and
    * already checked that stdin/out were open, so we don't have to
@@ -726,12 +731,7 @@ main (int argc, char *argv[])
     perror ("open");
     exit (EXIT_FAILURE);
   }
-
-  /* Tell the plugin that we are about to start serving.  This must be
-   * called before we change user, fork, or open any sockets.
-   */
   configured = true;
-  top->get_ready (top);
 
   start_serving ();
 
diff --git a/tests/test-stdio-plugin.c b/tests/test-stdio-plugin.c
index 618eae83..86447278 100644
--- a/tests/test-stdio-plugin.c
+++ b/tests/test-stdio-plugin.c
@@ -122,6 +122,14 @@ stdio_config_complete (void)
 
 static int
 stdio_get_ready (void)
+{
+  bool check = stdio_check ();
+  assert (check == false);
+  return 0;
+}
+
+static int
+stdio_after_fork (void)
 {
   bool check = stdio_check ();
   assert (check == true);
@@ -163,6 +171,7 @@ static struct nbdkit_plugin plugin = {
   .config            = stdio_config,
   .config_complete   = stdio_config_complete,
   .get_ready         = stdio_get_ready,
+  .after_fork        = stdio_after_fork,
   .open              = stdio_open,
   .get_size          = stdio_get_size,
   .pread             = stdio_pread,
-- 
2.27.0

---
 tests/Makefile.am      |  8 +++---
 tests/test-vddk-run.sh | 56 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 676e7e68..79be5639 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -913,12 +913,13 @@ if HAVE_VDDK
 noinst_LTLIBRARIES += libvixDiskLib.la
 LIBGUESTFS_TESTS += test-vddk
 TESTS += \
-	test-vddk-reexec.sh \
 	test-vddk-dump-plugin.sh \
 	test-vddk-password-fd.sh \
 	test-vddk-password-interactive.sh \
-	test-vddk-real.sh \
 	test-vddk-real-dump-plugin.sh \
+	test-vddk-real.sh \
+	test-vddk-reexec.sh \
+	test-vddk-run.sh \
 	$(NULL)
 
 test_vddk_SOURCES = test-vddk.c test.h
@@ -944,9 +945,10 @@ EXTRA_DIST += \
 	test-vddk-dump-plugin.sh \
 	test-vddk-password-fd.sh \
 	test-vddk-password-interactive.sh \
-	test-vddk-real.sh \
 	test-vddk-real-dump-plugin.sh \
+	test-vddk-real.sh \
 	test-vddk-reexec.sh \
+	test-vddk-run.sh \
 	$(NULL)
 
 # zero plugin test.
diff --git a/tests/test-vddk-run.sh b/tests/test-vddk-run.sh
new file mode 100755
index 00000000..19055245
--- /dev/null
+++ b/tests/test-vddk-run.sh
@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2020 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.
+
+# Test VDDK + --run parameter.
+
+source ./functions.sh
+set -e
+set -x
+
+# Testing $LD_LIBRARY_PATH stuff breaks valgrind, so skip the rest of
+# this test if valgrinding.
+if [ "x$NBDKIT_VALGRIND" = "x1" ]; then
+    echo "$0: skipped LD_LIBRARY_PATH test when doing valgrind"
+    exit 77
+fi
+
+requires qemu-img --version
+
+out=test-vddk-run.out
+rm -f $out
+cleanup_fn rm -f $out
+
+nbdkit -U - vddk libdir=.libs /dev/null --run 'qemu-img info $nbd' >
$out
+cat $out
+
+grep 'file format: raw' $out
+grep 'virtual size:.*524288 bytes' $out
-- 
2.27.0

On 8/5/20 5:29 AM, Richard W.M. Jones wrote:
> VDDK plugin + --run was broken because of the following sequence of
> events:
> 
> (1) After .config_complete, server redirects stdin/stdout to /dev/null.
> 
> (2) Server then calls vddk_get_ready which reexecs.
> 
> (3) We restart the server from the top, but with stdin/stdout
>      redirected to /dev/null.  So saved_stdin/saved_stdout save
>      /dev/null.
> 
> (4) run_command is called which "restores" /dev/null as
stdin/stdout.
> 
> (5) The output of the --run option is sent to /dev/null.
> 
> In addition to fixing this problem with VDDK, it also makes general
> sense not to redirect stdin/stdout before calling .get_ready since
> this callback is supposed to be the last chance before the server
> daemonizes, and redirecting stdin/stdout is part of daemonization.
It's been an interesting game of sequencing, but as we add more tests, 
we're certainly going to be more robust against future breakage ;)

Series looks good.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org