We had several poor option-parsing actions in our ./nbdkit wrapper: Attempting './nbdkit --filter' went into an infloop with growing memory, because bash treats 'shift 2' when $# as a soft error (which we ignored) without even shifting 1, such that $# never decreases but $args[] continues to grow (dash, on the other hand, follows the POSIX recommendation of a hard error with immediate script termination in a non-interactive shell). Fix by checking that $# is large enough before attempting to shift 2. Commit 091ce402 tried to make './nbdkit --export=foo' work the same as './nbdkit --export foo' - but has no testsuite coverage, and silently eats the option instead. Fix by incrementing i in that case label. Attempting './nbdkit --ipaddr=$addr plugin' doesn't special case 'plugin' properly, because we forgot to special-case '--ip*=*' similarly to --export and --pid. Fix by generalizing the early glob to catch ALL longopts used with = rather than just a select few, but only after we have handled the special-casing of --filter[=] first (the biggest part of this patch is thus code motion). We forgot to add -D/--debug and --log from recent command line additions. Fix by adding more patterns; and split the long line while at it. Not fixed: the real nbdkit allows things like 'nbdkit -thr 1 ...' while we don't; it's not worth teaching our debug wrapper about unambiguous abbreviations. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- nbdkit.in | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/nbdkit.in b/nbdkit.in index 0469f1a..a6318a4 100644 --- a/nbdkit.in +++ b/nbdkit.in @@ -64,19 +64,7 @@ verbose= while [ $# -gt 0 ]; do case "$1" in - # Flags that take an argument. We must not rewrite the argument. - # But make sure globs don't mishandle longopts with =. - --export*=* | --pid*=*) - args[$i]="$1" - shift - ;; - -e | --export* | -g | --group | -i | --ip* | -P | --pid* | -p | --port | --run | --selinux-label | -t | --threads | --tls | --tls-certificates | --tls-psk | -U | --unix | -u | --user) - args[$i]="$1" - ((++i)) - args[$i]="$2" - ((++i)) - shift 2 - ;; + # Options that we special-case. -v | --verbose) verbose=1 args[$i]="$1" @@ -93,6 +81,7 @@ while [ $# -gt 0 ]; do --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" @@ -106,6 +95,24 @@ while [ $# -gt 0 ]; do 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. *) -- 2.17.2

We are inconsistent on whether our plugins and filters end in a newline; as a result, things like nbdkit --help --filter=log --filter=nozero --filter=delay memory have inconsistent spacing between sections. Rather than documenting whether the user must have/avoid a trailing newline, it's nicer to just supply one ourselves only when it is missing. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- I'm not sure if it's worth factoring out a helper function that either prints newline or returns ""/"\n" based on whether the passed in parameter ends in newline; in this patch, it is open-coded 4 times. src/filters.c | 13 ++++++++----- src/plugins.c | 13 ++++++++----- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/filters.c b/src/filters.c index 3626742..773f4df 100644 --- a/src/filters.c +++ b/src/filters.c @@ -134,19 +134,22 @@ static void filter_usage (struct backend *b) { struct backend_filter *f = container_of (b, struct backend_filter, backend); + const char *p; printf ("filter: %s", f->name); if (f->filter.longname) printf (" (%s)", f->filter.longname); printf ("\n"); - printf ("(%s)", f->filename); + printf ("(%s)\n", f->filename); if (f->filter.description) { - printf ("\n"); - printf ("%s\n", f->filter.description); + printf ("%s", f->filter.description); + if ((p = strrchr (f->filter.description, '\n')) == NULL || p[1]) + printf ("\n"); } if (f->filter.config_help) { - printf ("\n"); - printf ("%s\n", f->filter.config_help); + printf ("%s", f->filter.config_help); + if ((p = strrchr (f->filter.config_help, '\n')) == NULL || p[1]) + printf ("\n"); } } diff --git a/src/plugins.c b/src/plugins.c index 2bea6ac..912c226 100644 --- a/src/plugins.c +++ b/src/plugins.c @@ -103,19 +103,22 @@ static void plugin_usage (struct backend *b) { struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + const char *t; printf ("plugin: %s", p->name); if (p->plugin.longname) printf (" (%s)", p->plugin.longname); printf ("\n"); - printf ("(%s)", p->filename); + printf ("(%s)\n", p->filename); if (p->plugin.description) { - printf ("\n"); - printf ("%s\n", p->plugin.description); + printf ("%s", p->plugin.description); + if ((t = strrchr (p->plugin.description, '\n')) == NULL || t[1]) + printf ("\n"); } if (p->plugin.config_help) { - printf ("\n"); - printf ("%s\n", p->plugin.config_help); + printf ("%s", p->plugin.config_help); + if ((t = strrchr (p->plugin.config_help, '\n')) == NULL || t[1]) + printf ("\n"); } } -- 2.17.2

Our use of --tls is a superset of boolean because we add a force mode; but once we have checked for that, it's more consistent if we accept the same set of other boolean values as anything else (in this case, adding 'yes/no' and 'true/false'), as well as consistently allowing case-insensitivity. The error message for an unrecognized spelling changes from: $ nbdkit --tls huh nbdkit: --tls flag must be off|on|require to: $ nbdkit --tls huh nbdkit: error: could not decipher boolean (huh) but I don't think that hurts too much. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/main.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/main.c b/src/main.c index 7ebbba6..0a883e1 100644 --- a/src/main.c +++ b/src/main.c @@ -376,18 +376,14 @@ main (int argc, char *argv[]) case TLS_OPTION: tls_set_on_cli = 1; - if (strcmp (optarg, "off") == 0 || strcmp (optarg, "0") == 0) - tls = 0; - else if (strcmp (optarg, "on") == 0 || strcmp (optarg, "1") == 0) - tls = 1; - else if (strcmp (optarg, "require") == 0 || - strcmp (optarg, "required") == 0 || - strcmp (optarg, "force") == 0) + if (strcasecmp (optarg, "require") == 0 || + strcasecmp (optarg, "required") == 0 || + strcasecmp (optarg, "force") == 0) tls = 2; else { - fprintf (stderr, "%s: --tls flag must be off|on|require\n", - program_name); - exit (EXIT_FAILURE); + tls = nbdkit_parse_bool (optarg); + if (tls < 0) + exit (EXIT_FAILURE); } break; -- 2.17.2

Always truncating a log can wipe out useful history. Add an option to change this. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v2: tweaks to documentation, rely on nbdkit_parse_bool filters/log/nbdkit-log-filter.pod | 5 +++-- filters/log/log.c | 12 ++++++++++-- tests/test-log.sh | 6 +++++- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/filters/log/nbdkit-log-filter.pod b/filters/log/nbdkit-log-filter.pod index 0903329..cff209e 100644 --- a/filters/log/nbdkit-log-filter.pod +++ b/filters/log/nbdkit-log-filter.pod @@ -4,7 +4,7 @@ nbdkit-log-filter - nbdkit log filter =head1 SYNOPSIS - nbdkit --filter=log plugin logfile=FILE [plugin-args...] + nbdkit --filter=log plugin logfile=FILE [logappend=BOOL] [plugin-args...] =head1 DESCRIPTION @@ -22,7 +22,8 @@ work even when nbdkit is run as a daemon. The nbdkit-log-filter requires a single parameter C<logfile> which specifies the path of the file to use for logging. If the file -already exists, it will be truncated. +already exists, it will be truncated unless the C<logappend> parameter +was specified with a value that can be parsed as a boolean true. =head1 EXAMPLES diff --git a/filters/log/log.c b/filters/log/log.c index 2079084..a497e9e 100644 --- a/filters/log/log.c +++ b/filters/log/log.c @@ -51,6 +51,7 @@ static uint64_t connections; static char *logfilename; static FILE *logfile; +static int append; static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER; static void @@ -75,6 +76,12 @@ log_config (nbdkit_next_config *next, void *nxdata, } return 0; } + if (strcmp (key, "logappend") == 0) { + append = nbdkit_parse_bool (value); + if (append < 0) + return -1; + return 0; + } return next (nxdata, key, value); } @@ -86,7 +93,7 @@ log_config_complete (nbdkit_next_config_complete *next, void *nxdata) nbdkit_error ("missing logfile= parameter for the log filter"); return -1; } - logfile = fopen (logfilename, "w"); + logfile = fopen (logfilename, append ? "a" : "w"); if (!logfile) { nbdkit_error ("fopen: %m"); return -1; @@ -96,7 +103,8 @@ log_config_complete (nbdkit_next_config_complete *next, void *nxdata) } #define log_config_help \ - "logfile=<FILE> The file to place the log in." + "logfile=<FILE> (required) The file to place the log in.\n" \ + "logappend=<BOOL> True to append to the log (default false).\n" struct handle { uint64_t connection; diff --git a/tests/test-log.sh b/tests/test-log.sh index f0eacb7..b30d20e 100755 --- a/tests/test-log.sh +++ b/tests/test-log.sh @@ -45,7 +45,9 @@ if ! qemu-io -f raw -c 'w 1M 2M' log.img; then fi # Run nbdkit with logging enabled to file. -start_nbdkit -P log.pid -U log.sock --filter=log file log.img logfile=log.log +echo '# My log' > log.log +start_nbdkit -P log.pid -U log.sock --filter=log file log.img \ + logfile=log.log logappend=1 # For easier debugging, dump the final log files before removing them # on exit. @@ -61,6 +63,8 @@ cleanup_fn cleanup qemu-io -f raw -c 'w -P 11 1M 2M' 'nbd+unix://?socket=log.sock' qemu-io -r -f raw -c 'r -P 11 2M 1M' 'nbd+unix://?socket=log.sock' +# The log should have been appended, preserving our marker. +grep '# My log' log.log # The log should show a write on connection 1, and read on connection 2. grep 'connection=1 Write id=1 offset=0x100000 count=0x200000 ' log.log grep 'connection=2 Read id=1 offset=0x200000 count=0x100000 ' log.log -- 2.17.2