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

[Eric Blake]

On 11/14/18 3:45 AM, Richard W.M. Jones wrote:
> On Tue, Nov 13, 2018 at 08:10:34PM -0600, Eric Blake wrote:
>> On 11/13/18 4:51 PM, Richard W.M. Jones wrote:
> [...]
>>> +  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)'?
> 
> I was following the documentation rather than what the shell script
> did.  The existing code sets NBDKIT_VALGRIND=1 and in other tests we
> have awkward tests such as:
> 
>    if [ "$NBDKIT_VALGRIND" = "1" ]; then
> 
> so I guess the intention is NBDKIT_VALGRIND=1 rather than just is set.

Comparing all instances:

tests/test-dump-plugin-example4.sh:if [ "$NBDKIT_VALGRIND" = "1" ]; then

Looking for exactly "1" (if you run with NBDKIT_VALGRIND=2, the test is 
likely to fail)

tests/test-dump-plugin.sh:    case "$1${NBDKIT_VALGRIND:+-valgrind}" in
tests/test-help.sh:    case "$1${NBDKIT_VALGRIND:+-valgrind}" in
tests/test-python-exception.sh:if test -n "$NBDKIT_VALGRIND"; then
tests/test-version.sh:    case "$1${NBDKIT_VALGRIND:+-valgrind}" in

Looking for non-empty (any value except empty triggers it).

tests/test-ext2.c:  if (getenv ("NBDKIT_VALGRIND") != NULL) {
tests/test-lang-plugins.c:  if (getenv ("NBDKIT_VALGRIND") != NULL &&
tests/test-ocaml.c:  if (getenv ("NBDKIT_VALGRIND") != NULL) {

Looing for set at all (even to empty)

We aren't at all consistent, but you are also right that setting to 
exactly 1 is the only setting that currently gets past all of our tests 
without odd behavior.  I'm fine if you clean it up to be consistent as a 
separate patch (whether you stick with enforcing exactly "1" everywhere, 
or with either of the simpler "is set to anything, even empty" or "is 
set to anything non-empty").

>>> +    }
>>
>> 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?
> 
> I believe yes, we always need this loop, so that we can test
> long_options[long_index].has_arg.

Ah, but here, we can rely on the fact that optarg is NULL when there is 
no option (regardless of whether the option was short or long). (It's 
trickier if there are any optional_argument entries for has_arg - but 
since we stick to just no_argument and required_argument, we don't have 
to worry about that).

> 
>>> +
>>> +    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")?

I obviously meant passthru("--filter"), but you figured that out.

> I still think, because we test long_options.has_arg, we must compute
> long_index.  However in the second version I have made it preserve
> original long/short arguments using a variation of your technique
> described above.

I still think we don't have to probe has_arg, when optarg itself is a 
witness of whether there was an argument.

> 
>>> +
>>> +  /* 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.
> 
> I'm not sure.  At the moment you are right it does:
> 
> $ ./nbdkit --help --garbage
> ./nbdkit: unrecognized option '--garbage'
> 
> which seems reasonable to me (after all, if you want help, then
> just use --help ...)

And precedent for that: '[ --help' prints help, '[ --help --garbage' 
complains about a syntax error. So while it is often common to see 
programs that recognize --help at a higher priority regardless of what 
else is present, it is by no means universal, and people are already 
used to using exactly '--help' in isolation when that's what they really 
want.  So I'm fine with not changing this behavior.

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