Re: [Libguestfs] [PATCH libguestfs 2/5] guestfish: write --help to stdout, use gnulib's progname module
Richard W.M. Jones wrote:
On Mon, Aug 24, 2009 at 04:26:16PM +0200, Jim Meyering wrote:
> Richard W.M. Jones wrote:
>
> > On Mon, Aug 24, 2009 at 02:21:51PM +0200, Jim Meyering wrote:
> >> Sure. That works, too.
> >> Here's the incremental:
> >
> > No, this doesn't fix the path munging to use real_argv0.
>
> "fix"?
> Is something incorrect, or is it just that you'd prefer
> to move the definition of real_argv0 "up"?
>
> I see only two uses of argv[0] in main:
>
> - path-related stuff *before* I change argv[0] and define real_argv0
>
> if (getenv ("LIBGUESTFS_PATH") == NULL &&
> argv[0] &&
> (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") !=
NULL))
> guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH);
>
> - then, this code, afterwards, that I did change to use real_argv0:
>
> strcpy (cmd, "a=`virt-inspector");
> while (optind < argc) {
> if (strlen (cmd) + strlen (argv[optind]) + strlen (real_argv0) + 60
> ...
> sprintf (&cmd[strlen(cmd)], "` && %s $a", real_argv0);
>
> If you'd prefer to move the definition of real_argv0 to precede
> the former, let me know, and I'll move it and adjust.
> However, note that that has no impact on correctness.
You're right - for some reason I thought the path-munging code
occurred after overwriting argv[0].
In general though I think the code is now much more prone to errors if
we rearrange the code in future. (It's already complex and prone to
introducing errors).
If the only benefit to rewriting argv[0] is that we get a slightly
better error message from getopt, then I'm inclined not to do it at
all. Leave all the argv[0] code as it is, and change anything else to
use program_name.
There are a couple of compelling arguments for making the
change in general:
- testing and reproducibility: with the change, all diagnostics
start with the constant string, "program_name: ".
Otherwise, you'll have most that are that way, yet a few
that then start with an absolute directory name and end in
"/.libs/lt-program_name: ". Worse still, in some environments,
that leading directory name will vary from run to run depending
on mount point names.
- overall uniformity. in building the tools and testing them, we've
seen how much output you may have to wade through when diagnosing a
problem. If we ensure that all diagnostics start with "program_name: "
it's that much easier to be sure that a quick use of grep finds all
error messages.
If it will make it easier to accept, I will add some small tests
that verify the expected behavior.
An alternative is not to modify argv at all, but
rather to make a copy of it, change argv_copy[0],
and to pass argv_copy to getopt_long.
Let me know.