[Libguestfs] [PATCH nbdkit v2 2/2] build: Replace ./nbdkit with a C program.

[Eric Blake]

On 11/14/18 3:53 AM, 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 to use a shell script as a shebang path.
> 
> Apart from the above, this is a straightforward translation of the
> original shell script into C and preserves all the existing features
> such as valgrind and gdb support.
> ---
>   Makefile.am  |  11 ++-
>   README       |   2 +-
>   configure.ac |   2 -
>   nbdkit.in    | 160 -------------------------------
>   wrapper.c    | 259 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 270 insertions(+), 164 deletions(-)
> 

> +
> +  /* 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) {

See my reply on the other thread about a potential followup for 
tree-wide consistency.

> +  /* 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;
> +
> +    if (c == '?')               /* getopt prints an error */
> +      exit (EXIT_FAILURE);
> +
> +    /* 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
> +     */
> +    is_long_option = long_index >= 0;
> +    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'm okay if you check the loop in as-is for readability sake, however, 
in my other thread I argued (and just now tested) that post-loop:

assert(!long_options[long_index].has_arg == !optarg);

will not kill the program, which means:

> +    /* Any short option. */
> +    else {
> +      /* Short option which takes an argument. */
> +      if (long_options[long_index].has_arg) {

This can be written as 'if (optarg)', removing the dependence on 
.has_arg, and thus removing the need for the reverse-lookup loop.

> +        passthru_format ("-%c", c);
> +        passthru (optarg);
> +      }
> +      /* Short option which takes no argument. */
> +      else
> +        passthru_format ("-%c", c);
> +    }
> +  }
> +

LGTM, whether or not you optimize out the loop.

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