There are advantages to having the same code parse the options in the ./nbdkit wrapper as in the real nbdkit: - We can parse options in exactly the same way as the real program. - Use the more accurate ‘is_short_name’ test for unadorned plugin/filter names on the command line. - Fixes the FreeBSD problem with shebangs caused because FreeBSD refuses to use a shell script as a shebang path. Apart from the above, this is a straightforward translation of the original shell script into C and preserves all the existing features such as valgrind and gdb support. Thanks: Eric Blake. --- Makefile.am | 12 ++- README | 2 +- configure.ac | 2 - nbdkit.in | 160 -------------------------------- wrapper.c | 252 +++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 264 insertions(+), 164 deletions(-) diff --git a/Makefile.am b/Makefile.am index d5ef59f..5abe18e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -46,7 +46,17 @@ EXTRA_DIST = \ CLEANFILES += html/*.html -noinst_SCRIPTS = nbdkit +# NB: This is not the real nbdkit binary. It's a wrapper that allows +# you to run nbdkit from the build directory before it is installed. +noinst_PROGRAMS = nbdkit +nbdkit_SOURCES = wrapper.c src/options.h +nbdkit_CPPFLAGS = \ + -I$(top_srcdir)/src \ + -Dbuilddir=\"$(abs_top_builddir)\" \ + -Dsrcdir=\"$(abs_top_srcdir)\" \ + -DVALGRIND=\"$(VALGRIND)\" +nbdkit_CFLAGS = $(WARNINGS_CFLAGS) +nbdkit_DEPENDENCIES = config.status SUBDIRS = \ bash \ diff --git a/README b/README index 0b266f2..57583a3 100644 --- a/README +++ b/README @@ -154,7 +154,7 @@ Building make check make check To run nbdkit from the source directory, use the top level ./nbdkit -script. It will run nbdkit and plugins from the locally compiled +wrapper. It will run nbdkit and plugins from the locally compiled directory: $ ./nbdkit example1 -f -v diff --git a/configure.ac b/configure.ac index ed07177..5c2e951 100644 --- a/configure.ac +++ b/configure.ac @@ -736,8 +736,6 @@ AC_SUBST([filters]) dnl Produce output files. AC_CONFIG_HEADERS([config.h]) -AC_CONFIG_FILES([nbdkit], - [chmod +x,-w nbdkit]) AC_CONFIG_FILES([podwrapper.pl], [chmod +x,-w podwrapper.pl]) AC_CONFIG_FILES([Makefile diff --git a/nbdkit.in b/nbdkit.in deleted file mode 100644 index 5efbf75..0000000 --- a/nbdkit.in +++ /dev/null @@ -1,160 +0,0 @@ -#!/usr/bin/env bash -# @configure_input@ - -# Copyright (C) 2017-2018 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. - -#------------------------------------------------------------ -# This script lets you run nbdkit from the source directory. -# -# You can use either: -# ./nbdkit file [arg=value] [arg=value] ... -# or: -# /path/to/nbdkit file [arg=value] [arg=value] ... -# -# Or you can set $PATH to include the nbdkit source directory and run -# the bare "nbdkit" command without supplying the full path. -# -# The script modifies the bare plugin name (eg. "file") to be the full -# path to the locally compiled plugin. If you don't use this script -# and run src/nbdkit directly then it will pick up the installed -# plugins which is not usually what you want. -# -# This script is also used to run the tests (make check). -#------------------------------------------------------------ - -# The location of the source and build directories. Absolute paths -# are used so this script can be called from any location. -s="$(cd @abs_srcdir@ && pwd)" -b="$(cd @abs_builddir@ && pwd)" - -# Rewrite the bare module name on the command line. -declare -a args -i=0 -done= -verbose= - -while [ $# -gt 0 ]; do - case "$1" in - # Options that we special-case. Unlike getopt_long with short option - # squashing, we only recognize -v in isolation. Oh well. - -v | --verbose) - verbose=1 - args[$i]="$1" - ((++i)) - shift - ;; - - # Filters can be rewritten if purely alphanumeric. - --filter=*) - tmp=${1#*=} - shift - set - --filter "$tmp" "$@" - ;& # fallthru - --filter) - args[$i]="--filter" - ((++i)) - [ $# -gt 1 ] || break - if [[ "$2" =~ ^[a-zA-Z0-9]+$ ]]; then - if [ -x "$b/filters/$2/.libs/nbdkit-$2-filter.so" ]; then - args[$i]="$b/filters/$2/.libs/nbdkit-$2-filter.so" - else - args[$i]="$2" - fi - else - args[$i]="$2" - fi - ((++i)) - shift 2 - ;; - - # Remaining options that take an argument, which we pass through as is. - # Although getopt_long handles abbreviations, we don't. Oh well. - --*=*) - args[$i]="$1" - ((++i)) - shift - ;; - -D | --debug | -e | --export* | -g | --group | -i | --ip* | --log | \ - -P | --pid* | -p | --port | --run | --selinux-label | -t | --threads | \ - --tls | --tls-certificates | --tls-psk | -U | --unix | -u | --user) - args[$i]="$1" - ((++i)) - [ $# -gt 1 ] || break - args[$i]="$2" - ((++i)) - shift 2 - ;; - - # Anything else can be rewritten if it's purely alphanumeric, - # but there is only one module name so only rewrite once. - *) - if [ ! $done ] && [[ "$1" =~ ^[a-zA-Z0-9]+$ ]]; then - # Usual plugins written in C. - if [ -x "$b/plugins/$1/.libs/nbdkit-$1-plugin.so" ]; then - args[$i]="$b/plugins/$1/.libs/nbdkit-$1-plugin.so" - done=1 - # Special plugins written in Perl. - elif [ "$1" = "example4" ] || [ "$1" = "tar" ]; then - args[$i]="$b/plugins/perl/.libs/nbdkit-perl-plugin.so" - ((++i)) - args[$i]="$b/plugins/$1/nbdkit-$1-plugin" - done=1 - else - args[$i]="$1" - fi - else - args[$i]="$1" - fi - ((++i)) - shift - ;; - esac -done - -# If -v flag was given on the command line, print the command. -if [ $verbose ]; then - echo $b/src/nbdkit "${args[@]}" -fi - -prefix= -# If NBDKIT_VALGRIND=1 is set in the environment, then we run -# the program under valgrind. This is used by the tests. -# Similarly, if NBDKIT_GDB=1 is set in the environment, we run -# the program under gdb, useful during development. -if [ "$NBDKIT_VALGRIND" ]; then - prefix="@VALGRIND@ --vgdb=no --leak-check=full --error-exitcode=119 --suppressions=$s/valgrind-suppressions --trace-children=no --child-silent-after-fork=yes --run-libc-freeres=no --num-callers=20" -elif [ "$NBDKIT_GDB" ]; then - prefix="gdb --args" -fi - -# Run the final command. -exec $prefix $b/src/nbdkit "${args[@]}" diff --git a/wrapper.c b/wrapper.c new file mode 100644 index 0000000..c253e9f --- /dev/null +++ b/wrapper.c @@ -0,0 +1,252 @@ +/* nbdkit + * Copyright (C) 2017-2018 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. + */ + +/*------------------------------------------------------------ + * This wrapper lets you run nbdkit from the source directory. + * + * You can use either: + * ./nbdkit file [arg=value] [arg=value] ... + * or: + * /path/to/nbdkit file [arg=value] [arg=value] ... + * + * Or you can set $PATH to include the nbdkit source directory and run + * the bare "nbdkit" command without supplying the full path. + * + * The wrapper modifies the bare plugin name (eg. "file") to be the + * full path to the locally compiled plugin. If you don't use this + * program and run src/nbdkit directly then it will pick up the + * installed plugins which is not usually what you want. + * + * This program is also used to run the tests (make check). + * + * You can enable valgrind by setting NBDKIT_VALGRIND=1 (this + * is mainly used by the internal tests). + * + * You can enable debugging by setting NBDKIT_GDB=1 + *------------------------------------------------------------ + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdbool.h> +#include <stdarg.h> +#include <string.h> +#include <unistd.h> +#include <getopt.h> +#include <limits.h> + +#include "options.h" + +/* Construct an array of parameters passed through to real nbdkit. */ +static const char **cmd; +static size_t len; + +static void +passthru (const char *s) +{ + cmd = realloc (cmd, (len+1) * sizeof (const char *)); + if (cmd == NULL) + abort (); + cmd[len] = s; + ++len; +} + +static void __attribute__((format (printf, 1, 2))) +passthru_format (const char *fs, ...) +{ + va_list args; + char *str; + + va_start (args, fs); + if (vasprintf (&str, fs, args) == -1) + abort (); + va_end (args); + passthru (str); +} + +static void +end_passthru (void) +{ + passthru (NULL); +} + +static void +print_command (void) +{ + size_t i; + + if (len > 0) + fprintf (stderr, "%s", cmd[0]); + for (i = 1; i < len && cmd[i] != NULL; ++i) + fprintf (stderr, " %s", cmd[i]); + fprintf (stderr, "\n"); +} + +int +main (int argc, char *argv[]) +{ + bool verbose = false; + char *s; + + /* If NBDKIT_VALGRIND=1 is set in the environment, then we run the + * program under valgrind. This is used by the tests. Similarly if + * NBDKIT_GDB=1 is set, we run the program under GDB, useful during + * development. + */ + s = getenv ("NBDKIT_VALGRIND"); + if (s && strcmp (s, "1") == 0) { + passthru (VALGRIND); + passthru ("--vgdb=no"); + passthru ("--leak-check=full"); + passthru ("--error-exitcode=119"); + passthru_format ("--suppressions=%s/valgrind-suppressions", srcdir); + passthru ("--trace-children=no"); + passthru ("--child-silent-after-fork=yes"); + passthru ("--run-libc-freeres=no"); + passthru ("--num-callers=20"); + } + else { + s = getenv ("NBDKIT_GDB"); + if (s && strcmp (s, "1") == 0) { + passthru ("gdb"); + passthru ("--args"); + } + } + + /* Absolute path of the real nbdkit command. */ + passthru_format ("%s/src/nbdkit", builddir); + + /* Option parsing. We don't really parse options here. We are only + * interested in which options have arguments and which need + * rewriting. + */ + for (;;) { + int c; + int long_index = -1; + bool is_long_option, has_arg; + + c = getopt_long (argc, argv, short_options, long_options, &long_index); + if (c == -1) + break; + + if (c == '?') /* getopt prints an error */ + exit (EXIT_FAILURE); + + /* long_index is only set if it's an actual long option. */ + is_long_option = long_index >= 0; + + /* If optarg != NULL then the option has an argument. */ + has_arg = optarg != NULL; + + /* Verbose is special because we will print the final command. */ + if (c == 'v') { + verbose = true; + if (is_long_option) + passthru ("--verbose"); + else + passthru ("-v"); + } + /* Filters can be rewritten if they are a short name. */ + else if (c == FILTER_OPTION) { + if (is_short_name (optarg)) + passthru_format ("--filter=%s/filters/%s/.libs/nbdkit-%s-filter.so", + builddir, optarg, optarg); + else + passthru_format ("--filter=%s", optarg); + } + /* Any long option. */ + else if (is_long_option) { + /* Long option which takes an argument. */ + if (has_arg) + passthru_format ("--%s=%s", long_options[long_index].name, optarg); + /* Long option which takes no argument. */ + else + passthru_format ("--%s", long_options[long_index].name); + } + /* Any short option. */ + else { + /* Short option which takes an argument. */ + if (has_arg) { + passthru_format ("-%c", c); + passthru (optarg); + } + /* Short option which takes no argument. */ + else + passthru_format ("-%c", c); + } + } + + /* Are there any non-option arguments? */ + if (optind < argc) { + /* Ensure any further parameters can never be parsed as options by + * real nbdkit. + */ + passthru ("--"); + + /* The first non-option argument is the plugin name. If it is + * a short then rewrite it. + */ + if (is_short_name (argv[optind])) { + /* Special plugins written in Perl. */ + if (strcmp (argv[optind], "example4") == 0 || + strcmp (argv[optind], "tar") == 0) { + passthru_format ("%s/plugins/perl/.libs/nbdkit-perl-plugin.so", + builddir); + passthru_format ("%s/plugins/%s/nbdkit-%s-plugin", + builddir, argv[optind], argv[optind]); + } + else { + passthru_format ("%s/plugins/%s/.libs/nbdkit-%s-plugin.so", + builddir, argv[optind], argv[optind]); + } + ++optind; + } + + /* Everything else is passed through without rewriting. */ + while (optind < argc) { + passthru (argv[optind]); + ++optind; + } + } + + end_passthru (); + if (verbose) + print_command (); + + /* Run the final command. */ + execvp (cmd[0], (char **) cmd); + perror (cmd[0]); + exit (EXIT_FAILURE); +} -- 2.19.0.rc0

Fix the tests so they only enable valgrind workarounds or skips when NBDKIT_VALGRIND is exactly set to "1" and not any other values (including empty string, or not set). Thanks: Eric Blake. --- tests/test-dump-plugin.sh | 3 ++- tests/test-ext2.c | 4 +++- tests/test-help.sh | 3 ++- tests/test-lang-plugins.c | 4 +++- tests/test-ocaml.c | 4 +++- tests/test-python-exception.sh | 2 +- tests/test-version.sh | 3 ++- 7 files changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/test-dump-plugin.sh b/tests/test-dump-plugin.sh index dc52742..6e0e3d7 100755 --- a/tests/test-dump-plugin.sh +++ b/tests/test-dump-plugin.sh @@ -55,7 +55,8 @@ fi # However some of these tests are expected to fail. test () { - case "$1${NBDKIT_VALGRIND:+-valgrind}" in + [ "$NBDKIT_VALGRIND" = "1" ] && vg="-valgrind" + case "$1$vg" in vddk | vddk-valgrind) # VDDK won't run without special environment variables # being set, so ignore it. diff --git a/tests/test-ext2.c b/tests/test-ext2.c index 6878028..59e9281 100644 --- a/tests/test-ext2.c +++ b/tests/test-ext2.c @@ -49,12 +49,14 @@ main (int argc, char *argv[]) { guestfs_h *g; int r; + const char *s; char *data; /* The ext2 test fails valgrind. It seems as if the ext2fs error * table cannot be freed. */ - if (getenv ("NBDKIT_VALGRIND") != NULL) { + s = getenv ("NBDKIT_VALGRIND"); + if (s && strcmp (s, "1") == 0) { fprintf (stderr, "ext2 test skipped under valgrind.\n"); exit (77); /* Tells automake to skip the test. */ } diff --git a/tests/test-help.sh b/tests/test-help.sh index 68e0bfc..8dc0675 100755 --- a/tests/test-help.sh +++ b/tests/test-help.sh @@ -45,7 +45,8 @@ fi # However some of these tests are expected to fail. test () { - case "$1${NBDKIT_VALGRIND:+-valgrind}" in + [ "$NBDKIT_VALGRIND" = "1" ] && vg="-valgrind" + case "$1$vg" in vddk | vddk-valgrind) # VDDK won't run without special environment variables # being set, so ignore it. diff --git a/tests/test-lang-plugins.c b/tests/test-lang-plugins.c index 46b0563..f569540 100644 --- a/tests/test-lang-plugins.c +++ b/tests/test-lang-plugins.c @@ -49,6 +49,7 @@ main (int argc, char *argv[]) { guestfs_h *g; int r; + const char *s; char *data; /* These languages currently fail completely when run under @@ -56,7 +57,8 @@ main (int argc, char *argv[]) */ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-result" - if (getenv ("NBDKIT_VALGRIND") != NULL && + s = getenv ("NBDKIT_VALGRIND"); + if (s && strcmp (s, "1") == 0 && (strcmp (LANG, "python") == 0 || strcmp (LANG, "ruby") == 0 || strcmp (LANG, "tcl") == 0)) { diff --git a/tests/test-ocaml.c b/tests/test-ocaml.c index 4cae8a6..fa61966 100644 --- a/tests/test-ocaml.c +++ b/tests/test-ocaml.c @@ -49,13 +49,15 @@ main (int argc, char *argv[]) { guestfs_h *g; int r; + const char *s; char *data; size_t i, size; /* The OCaml tests fail valgrind, so skip them. We should be able * to get this working with a bit of effort. XXX */ - if (getenv ("NBDKIT_VALGRIND") != NULL) { + s = getenv ("NBDKIT_VALGRIND"); + if (s && strcmp (s, "1") == 0) { fprintf (stderr, "ocaml test skipped under valgrind.\n"); exit (77); /* Tells automake to skip the test. */ } diff --git a/tests/test-python-exception.sh b/tests/test-python-exception.sh index 567853a..7813918 100755 --- a/tests/test-python-exception.sh +++ b/tests/test-python-exception.sh @@ -36,7 +36,7 @@ set -x # Python language leaks like a sieve as well as a lot of worrying # "Conditional jump or move depends on uninitialised value(s)". -if test -n "$NBDKIT_VALGRIND"; then +if [ "$NBDKIT_VALGRIND" = "1" ]; then echo "$0: skipping Python test under valgrind." exit 77 fi diff --git a/tests/test-version.sh b/tests/test-version.sh index 869d88c..a3b8a18 100755 --- a/tests/test-version.sh +++ b/tests/test-version.sh @@ -45,7 +45,8 @@ fi # However some of these tests are expected to fail. test () { - case "$1${NBDKIT_VALGRIND:+-valgrind}" in + [ "$NBDKIT_VALGRIND" = "1" ] && vg="-valgrind" + case "$1$vg" in vddk | vddk-valgrind) # VDDK won't run without special environment variables # being set, so ignore it. -- 2.19.0.rc0

On Wed, Nov 14, 2018 at 08:43:32AM -0600, Eric Blake wrote: TBH I'm not even sure why we had it before. Anyway I have removed it and it doesn't look like there is any problem. Thanks for the review. I simplified the wrapper a little bit more by removing ‘bool has_arg’ completely - which made the code shorter - and pushed it. 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/