thr3ads.net - Libguestfs - [Libguestfs] [nbdkit PATCH v2 1/5] maint: Improve ./nbdkit option parsing [Nov 2018]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2018-Nov-08 21:29 UTC

[Libguestfs] [nbdkit PATCH v2 0/5] log appends

v2 turned out to be much more involved, as I ended up fixing
several things along the way that I noticed while debugging a
feature addition.

Eric Blake (5):
  maint: Improve ./nbdkit option parsing
  main: Saner newline printing during --help
  utils: Add nbdkit_parse_bool
  main: Use new bool parser for --tls
  log: Allow user option of appending to log

 docs/nbdkit-plugin.pod            | 11 +++++++++++
 filters/log/nbdkit-log-filter.pod |  5 +++--
 include/nbdkit-common.h           |  1 +
 src/filters.c                     | 13 +++++++-----
 src/main.c                        | 16 ++++++---------
 src/plugins.c                     | 13 +++++++-----
 src/utils.c                       | 25 +++++++++++++++++++++++
 filters/log/log.c                 | 12 +++++++++--
 tests/test-log.sh                 |  6 +++++-
 nbdkit.in                         | 33 +++++++++++++++++++------------
 10 files changed, 97 insertions(+), 38 deletions(-)

-- 
2.17.2

Eric Blake

2018-Nov-08 21:29 UTC

head link

[Libguestfs] [nbdkit PATCH v2 1/5] maint: Improve ./nbdkit option parsing

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@redhat.com>
---
 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

Eric Blake

2018-Nov-08 21:29 UTC

head link

[Libguestfs] [nbdkit PATCH v2 2/5] main: Saner newline printing during --help

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@redhat.com>
---

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

Eric Blake

2018-Nov-08 21:29 UTC

head link

[Libguestfs] [nbdkit PATCH v2 3/5] utils: Add nbdkit_parse_bool

Parses the same set of boolean parameters as libguestfs:
https://github.com/libguestfs/libguestfs/blob/dd665e4bb/common/utils/utils.c#L345
with permission from Rich Jones to relax his code to BSD licensing:
https://www.redhat.com/archives/libguestfs/2018-November/msg00094.html

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-plugin.pod  | 11 +++++++++++
 include/nbdkit-common.h |  1 +
 src/utils.c             | 25 +++++++++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 594f2f3..4754d2c 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -763,6 +763,17 @@ size strings such as "100M" into the size in
bytes.
 C<str> can be a string in a number of common formats.  The function
 returns the size in bytes.  If there was an error, it returns C<-1>.

+=head1 PARSING BOOLEAN PARAMETERS
+
+Use the C<nbdkit_parse_bool> utility function to parse human-readable
+strings such as "on" into a boolean value.
+
+ int nbdkit_parse_bool (const char *str);
+
+C<str> can be a string containing a case-insensitive form of various
+common toggle values.  The function returns 0 or 1 if the parse was
+successful.  If there was an error, it returns C<-1>.
+
 =head1 READING PASSWORDS

 The C<nbdkit_read_password> utility function can be used to read
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index 47d2c47..9295b8a 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -66,6 +66,7 @@ extern void nbdkit_vdebug (const char *msg, va_list args);

 extern char *nbdkit_absolute_path (const char *path);
 extern int64_t nbdkit_parse_size (const char *str);
+extern int nbdkit_parse_bool (const char *str);
 extern int nbdkit_read_password (const char *value, char **password);
 extern char *nbdkit_realpath (const char *path);

diff --git a/src/utils.c b/src/utils.c
index c9e1e14..18011fd 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -158,6 +158,31 @@ nbdkit_parse_size (const char *str)
   return size * scale;
 }

+/* Parse a string as a boolean, or return -1 after reporting the error.
+ */
+int
+nbdkit_parse_bool (const char *str)
+{
+  if (!strcmp (str, "1") ||
+      !strcasecmp (str, "true") ||
+      !strcasecmp (str, "t") ||
+      !strcasecmp (str, "yes") ||
+      !strcasecmp (str, "y") ||
+      !strcasecmp (str, "on"))
+    return 1;
+
+  if (!strcmp (str, "0") ||
+      !strcasecmp (str, "false") ||
+      !strcasecmp (str, "f") ||
+      !strcasecmp (str, "no") ||
+      !strcasecmp (str, "n") ||
+      !strcasecmp (str, "off"))
+    return 0;
+
+  nbdkit_error ("could not decipher boolean (%s)", str);
+  return -1;
+}
+
 /* Read a password from configuration value. */
 int
 nbdkit_read_password (const char *value, char **password)
-- 
2.17.2

Eric Blake

2018-Nov-08 21:29 UTC

head link

[Libguestfs] [nbdkit PATCH v2 4/5] main: Use new bool parser for --tls

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@redhat.com>
---

 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

Eric Blake

2018-Nov-08 21:29 UTC

head link

[Libguestfs] [nbdkit PATCH v2 5/5] log: Allow user option of appending to log

Always truncating a log can wipe out useful history. Add an
option to change this.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

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

Eric Blake

2018-Nov-08 21:53 UTC

head link

Re: [Libguestfs] [nbdkit PATCH v2 1/5] maint: Improve ./nbdkit option parsing

On 11/8/18 3:29 PM, Eric Blake wrote:> We had several poor option-parsing actions in our ./nbdkit wrapper:
> 
> @@ -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.
I'll fix the TAB damage before pushing.

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

Richard W.M. Jones

2018-Nov-09 08:58 UTC

head link

Re: [Libguestfs] [nbdkit PATCH v2 4/5] main: Use new bool parser for --tls

On Thu, Nov 08, 2018 at 03:29:51PM -0600, Eric Blake
wrote:> 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@redhat.com>
> ---
> 
>  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)
Stylisticly I guess we use tls == -1 elsewhere, but it's not
a big deal.

Rich.
> +          exit (EXIT_FAILURE);
>        }
>        break;
> 
> -- 
> 2.17.2
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Richard W.M. Jones

2018-Nov-09 08:58 UTC

head link

Re: [Libguestfs] [nbdkit PATCH v2 5/5] log: Allow user option of appending to log

The whole series looks good except for the minor stylistic
point I made about patch 4.  Hence:

ACK series.

Thanks, Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Reasonably Related Threads

Search for more reasonably related threads

Libguestfs - Nov 2018 - [nbdkit PATCH v2 1/5] maint: Improve ./nbdkit option parsing

[Libguestfs] [nbdkit PATCH v2 0/5] log appends

[Libguestfs] [nbdkit PATCH v2 1/5] maint: Improve ./nbdkit option parsing

[Libguestfs] [nbdkit PATCH v2 2/5] main: Saner newline printing during --help

[Libguestfs] [nbdkit PATCH v2 3/5] utils: Add nbdkit_parse_bool

[Libguestfs] [nbdkit PATCH v2 4/5] main: Use new bool parser for --tls

[Libguestfs] [nbdkit PATCH v2 5/5] log: Allow user option of appending to log

Re: [Libguestfs] [nbdkit PATCH v2 1/5] maint: Improve ./nbdkit option parsing

Re: [Libguestfs] [nbdkit PATCH v2 4/5] main: Use new bool parser for --tls

Re: [Libguestfs] [nbdkit PATCH v2 5/5] log: Allow user option of appending to log

Reasonably Related Threads