Richard W.M. Jones
2018-Nov-13 22:51 UTC
[Libguestfs] [PATCH 0/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.
Richard W.M. Jones
2018-Nov-13 22:51 UTC
[Libguestfs] [PATCH 1/2] src: Move long_options short_options, is_short_name to a header file.
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
Richard W.M. Jones
2018-Nov-13 22:51 UTC
[Libguestfs] [PATCH 2/2] build: Replace ./nbdkit with a C program.
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
Eric Blake
2018-Nov-14 02:10 UTC
Re: [Libguestfs] [PATCH 2/2] build: Replace ./nbdkit with a C program.
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 = nbdkitMaybe 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
Seemingly Similar Threads
- Re: [PATCH 2/2] build: Replace ./nbdkit with a C program.
- Re: [PATCH 2/2] build: Replace ./nbdkit with a C program.
- [PATCH nbdkit v2 2/2] build: Replace ./nbdkit with a C program.
- Re: [PATCH 2/2] build: Replace ./nbdkit with a C program.
- [PATCH nbdkit 12/13] wrapper: Port the wrapper to run on Windows.