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

On 11/13/18 4:51 PM, Richard W.M. Jones wrote: s/the/to/ Maybe 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. Comment matches the script version, with appropriate rewording. I'd give this a gcc __attribute__((printf...)), so that the compiler can help diagnose misuse. Phew. I didn't see this on my first glance through, and wondered whether your realloc was passing trailing garbage to exec. 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? 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) Worth using <stdbool.h> and making this one bool? 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)'? Ditto. 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. 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? 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'. 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