Libguestfs - Nov 2018 - Re: [PATCH 2/2] build: Replace ./nbdkit with a C program.

This patch series solves the FreeBSD shebang problem in a completely
different way, and a few other things besides.  I propose that we
replace ./nbdkit with a C program.  The C program is a straightforward
translation of the shell script.

Some advantages of this approach are:

  - 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 the use a shell script as a shebang path.

  - No longer need to maintain that troublesome shell script.

I have verified that this does indeed fix the FreeBSD shebang problem
(but didn't test on Haiku).  It also passes 'make check' and
'make check-valgrind' on Linux.

Rich.

This refactoring is simply so the exact same options can be reused in
another program.
---
 src/Makefile.am |   1 +
 src/main.c      |  70 +------------------------------
 src/options.h   | 109 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 69 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index c094ed4..3490c0f 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -46,6 +46,7 @@ nbdkit_SOURCES = \
 	log-stderr.c \
 	log-syslog.c \
 	main.c \
+	options.h \
 	plugins.c \
 	protocol.h \
 	sockets.c \
diff --git a/src/main.c b/src/main.c
index 224955b..0c7dbce 100644
--- a/src/main.c
+++ b/src/main.c
@@ -55,11 +55,11 @@
 #include <dlfcn.h>
 
 #include "internal.h"
+#include "options.h"
 #include "exit-with-parent.h"
 
 #define FIRST_SOCKET_ACTIVATION_FD 3 /* defined by systemd ABI */
 
-static int is_short_name (const char *);
 static char *make_random_fifo (void);
 static struct backend *open_plugin_so (size_t i, const char *filename, int
short_name);
 static struct backend *open_filter_so (struct backend *next, size_t i, const
char *filename, int short_name);
@@ -114,67 +114,6 @@ struct backend *backend;
 static char *random_fifo_dir = NULL;
 static char *random_fifo = NULL;
 
-enum {
-  HELP_OPTION = CHAR_MAX + 1,
-  DUMP_CONFIG_OPTION,
-  DUMP_PLUGIN_OPTION,
-  EXIT_WITH_PARENT_OPTION,
-  FILTER_OPTION,
-  LOG_OPTION,
-  LONG_OPTIONS_OPTION,
-  RUN_OPTION,
-  SELINUX_LABEL_OPTION,
-  SHORT_OPTIONS_OPTION,
-  TLS_OPTION,
-  TLS_CERTIFICATES_OPTION,
-  TLS_PSK_OPTION,
-  TLS_VERIFY_PEER_OPTION,
-};
-
-static const char *short_options = "D:e:fg:i:nop:P:rst:u:U:vV";
-static const struct option long_options[] = {
-  { "debug",            required_argument, NULL, 'D' },
-  { "dump-config",      no_argument,       NULL, DUMP_CONFIG_OPTION
},
-  { "dump-plugin",      no_argument,       NULL, DUMP_PLUGIN_OPTION
},
-  { "exit-with-parent", no_argument,       NULL,
EXIT_WITH_PARENT_OPTION },
-  { "export",           required_argument, NULL, 'e' },
-  { "export-name",      required_argument, NULL, 'e' },
-  { "exportname",       required_argument, NULL, 'e' },
-  { "filter",           required_argument, NULL, FILTER_OPTION },
-  { "foreground",       no_argument,       NULL, 'f' },
-  { "no-fork",          no_argument,       NULL, 'f' },
-  { "group",            required_argument, NULL, 'g' },
-  { "help",             no_argument,       NULL, HELP_OPTION },
-  { "ip-addr",          required_argument, NULL, 'i' },
-  { "ipaddr",           required_argument, NULL, 'i' },
-  { "log",              required_argument, NULL, LOG_OPTION },
-  { "long-options",     no_argument,       NULL, LONG_OPTIONS_OPTION
},
-  { "new-style",        no_argument,       NULL, 'n' },
-  { "newstyle",         no_argument,       NULL, 'n' },
-  { "old-style",        no_argument,       NULL, 'o' },
-  { "oldstyle",         no_argument,       NULL, 'o' },
-  { "pid-file",         required_argument, NULL, 'P' },
-  { "pidfile",          required_argument, NULL, 'P' },
-  { "port",             required_argument, NULL, 'p' },
-  { "read-only",        no_argument,       NULL, 'r' },
-  { "readonly",         no_argument,       NULL, 'r' },
-  { "run",              required_argument, NULL, RUN_OPTION },
-  { "selinux-label",    required_argument, NULL, SELINUX_LABEL_OPTION
},
-  { "short-options",    no_argument,       NULL, SHORT_OPTIONS_OPTION
},
-  { "single",           no_argument,       NULL, 's' },
-  { "stdin",            no_argument,       NULL, 's' },
-  { "threads",          required_argument, NULL, 't' },
-  { "tls",              required_argument, NULL, TLS_OPTION },
-  { "tls-certificates", required_argument, NULL,
TLS_CERTIFICATES_OPTION },
-  { "tls-psk",          required_argument, NULL, TLS_PSK_OPTION },
-  { "tls-verify-peer",  no_argument,       NULL,
TLS_VERIFY_PEER_OPTION },
-  { "unix",             required_argument, NULL, 'U' },
-  { "user",             required_argument, NULL, 'u' },
-  { "verbose",          no_argument,       NULL, 'v' },
-  { "version",          no_argument,       NULL, 'V' },
-  { NULL },
-};
-
 static void
 usage (void)
 {
@@ -748,13 +687,6 @@ main (int argc, char *argv[])
   exit (EXIT_SUCCESS);
 }
 
-/* Is it a plugin or filter name relative to the plugindir/filterdir? */
-static int
-is_short_name (const char *filename)
-{
-  return strchr (filename, '.') == NULL && strchr (filename,
'/') == NULL;
-}
-
 /* Implementation of '-U -' */
 static char *
 make_random_fifo (void)
diff --git a/src/options.h b/src/options.h
new file mode 100644
index 0000000..2bbc96e
--- /dev/null
+++ b/src/options.h
@@ -0,0 +1,109 @@
+/* nbdkit
+ * Copyright (C) 2013-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.
+ */
+
+#ifndef NBDKIT_OPTIONS_H
+#define NBDKIT_OPTIONS_H
+
+#include <getopt.h>
+#include <limits.h>
+#include <string.h>
+
+enum {
+  HELP_OPTION = CHAR_MAX + 1,
+  DUMP_CONFIG_OPTION,
+  DUMP_PLUGIN_OPTION,
+  EXIT_WITH_PARENT_OPTION,
+  FILTER_OPTION,
+  LOG_OPTION,
+  LONG_OPTIONS_OPTION,
+  RUN_OPTION,
+  SELINUX_LABEL_OPTION,
+  SHORT_OPTIONS_OPTION,
+  TLS_OPTION,
+  TLS_CERTIFICATES_OPTION,
+  TLS_PSK_OPTION,
+  TLS_VERIFY_PEER_OPTION,
+};
+
+static const char *short_options = "D:e:fg:i:nop:P:rst:u:U:vV";
+static const struct option long_options[] = {
+  { "debug",            required_argument, NULL, 'D' },
+  { "dump-config",      no_argument,       NULL, DUMP_CONFIG_OPTION
},
+  { "dump-plugin",      no_argument,       NULL, DUMP_PLUGIN_OPTION
},
+  { "exit-with-parent", no_argument,       NULL,
EXIT_WITH_PARENT_OPTION },
+  { "export",           required_argument, NULL, 'e' },
+  { "export-name",      required_argument, NULL, 'e' },
+  { "exportname",       required_argument, NULL, 'e' },
+  { "filter",           required_argument, NULL, FILTER_OPTION },
+  { "foreground",       no_argument,       NULL, 'f' },
+  { "no-fork",          no_argument,       NULL, 'f' },
+  { "group",            required_argument, NULL, 'g' },
+  { "help",             no_argument,       NULL, HELP_OPTION },
+  { "ip-addr",          required_argument, NULL, 'i' },
+  { "ipaddr",           required_argument, NULL, 'i' },
+  { "log",              required_argument, NULL, LOG_OPTION },
+  { "long-options",     no_argument,       NULL, LONG_OPTIONS_OPTION
},
+  { "new-style",        no_argument,       NULL, 'n' },
+  { "newstyle",         no_argument,       NULL, 'n' },
+  { "old-style",        no_argument,       NULL, 'o' },
+  { "oldstyle",         no_argument,       NULL, 'o' },
+  { "pid-file",         required_argument, NULL, 'P' },
+  { "pidfile",          required_argument, NULL, 'P' },
+  { "port",             required_argument, NULL, 'p' },
+  { "read-only",        no_argument,       NULL, 'r' },
+  { "readonly",         no_argument,       NULL, 'r' },
+  { "run",              required_argument, NULL, RUN_OPTION },
+  { "selinux-label",    required_argument, NULL, SELINUX_LABEL_OPTION
},
+  { "short-options",    no_argument,       NULL, SHORT_OPTIONS_OPTION
},
+  { "single",           no_argument,       NULL, 's' },
+  { "stdin",            no_argument,       NULL, 's' },
+  { "threads",          required_argument, NULL, 't' },
+  { "tls",              required_argument, NULL, TLS_OPTION },
+  { "tls-certificates", required_argument, NULL,
TLS_CERTIFICATES_OPTION },
+  { "tls-psk",          required_argument, NULL, TLS_PSK_OPTION },
+  { "tls-verify-peer",  no_argument,       NULL,
TLS_VERIFY_PEER_OPTION },
+  { "unix",             required_argument, NULL, 'U' },
+  { "user",             required_argument, NULL, 'u' },
+  { "verbose",          no_argument,       NULL, 'v' },
+  { "version",          no_argument,       NULL, 'V' },
+  { NULL },
+};
+
+/* Is it a plugin or filter name relative to the plugindir/filterdir? */
+static inline int
+is_short_name (const char *filename)
+{
+  return strchr (filename, '.') == NULL && strchr (filename,
'/') == NULL;
+}
+
+#endif /* NBDKIT_OPTIONS_H */
-- 
2.19.0.rc0

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 the 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.
---
 Makefile.am  |  11 ++-
 README       |   2 +-
 configure.ac |   2 -
 nbdkit.in    | 160 ----------------------------------
 wrapper.c    | 237 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 248 insertions(+), 164 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index d5ef59f..cd41132 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -46,7 +46,16 @@ 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)
 
 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..3176cbd
--- /dev/null
+++ b/wrapper.c
@@ -0,0 +1,237 @@
+/* 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).
+ *------------------------------------------------------------
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.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
+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[])
+{
+  int c;
+  int long_index;
+  int verbose = 0;
+  size_t i;
+  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 (;;) {
+    long_index = -1;
+    c = getopt_long (argc, argv, short_options, long_options, &long_index);
+    if (c == -1)
+      break;
+
+    /* long_index is only set if it's an actual long option.  However
+     * in nbdkit all short options have long equivalents so we can
+     * associate those with a long_index, but we must do it ourselves.
+     * https://stackoverflow.com/a/34070441
+     */
+    if (long_index == -1) {
+      for (i = 0; long_options[i].name != NULL; ++i) {
+        if (long_options[i].val == c) {
+          long_index = i;
+          break;
+        }
+      }
+      if (long_index == -1)
+        abort ();
+    }
+
+    if (c == FILTER_OPTION) {
+      /* Filters can be rewritten if purely alphanumeric. */
+      if (is_short_name (optarg)) {
+        passthru_format ("--%s", /* --filter */
+                         long_options[long_index].name);
+        passthru_format ("%s/filters/%s/.libs/nbdkit-%s-filter.so",
+                         builddir, optarg, optarg);
+      }
+      else goto opt_with_arg;
+    }
+    else if (c == 'v') {
+      /* Verbose is special because we will print the final command. */
+      verbose = 1;
+      passthru_format ("--%s", long_options[long_index].name);
+    }
+    else if (long_options[long_index].has_arg) {
+      /* Remaining options that take an argument are passed through. */
+    opt_with_arg:
+      passthru_format ("--%s", long_options[long_index].name);
+      passthru (optarg);
+    }
+    else {
+      /* Remaining options that do not take an argument. */
+      passthru_format ("--%s", long_options[long_index].name);
+    }
+  }
+
+  /* Are there any non-option arguments? */
+  if (optind < argc) {
+    /* The first non-option argument is the plugin name.  If it is
+     * purely alphanumeric 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

On 11/13/18 4:51 PM, Richard W.M. Jones wrote:
> 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 the use a shell script as a shebang path.
s/the/to/
> 
> 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.
> ---
>   Makefile.am  |  11 ++-
>   README       |   2 +-
>   configure.ac |   2 -
>   nbdkit.in    | 160 ----------------------------------
>   wrapper.c    | 237 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 248 insertions(+), 164 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index d5ef59f..cd41132 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -46,7 +46,16 @@ 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
Maybe you could build the executable as 'wrapper' and then hard- or 
sym-link the name 'nbdkit' to the executable? Don't know if that
would
reduce or add to the confusion of having two separate binaries named 
nbdkit in the build tree.  Not a show-stopper, and the idea of a wrapper 
binary rather than a wrapper script seems interesting, regardless of 
what naming you settle on.
> +++ b/wrapper.c
> +
> +/*------------------------------------------------------------
> + * 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).
> + *------------------------------------------------------------
Comment matches the script version, with appropriate rewording.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.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
> +passthru_format (const char *fs, ...)
I'd give this a gcc __attribute__((printf...)), so that the compiler can 
help diagnose misuse.
> +{
> +  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);
> +}
Phew. I didn't see this on my first glance through, and wondered whether 
your realloc was passing trailing garbage to exec.
> +
> +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)
Is the 'i < len' check wasted effort, given that 'cmd[i] !=
NULL' is
sane after end_passthru(), and you don't print prior to then? Or is it 
intended that you could use 'p print_command' while in a gdb session?
> +    fprintf (stderr, " %s", cmd[i]);
Ambiguous output if the user passed in whitespace in an argument, but 
that's not too horrible (in bash, 'printf %q' would solve the
problem;
in C, it's actually more work to figure out how to quote just the things 
that need quoting, rather than to print everything the same way whether 
or not that can be reparsed the same as the original input)
> +  fprintf (stderr, "\n");
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  int c;
> +  int long_index;
> +  int verbose = 0;
Worth using <stdbool.h> and making this one bool?
> +  size_t i;
> +  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) {
The shell did this if NBDKIT_VALGRIND was set to anything non-empty; now 
you are requiring it to be exactly "1". Intentional, or do you really 
want 'if (s && *s)'?
> +    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) {
Ditto.
> +      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 (;;) {
> +    long_index = -1;
Why do we need this?

/me reads ahead...

Oh, because you are taking the lazy way out by rewriting all user's 
input back in long-opt form, regardless of how the user spelled it on 
input, rather than duplicating a switch statement that has to be 
maintained in parallel with main.c.
> +    c = getopt_long (argc, argv, short_options, long_options,
&long_index);
> +    if (c == -1)
> +      break;
> +
> +    /* long_index is only set if it's an actual long option.  However
> +     * in nbdkit all short options have long equivalents so we can
> +     * associate those with a long_index, but we must do it ourselves.
> +     * https://stackoverflow.com/a/34070441
> +     */
> +    if (long_index == -1) {
> +      for (i = 0; long_options[i].name != NULL; ++i) {
> +        if (long_options[i].val == c) {
> +          long_index = i;
> +          break;
> +        }
> +      }
> +      if (long_index == -1)
> +        abort ();
I can abort this, by passing in a garbage argument - at which point 
getopt_long returns '?', but that doesn't map back to
long_options[].

$ ./nbdkit -1
./nbdkit: invalid option -- '1'
Aborted (core dumped)
$ ./nbdkit --huh
./nbdkit: unrecognized option '--huh'
Aborted (core dumped)
$ ./nbdkit --filter
./nbdkit: option '--filter' requires an argument
Aborted (core dumped)

Best is probably to just exit(1) if c == '?' (you've already
diagnosed
the option error, so passing through to the real nbdkit would diagnose 
it again), or MAYBE to do the equivalent of "src/nbdkit --help" (but 
then still exit with non-zero status - that gets trickier, unless you 
also move the usage text to options.h in patch 1) to get the effects of 
normal nbdkit printing usage after improper options.
> +    }
Still, I wonder if this loop is necessary - if long_index is -1, you 
know the user passed a short opt, AND you know that printf("-%c", c) 
will spell that short opt (except for c == '?' - but you should already 
be special-casing that). So is it really necessary to map all the user's 
short options into a long option before doing the passthrough?
> +
> +    if (c == FILTER_OPTION) {
> +      /* Filters can be rewritten if purely alphanumeric. */
> +      if (is_short_name (optarg)) {
> +        passthru_format ("--%s", /* --filter */
> +                         long_options[long_index].name);
Here, you know that there is no short option for --filter, but you ALSO 
know that the option is spelled exactly "--filter", so why bother with
formatting the reverse lookup into long_options when you can just 
hardcode passthru("--format")?
> +        passthru_format
("%s/filters/%s/.libs/nbdkit-%s-filter.so",
> +                         builddir, optarg, optarg);
> +      }
> +      else goto opt_with_arg;
> +    }
> +    else if (c == 'v') {
> +      /* Verbose is special because we will print the final command. */
> +      verbose = 1;
> +      passthru_format ("--%s", long_options[long_index].name);
Okay, here if you don't reverse map long_index when the user passed a 
short option, then you have to do a runtime decision between "--%s" or
"-%c",
> +    }
> +    else if (long_options[long_index].has_arg) {
> +      /* Remaining options that take an argument are passed through. */
> +    opt_with_arg:
> +      passthru_format ("--%s", long_options[long_index].name);
and repeat that runtime decision,
> +      passthru (optarg);
> +    }
> +    else {
> +      /* Remaining options that do not take an argument. */
> +      passthru_format ("--%s", long_options[long_index].name);
and repeat again.  At the end of the day, it may still be slightly more 
efficient to avoid the reverse lookup loop, but I don't know if it 
becomes any easier to maintain (you'd be best off adding in a helper 
function rather than open-coding the decision).  Offhand,

void passthru_option (int c, int long_index)
{
   assert (c != '?');
   if (long_index == -1)
     passthru_format ("--%s", long_options[long_index].name);
   else {
     assert (c <= CHAR_MAX)
     passthru_format ("-%c", c);
   }
}
> +    }
> +  }
> +
> +  /* Are there any non-option arguments? */
> +  if (optind < argc) {
> +    /* The first non-option argument is the plugin name.  If it is
> +     * purely alphanumeric then rewrite it.
> +     */
> +    if (is_short_name (argv[optind])) {
is_short_name(" ") and is_short_name("-") both return true,
leading to
some unusual messages:

$ ./nbdkit -
nbdkit: /home/eblake/nbdkit/plugins/-/.libs/nbdkit---plugin.so: 
/home/eblake/nbdkit/plugins/-/.libs/nbdkit---plugin.so: cannot open 
shared object file: No such file or directory
$ ./nbdkit ' '
nbdkit: /home/eblake/nbdkit/plugins/ /.libs/nbdkit- -plugin.so: 
/home/eblake/nbdkit/plugins/ /.libs/nbdkit- -plugin.so: cannot open 
shared object file: No such file or directory

but that's pre-existing even with the real nbdkit, and not something you 
need to worry about in this patch other than the comment.  Among other 
things, the comment lies (while I can allow "-" as a stretch that is 
nearly alphanumeric, " " certainly doesn't fit the bill).  Note
that it
IS different from the shell script, where you did the rewrite only after 
checking the regex ^[a-zA-Z0-9]+$ (no '_'?) and was therefore purely 
alphanumeric - but by being consistent with src/main.c, now you can 
rewrite is_short_name() in a separate patch to get both the main binary 
and the wrapper to change their rules on how to treat the plugin name.
> +      /* 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;
> +    }
Hmm. If the user calls 'nbdkit file -- -my-file' as an alternative to 
'nbdkit file ./-my-file', you end up not repeating the "--"
argument.
You probably want to do passthru("--") right here, whether or not it
was
present in the caller's command line, to ensure that the real nbdkit 
doesn't see any remaining arguments as options.
> +
> +    /* Everything else is passed through without rewriting. */
> +    while (optind < argc) {
> +      passthru (argv[optind]);
> +      ++optind;
> +    }
> +  }
> +
> +  end_passthru ();
> +  if (verbose)
> +    print_command ();
The rewrite from './nbdkit -f' into '/path/to/src/nbdkit
--foreground'
looks weird; I argued that a helper function to preserve the short 
option spelling when possible might be nicer (you'll still rewrite 
'./nbdkit -vf' into '/path/to/src/nbdkit -v -f' even if you
preserve
short options, but that's not as drastic).  Similarly, the rewrite from 
'./nbdkit --filter=foo' into '/path/to/src/nbdkit --filter foo'
is
reasonable, as is './nbdkit -Pfile.pid' into '/path/to/src/nbdkit -P
file.pid'.
> +
> +  /* Run the final command. */
> +  execvp (cmd[0], (char **) cmd);
> +  perror (cmd[0]);
> +  exit (EXIT_FAILURE);
> +}
> 
Hmm - should 'nbdkit --help --garbage' warn about --garbage being 
unrecognized, or should it unconditionally print help regardless of the 
rest of the command line?  Right now it does the former; but if it 
switches to the latter, then you have to special-case --help in 
wrapper.c to get the same effect.  Ditto for --version.

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