[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
participants (2)
-
Eric Blake -
Richard W.M. Jones