Jim Meyering wrote:

Richard W.M. Jones wrote:

...

On Mon, Aug 24, 2009 at 12:21:54PM +0200, Jim Meyering wrote:

...

+ /* getopt_long uses argv[0], so give it the sanitized name, too. */ + argv[0] = bad_cast (program_name);

This is wrong.

We rely on seeing the original argv[0] later in the code, so that people can run ./fish/guestfish from the source directory and have the appliance path set correctly:

/* If developing, add ./appliance to the path. Note that libtools * interferes with this because uninstalled guestfish is a shell * script that runs the real program with an absolute path. Detect * that too. * * BUT if LIBGUESTFS_PATH environment variable is already set by * the user, then don't override it. */ if (getenv ("LIBGUESTFS_PATH") == NULL && argv[0] && (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") != NULL)) guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH);

Good catch. I'll adapt by saving argv[0] just before calling getopt_long, setting it as above, and then restoring the original, afterwards.

Here's an incremental patch I'll fold onto the one discussed above: diff --git a/fish/fish.c b/fish/fish.c index 9f0f8df..547342f 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -139,9 +139,6 @@ main (int argc, char *argv[]) /* Set global program name that is not polluted with libtool artifacts. */ set_program_name (argv[0]); - /* getopt_long uses argv[0], so give it the sanitized name, too. */ - argv[0] = bad_cast (program_name); - atexit (close_stdout); enum { HELP_OPTION = CHAR_MAX + 1 }; @@ -205,6 +202,11 @@ main (int argc, char *argv[]) (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") != NULL)) guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH); + /* getopt_long uses argv[0], so give it the sanitized name, too. + But only temporarily. */ + char *argv0 = argv[0]; + argv[0] = bad_cast (program_name); + for (;;) { c = getopt_long (argc, argv, options, long_options, &option_index); if (c == -1) break; @@ -325,6 +327,9 @@ main (int argc, char *argv[]) } } + /* Restore original value. */ + argv[0] = argv0; + /* Inspector mode invalidates most of the other arguments. */ if (inspector) { char cmd[1024];

Richard W.M. Jones wrote:

On Mon, Aug 24, 2009 at 01:51:18PM +0200, Jim Meyering wrote:

...

@@ -205,6 +202,11 @@ main (int argc, char *argv[]) (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") != NULL)) guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH);

+ /* getopt_long uses argv[0], so give it the sanitized name, too. + But only temporarily. */ + char *argv0 = argv[0]; + argv[0] = bad_cast (program_name); + for (;;) { c = getopt_long (argc, argv, options, long_options, &option_index); if (c == -1) break;

...

@@ -325,6 +327,9 @@ main (int argc, char *argv[]) } }

+ /* Restore original value. */ + argv[0] = argv0; + /* Inspector mode invalidates most of the other arguments. */ if (inspector) { char cmd[1024];

I'm maybe missing something, but why restore it?

How about instead something like:

char *real_argv0 = argv[0]; argv[0] = bad_cast (program_name);

//...

/* change the path-munging code to use real_argv0 */

//...

/* don't bother restoring argv[0] */

Sure. That works, too. Here's the incremental: diff --git a/fish/fish.c b/fish/fish.c index baa1b6e..e799f86 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -203,7 +203,7 @@ main (int argc, char *argv[]) /* getopt_long uses argv[0], so give it the sanitized name, too. But only temporarily. */ - char *argv0 = argv[0]; + char *real_argv0 = argv[0]; argv[0] = bad_cast (program_name); for (;;) { @@ -328,9 +328,6 @@ main (int argc, char *argv[]) } } - /* Restore original value. */ - argv[0] = argv0; - /* Inspector mode invalidates most of the other arguments. */ if (inspector) { char cmd[1024]; @@ -352,7 +349,7 @@ main (int argc, char *argv[]) strcpy (cmd, "a=`virt-inspector"); while (optind < argc) { - if (strlen (cmd) + strlen (argv[optind]) + strlen (argv[0]) + 60 + if (strlen (cmd) + strlen (argv[optind]) + strlen (real_argv0) + 60 >= sizeof cmd) { fprintf (stderr, _("%s: virt-inspector command too long for fixed-size buffer\n"), @@ -370,7 +367,7 @@ main (int argc, char *argv[]) else strcat (cmd, " --fish"); - sprintf (&cmd[strlen(cmd)], "` && %s $a", argv[0]); + sprintf (&cmd[strlen(cmd)], "` && %s $a", real_argv0); if (guestfs_get_verbose (g)) strcat (cmd, " -v");

Richard W.M. Jones wrote:

On Mon, Aug 24, 2009 at 05:46:03PM +0200, Jim Meyering wrote:

...

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.

Well OK. What's the final version of this patch (not incremental)?

Here it is:

From 5fbc1b89dc7d23171565eb6f28105ac001d5cacd Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 21 Aug 2009 19:05:20 +0200 Subject: [PATCH libguestfs 2/5] guestfish: write --help to stdout, use gnulib's progname module

* fish/fish.c: Include "progname.h". (main): Call set_program_name to initialize. Don't hard-code guestfish everywhere. Use program_name. However, be careful when modifying argv[0], since it is used in the hopes that it is an absolute file name. (usage): Don't spew all of --help for a mis-typed option. Split long lines. --- bootstrap | 1 + fish/Makefile.am | 3 +- fish/fish.c | 130 +++++++++++++++++++++++++++++++++++------------------ 3 files changed, 89 insertions(+), 45 deletions(-) diff --git a/bootstrap b/bootstrap index 76e2216..3fd8811 100755 --- a/bootstrap +++ b/bootstrap @@ -56,6 +56,7 @@ gnumakefile ignore-value maintainer-makefile manywarnings +progname warnings vc-list-files ' diff --git a/fish/Makefile.am b/fish/Makefile.am index 94bf757..c8ba3ea 100644 --- a/fish/Makefile.am +++ b/fish/Makefile.am @@ -52,13 +52,14 @@ guestfish_CFLAGS = \ -I$(top_srcdir)/src -I$(top_builddir)/src \ -I$(top_srcdir)/fish -I$(top_builddir)/fish \ -DGUESTFS_DEFAULT_PATH='"$(libdir)/guestfs"' \ + -I$(srcdir)/../gnulib/lib -I../gnulib/lib \ $(WARN_CFLAGS) $(WERROR_CFLAGS) guestfish_LDADD = $(top_builddir)/src/libguestfs.la $(LIBREADLINE) # Make libguestfs use the convenience library. noinst_LTLIBRARIES = librc_protocol.la -guestfish_LDADD += librc_protocol.la +guestfish_LDADD += librc_protocol.la ../gnulib/lib/libgnu.la if HAVE_RPCGEN rc_protocol.c: rc_protocol.x diff --git a/fish/fish.c b/fish/fish.c index 3acd450..e799f86 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -40,6 +40,7 @@ #include <guestfs.h> #include "fish.h" +#include "progname.h" struct mp { struct mp *next; @@ -87,21 +88,25 @@ launch (guestfs_h *_g) return 0; } -static void -usage (void) +static void __attribute__((noreturn)) +usage (int status) { - fprintf (stderr, - _("guestfish: guest filesystem shell\n" - "guestfish lets you edit virtual machine filesystems\n" + if (status != EXIT_SUCCESS) + fprintf (stderr, _("Try `%s --help' for more information.\n"), + program_name); + else { + fprintf (stdout, + _("%s: guest filesystem shell\n" + "%s lets you edit virtual machine filesystems\n" "Copyright (C) 2009 Red Hat Inc.\n" "Usage:\n" - " guestfish [--options] cmd [: cmd : cmd ...]\n" - " guestfish -i libvirt-domain\n" - " guestfish -i disk-image(s)\n" + " %s [--options] cmd [: cmd : cmd ...]\n" + " %s -i libvirt-domain\n" + " %s -i disk-image(s)\n" "or for interactive use:\n" - " guestfish\n" + " %s\n" "or from a shell script:\n" - " guestfish <<EOF\n" + " %s <<EOF\n" " cmd\n" " ...\n" " EOF\n" @@ -115,24 +120,34 @@ usage (void) " --listen Listen for remote commands\n" " -m|--mount dev[:mnt] Mount dev on mnt (if omitted, /)\n" " -n|--no-sync Don't autosync\n" - " --remote[=pid] Send commands to remote guestfish\n" + " --remote[=pid] Send commands to remote %s\n" " -r|--ro Mount read-only\n" " --selinux Enable SELinux support\n" " -v|--verbose Verbose messages\n" " -x Echo each command before executing it\n" " -V|--version Display version and exit\n" - "For more information, see the manpage guestfish(1).\n")); + "For more information, see the manpage %s(1).\n"), + program_name, program_name, program_name, + program_name, program_name, program_name, + program_name, program_name, program_name); + } + exit (status); } int main (int argc, char *argv[]) { + /* Set global program name that is not polluted with libtool artifacts. */ + set_program_name (argv[0]); + + enum { HELP_OPTION = CHAR_MAX + 1 }; + static const char *options = "a:Df:h::im:nrv?Vx"; static const struct option long_options[] = { { "add", 1, 0, 'a' }, { "cmd-help", 2, 0, 'h' }, { "file", 1, 0, 'f' }, - { "help", 0, 0, '?' }, + { "help", 0, 0, HELP_OPTION }, { "inspector", 0, 0, 'i' }, { "listen", 0, 0, 0 }, { "mount", 1, 0, 'm' }, @@ -186,6 +201,11 @@ main (int argc, char *argv[]) (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") != NULL)) guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH); + /* getopt_long uses argv[0], so give it the sanitized name, too. + But only temporarily. */ + char *real_argv0 = argv[0]; + argv[0] = bad_cast (program_name); + for (;;) { c = getopt_long (argc, argv, options, long_options, &option_index); if (c == -1) break; @@ -197,21 +217,24 @@ main (int argc, char *argv[]) else if (strcmp (long_options[option_index].name, "remote") == 0) { if (optarg) { if (sscanf (optarg, "%d", &remote_control) != 1) { - fprintf (stderr, _("guestfish: --listen=PID: PID was not a number: %s\n"), optarg); + fprintf (stderr, _("%s: --listen=PID: PID was not a number: %s\n"), + program_name, optarg); exit (1); } } else { p = getenv ("GUESTFISH_PID"); if (!p || sscanf (p, "%d", &remote_control) != 1) { - fprintf (stderr, _("guestfish: remote: $GUESTFISH_PID must be set to the PID of the remote process\n")); + fprintf (stderr, _("%s: remote: $GUESTFISH_PID must be set" + " to the PID of the remote process\n"), + program_name); exit (1); } } } else if (strcmp (long_options[option_index].name, "selinux") == 0) { guestfs_set_selinux (g, 1); } else { - fprintf (stderr, _("guestfish: unknown long option: %s (%d)\n"), - long_options[option_index].name, option_index); + fprintf (stderr, _("%s: unknown long option: %s (%d)\n"), + program_name, long_options[option_index].name, option_index); exit (1); } break; @@ -237,7 +260,8 @@ main (int argc, char *argv[]) case 'f': if (file) { - fprintf (stderr, _("guestfish: only one -f parameter can be given\n")); + fprintf (stderr, _("%s: only one -f parameter can be given\n"), + program_name); exit (1); } file = optarg; @@ -287,21 +311,20 @@ main (int argc, char *argv[]) break; case 'V': - printf ("guestfish %s\n", PACKAGE_VERSION); + printf ("%s %s\n", program_name, PACKAGE_VERSION); exit (0); case 'x': echo_commands = 1; break; - case '?': - usage (); - exit (0); + case HELP_OPTION: + usage (0); default: - fprintf (stderr, _("guestfish: unexpected command line option 0x%x\n"), - c); - exit (1); + fprintf (stderr, _("%s: unexpected command line option 0x%x\n"), + program_name, c); + usage (1); } } @@ -312,19 +335,25 @@ main (int argc, char *argv[]) if (drvs || mps || remote_control_listen || remote_control || guestfs_get_selinux (g)) { - fprintf (stderr, _("guestfish: cannot use -i option with -a, -m, --listen, --remote or --selinux\n")); + fprintf (stderr, _("%s: cannot use -i option with -a, -m," + " --listen, --remote or --selinux\n"), + program_name); exit (1); } if (optind >= argc) { - fprintf (stderr, _("guestfish -i requires a libvirt domain or path(s) to disk image(s)\n")); + fprintf (stderr, + _("%s: -i requires a libvirt domain or path(s) to disk image(s)\n"), + program_name); exit (1); } strcpy (cmd, "a=`virt-inspector"); while (optind < argc) { - if (strlen (cmd) + strlen (argv[optind]) + strlen (argv[0]) + 60 + if (strlen (cmd) + strlen (argv[optind]) + strlen (real_argv0) + 60 >= sizeof cmd) { - fprintf (stderr, _("guestfish: virt-inspector command too long for fixed-size buffer\n")); + fprintf (stderr, + _("%s: virt-inspector command too long for fixed-size buffer\n"), + program_name); exit (1); } strcat (cmd, " '"); @@ -338,7 +367,7 @@ main (int argc, char *argv[]) else strcat (cmd, " --fish"); - sprintf (&cmd[strlen(cmd)], "` && %s $a", argv[0]); + sprintf (&cmd[strlen(cmd)], "` && %s $a", real_argv0); if (guestfs_get_verbose (g)) strcat (cmd, " -v"); @@ -347,7 +376,7 @@ main (int argc, char *argv[]) if (verbose) fprintf (stderr, - "guestfish -i: running virt-inspector command:\n%s\n", cmd); + "%s -i: running virt-inspector command:\n%s\n", program_name, cmd); r = system (cmd); if (r == -1) { @@ -368,17 +397,23 @@ main (int argc, char *argv[]) /* Remote control? */ if (remote_control_listen && remote_control) { - fprintf (stderr, _("guestfish: cannot use --listen and --remote options at the same time\n")); + fprintf (stderr, + _("%s: cannot use --listen and --remote options at the same time\n"), + program_name); exit (1); } if (remote_control_listen) { if (optind < argc) { - fprintf (stderr, _("guestfish: extra parameters on the command line with --listen flag\n")); + fprintf (stderr, + _("%s: extra parameters on the command line with --listen flag\n"), + program_name); exit (1); } if (file) { - fprintf (stderr, _("guestfish: cannot use --listen and --file options at the same time\n")); + fprintf (stderr, + _("%s: cannot use --listen and --file options at the same time\n"), + program_name); exit (1); } rc_listen (); @@ -602,12 +637,14 @@ script (int prompt) p++; len = strcspn (p, "\""); if (p[len] == '\0') { - fprintf (stderr, _("guestfish: unterminated double quote\n")); + fprintf (stderr, _("%s: unterminated double quote\n"), program_name); if (exit_on_error) exit (1); goto next_command; } if (p[len+1] && (p[len+1] != ' ' && p[len+1] != '\t')) { - fprintf (stderr, _("guestfish: command arguments not separated by whitespace\n")); + fprintf (stderr, + _("%s: command arguments not separated by whitespace\n"), + program_name); if (exit_on_error) exit (1); goto next_command; } @@ -617,12 +654,14 @@ script (int prompt) p++; len = strcspn (p, "'"); if (p[len] == '\0') { - fprintf (stderr, _("guestfish: unterminated single quote\n")); + fprintf (stderr, _("%s: unterminated single quote\n"), program_name); if (exit_on_error) exit (1); goto next_command; } if (p[len+1] && (p[len+1] != ' ' && p[len+1] != '\t')) { - fprintf (stderr, _("guestfish: command arguments not separated by whitespace\n")); + fprintf (stderr, + _("%s: command arguments not separated by whitespace\n"), + program_name); if (exit_on_error) exit (1); goto next_command; } @@ -643,12 +682,15 @@ script (int prompt) pend++; } if (c != 0) { - fprintf (stderr, _("guestfish: unterminated \"[...]\" sequence\n")); + fprintf (stderr, + _("%s: unterminated \"[...]\" sequence\n"), program_name); if (exit_on_error) exit (1); goto next_command; } if (*pend && (*pend != ' ' && *pend != '\t')) { - fprintf (stderr, _("guestfish: command arguments not separated by whitespace\n")); + fprintf (stderr, + _("%s: command arguments not separated by whitespace\n"), + program_name); if (exit_on_error) exit (1); goto next_command; } @@ -667,8 +709,8 @@ script (int prompt) } else pend = &p[len]; } else { - fprintf (stderr, _("guestfish: internal error parsing string at '%s'\n"), - p); + fprintf (stderr, _("%s: internal error parsing string at '%s'\n"), + program_name, p); abort (); } @@ -684,7 +726,7 @@ script (int prompt) } if (i == sizeof argv / sizeof argv[0]) { - fprintf (stderr, _("guestfish: too many arguments\n")); + fprintf (stderr, _("%s: too many arguments\n"), program_name); if (exit_on_error) exit (1); goto next_command; } @@ -713,7 +755,7 @@ cmdline (char *argv[], int optind, int argc) cmd = argv[optind++]; if (strcmp (cmd, ":") == 0) { - fprintf (stderr, _("guestfish: empty command on command line\n")); + fprintf (stderr, _("%s: empty command on command line\n"), program_name); exit (1); } params = &argv[optind]; -- 1.6.4.378.g88f2f

Richard W.M. Jones wrote:

On Mon, Aug 24, 2009 at 06:00:19PM +0200, Jim Meyering wrote:

...

@@ -186,6 +201,11 @@ main (int argc, char *argv[]) (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") != NULL)) guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH);

+ /* getopt_long uses argv[0], so give it the sanitized name, too. + But only temporarily. */ + char *real_argv0 = argv[0]; + argv[0] = bad_cast (program_name);

ACK. Do you think you could change that comment to make it really clear that this piece of code mustn't ever move above the path munging done in the preceeding lines.

Sure. Here's the incremental: diff --git a/fish/fish.c b/fish/fish.c index e799f86..91c5dad 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -201,8 +201,12 @@ main (int argc, char *argv[]) (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") != NULL)) guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH); - /* getopt_long uses argv[0], so give it the sanitized name, too. - But only temporarily. */ + /* CAUTION: we are careful to modify argv[0] here, only after + * using it just above. + * + * getopt_long uses argv[0], so give it the sanitized name. Save a copy + * of the original, in case it's needed in virt-inspector mode, below. + */ char *real_argv0 = argv[0]; argv[0] = bad_cast (program_name);

Jim Meyering wrote:

Richard W.M. Jones wrote:

...

On Mon, Aug 24, 2009 at 12:21:54PM +0200, Jim Meyering wrote:

...

+ /* getopt_long uses argv[0], so give it the sanitized name, too. */ + argv[0] = bad_cast (program_name);

This is wrong.

We rely on seeing the original argv[0] later in the code, so that people can run ./fish/guestfish from the source directory and have the appliance path set correctly:

/* If developing, add ./appliance to the path. Note that libtools * interferes with this because uninstalled guestfish is a shell * script that runs the real program with an absolute path. Detect * that too. * * BUT if LIBGUESTFS_PATH environment variable is already set by * the user, then don't override it. */ if (getenv ("LIBGUESTFS_PATH") == NULL && argv[0] && (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") != NULL)) guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH);

Good catch. I'll adapt by saving argv[0] just before calling getopt_long, setting it as above, and then restoring the original, afterwards.

BTW, here's the complete, adjusted change-set:

From bdc25e068deaaa3f80d5d0dc773b41fa9c84d503 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 21 Aug 2009 19:05:20 +0200 Subject: [PATCH libguestfs 2/5] guestfish: write --help to stdout, use gnulib's progname module

* fish/fish.c: Include "progname.h". (main): Call set_program_name to initialize. Don't hard-code guestfish everywhere. Use program_name. (usage): Don't spew all of --help for a mis-typed option. Split long lines. --- bootstrap | 1 + fish/Makefile.am | 3 +- fish/fish.c | 129 ++++++++++++++++++++++++++++++++++++----------------- 3 files changed, 90 insertions(+), 43 deletions(-) diff --git a/bootstrap b/bootstrap index 76e2216..3fd8811 100755 --- a/bootstrap +++ b/bootstrap @@ -56,6 +56,7 @@ gnumakefile ignore-value maintainer-makefile manywarnings +progname warnings vc-list-files ' diff --git a/fish/Makefile.am b/fish/Makefile.am index 94bf757..c8ba3ea 100644 --- a/fish/Makefile.am +++ b/fish/Makefile.am @@ -52,13 +52,14 @@ guestfish_CFLAGS = \ -I$(top_srcdir)/src -I$(top_builddir)/src \ -I$(top_srcdir)/fish -I$(top_builddir)/fish \ -DGUESTFS_DEFAULT_PATH='"$(libdir)/guestfs"' \ + -I$(srcdir)/../gnulib/lib -I../gnulib/lib \ $(WARN_CFLAGS) $(WERROR_CFLAGS) guestfish_LDADD = $(top_builddir)/src/libguestfs.la $(LIBREADLINE) # Make libguestfs use the convenience library. noinst_LTLIBRARIES = librc_protocol.la -guestfish_LDADD += librc_protocol.la +guestfish_LDADD += librc_protocol.la ../gnulib/lib/libgnu.la if HAVE_RPCGEN rc_protocol.c: rc_protocol.x diff --git a/fish/fish.c b/fish/fish.c index 3acd450..baa1b6e 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -40,6 +40,7 @@ #include <guestfs.h> #include "fish.h" +#include "progname.h" struct mp { struct mp *next; @@ -87,21 +88,25 @@ launch (guestfs_h *_g) return 0; } -static void -usage (void) +static void __attribute__((noreturn)) +usage (int status) { - fprintf (stderr, - _("guestfish: guest filesystem shell\n" - "guestfish lets you edit virtual machine filesystems\n" + if (status != EXIT_SUCCESS) + fprintf (stderr, _("Try `%s --help' for more information.\n"), + program_name); + else { + fprintf (stdout, + _("%s: guest filesystem shell\n" + "%s lets you edit virtual machine filesystems\n" "Copyright (C) 2009 Red Hat Inc.\n" "Usage:\n" - " guestfish [--options] cmd [: cmd : cmd ...]\n" - " guestfish -i libvirt-domain\n" - " guestfish -i disk-image(s)\n" + " %s [--options] cmd [: cmd : cmd ...]\n" + " %s -i libvirt-domain\n" + " %s -i disk-image(s)\n" "or for interactive use:\n" - " guestfish\n" + " %s\n" "or from a shell script:\n" - " guestfish <<EOF\n" + " %s <<EOF\n" " cmd\n" " ...\n" " EOF\n" @@ -115,24 +120,34 @@ usage (void) " --listen Listen for remote commands\n" " -m|--mount dev[:mnt] Mount dev on mnt (if omitted, /)\n" " -n|--no-sync Don't autosync\n" - " --remote[=pid] Send commands to remote guestfish\n" + " --remote[=pid] Send commands to remote %s\n" " -r|--ro Mount read-only\n" " --selinux Enable SELinux support\n" " -v|--verbose Verbose messages\n" " -x Echo each command before executing it\n" " -V|--version Display version and exit\n" - "For more information, see the manpage guestfish(1).\n")); + "For more information, see the manpage %s(1).\n"), + program_name, program_name, program_name, + program_name, program_name, program_name, + program_name, program_name, program_name); + } + exit (status); } int main (int argc, char *argv[]) { + /* Set global program name that is not polluted with libtool artifacts. */ + set_program_name (argv[0]); + + enum { HELP_OPTION = CHAR_MAX + 1 }; + static const char *options = "a:Df:h::im:nrv?Vx"; static const struct option long_options[] = { { "add", 1, 0, 'a' }, { "cmd-help", 2, 0, 'h' }, { "file", 1, 0, 'f' }, - { "help", 0, 0, '?' }, + { "help", 0, 0, HELP_OPTION }, { "inspector", 0, 0, 'i' }, { "listen", 0, 0, 0 }, { "mount", 1, 0, 'm' }, @@ -186,6 +201,11 @@ main (int argc, char *argv[]) (argv[0][0] != '/' || strstr (argv[0], "/.libs/lt-") != NULL)) guestfs_set_path (g, "appliance:" GUESTFS_DEFAULT_PATH); + /* getopt_long uses argv[0], so give it the sanitized name, too. + But only temporarily. */ + char *argv0 = argv[0]; + argv[0] = bad_cast (program_name); + for (;;) { c = getopt_long (argc, argv, options, long_options, &option_index); if (c == -1) break; @@ -197,21 +217,24 @@ main (int argc, char *argv[]) else if (strcmp (long_options[option_index].name, "remote") == 0) { if (optarg) { if (sscanf (optarg, "%d", &remote_control) != 1) { - fprintf (stderr, _("guestfish: --listen=PID: PID was not a number: %s\n"), optarg); + fprintf (stderr, _("%s: --listen=PID: PID was not a number: %s\n"), + program_name, optarg); exit (1); } } else { p = getenv ("GUESTFISH_PID"); if (!p || sscanf (p, "%d", &remote_control) != 1) { - fprintf (stderr, _("guestfish: remote: $GUESTFISH_PID must be set to the PID of the remote process\n")); + fprintf (stderr, _("%s: remote: $GUESTFISH_PID must be set" + " to the PID of the remote process\n"), + program_name); exit (1); } } } else if (strcmp (long_options[option_index].name, "selinux") == 0) { guestfs_set_selinux (g, 1); } else { - fprintf (stderr, _("guestfish: unknown long option: %s (%d)\n"), - long_options[option_index].name, option_index); + fprintf (stderr, _("%s: unknown long option: %s (%d)\n"), + program_name, long_options[option_index].name, option_index); exit (1); } break; @@ -237,7 +260,8 @@ main (int argc, char *argv[]) case 'f': if (file) { - fprintf (stderr, _("guestfish: only one -f parameter can be given\n")); + fprintf (stderr, _("%s: only one -f parameter can be given\n"), + program_name); exit (1); } file = optarg; @@ -287,24 +311,26 @@ main (int argc, char *argv[]) break; case 'V': - printf ("guestfish %s\n", PACKAGE_VERSION); + printf ("%s %s\n", program_name, PACKAGE_VERSION); exit (0); case 'x': echo_commands = 1; break; - case '?': - usage (); - exit (0); + case HELP_OPTION: + usage (0); default: - fprintf (stderr, _("guestfish: unexpected command line option 0x%x\n"), - c); - exit (1); + fprintf (stderr, _("%s: unexpected command line option 0x%x\n"), + program_name, c); + usage (1); } } + /* Restore original value. */ + argv[0] = argv0; + /* Inspector mode invalidates most of the other arguments. */ if (inspector) { char cmd[1024]; @@ -312,11 +338,15 @@ main (int argc, char *argv[]) if (drvs || mps || remote_control_listen || remote_control || guestfs_get_selinux (g)) { - fprintf (stderr, _("guestfish: cannot use -i option with -a, -m, --listen, --remote or --selinux\n")); + fprintf (stderr, _("%s: cannot use -i option with -a, -m," + " --listen, --remote or --selinux\n"), + program_name); exit (1); } if (optind >= argc) { - fprintf (stderr, _("guestfish -i requires a libvirt domain or path(s) to disk image(s)\n")); + fprintf (stderr, + _("%s: -i requires a libvirt domain or path(s) to disk image(s)\n"), + program_name); exit (1); } @@ -324,7 +354,9 @@ main (int argc, char *argv[]) while (optind < argc) { if (strlen (cmd) + strlen (argv[optind]) + strlen (argv[0]) + 60 >= sizeof cmd) { - fprintf (stderr, _("guestfish: virt-inspector command too long for fixed-size buffer\n")); + fprintf (stderr, + _("%s: virt-inspector command too long for fixed-size buffer\n"), + program_name); exit (1); } strcat (cmd, " '"); @@ -347,7 +379,7 @@ main (int argc, char *argv[]) if (verbose) fprintf (stderr, - "guestfish -i: running virt-inspector command:\n%s\n", cmd); + "%s -i: running virt-inspector command:\n%s\n", program_name, cmd); r = system (cmd); if (r == -1) { @@ -368,17 +400,23 @@ main (int argc, char *argv[]) /* Remote control? */ if (remote_control_listen && remote_control) { - fprintf (stderr, _("guestfish: cannot use --listen and --remote options at the same time\n")); + fprintf (stderr, + _("%s: cannot use --listen and --remote options at the same time\n"), + program_name); exit (1); } if (remote_control_listen) { if (optind < argc) { - fprintf (stderr, _("guestfish: extra parameters on the command line with --listen flag\n")); + fprintf (stderr, + _("%s: extra parameters on the command line with --listen flag\n"), + program_name); exit (1); } if (file) { - fprintf (stderr, _("guestfish: cannot use --listen and --file options at the same time\n")); + fprintf (stderr, + _("%s: cannot use --listen and --file options at the same time\n"), + program_name); exit (1); } rc_listen (); @@ -602,12 +640,14 @@ script (int prompt) p++; len = strcspn (p, "\""); if (p[len] == '\0') { - fprintf (stderr, _("guestfish: unterminated double quote\n")); + fprintf (stderr, _("%s: unterminated double quote\n"), program_name); if (exit_on_error) exit (1); goto next_command; } if (p[len+1] && (p[len+1] != ' ' && p[len+1] != '\t')) { - fprintf (stderr, _("guestfish: command arguments not separated by whitespace\n")); + fprintf (stderr, + _("%s: command arguments not separated by whitespace\n"), + program_name); if (exit_on_error) exit (1); goto next_command; } @@ -617,12 +657,14 @@ script (int prompt) p++; len = strcspn (p, "'"); if (p[len] == '\0') { - fprintf (stderr, _("guestfish: unterminated single quote\n")); + fprintf (stderr, _("%s: unterminated single quote\n"), program_name); if (exit_on_error) exit (1); goto next_command; } if (p[len+1] && (p[len+1] != ' ' && p[len+1] != '\t')) { - fprintf (stderr, _("guestfish: command arguments not separated by whitespace\n")); + fprintf (stderr, + _("%s: command arguments not separated by whitespace\n"), + program_name); if (exit_on_error) exit (1); goto next_command; } @@ -643,12 +685,15 @@ script (int prompt) pend++; } if (c != 0) { - fprintf (stderr, _("guestfish: unterminated \"[...]\" sequence\n")); + fprintf (stderr, + _("%s: unterminated \"[...]\" sequence\n"), program_name); if (exit_on_error) exit (1); goto next_command; } if (*pend && (*pend != ' ' && *pend != '\t')) { - fprintf (stderr, _("guestfish: command arguments not separated by whitespace\n")); + fprintf (stderr, + _("%s: command arguments not separated by whitespace\n"), + program_name); if (exit_on_error) exit (1); goto next_command; } @@ -667,8 +712,8 @@ script (int prompt) } else pend = &p[len]; } else { - fprintf (stderr, _("guestfish: internal error parsing string at '%s'\n"), - p); + fprintf (stderr, _("%s: internal error parsing string at '%s'\n"), + program_name, p); abort (); } @@ -684,7 +729,7 @@ script (int prompt) } if (i == sizeof argv / sizeof argv[0]) { - fprintf (stderr, _("guestfish: too many arguments\n")); + fprintf (stderr, _("%s: too many arguments\n"), program_name); if (exit_on_error) exit (1); goto next_command; } @@ -713,7 +758,7 @@ cmdline (char *argv[], int optind, int argc) cmd = argv[optind++]; if (strcmp (cmd, ":") == 0) { - fprintf (stderr, _("guestfish: empty command on command line\n")); + fprintf (stderr, _("%s: empty command on command line\n"), program_name); exit (1); } params = &argv[optind]; -- 1.6.4.378.g88f2f

On Mon, Aug 24, 2009 at 12:21:55PM +0200, Jim Meyering wrote:

From: Jim Meyering <meyering@redhat.com>

* fish/fish.c: ... getopt_long already does that. Instead, suggest "Try `guestfish --help' for more information." --- fish/fish.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fish/fish.c b/fish/fish.c index 8ea1c49..964c906 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -320,8 +320,6 @@ main (int argc, char *argv[]) usage (0);

default: - fprintf (stderr, _("%s: unexpected command line option 0x%x\n"), - program_name, c); usage (1); } } -- 1.6.4.378.g88f2f

If the previous patch is fixed, then this looks good - ACK. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v

On Mon, Aug 24, 2009 at 12:21:56PM +0200, Jim Meyering wrote:

From: Jim Meyering <meyering@redhat.com>

Use gnulib's closeout module to ensure any failure to write to stdout is detected and reported. * fish/fish.c: Include "closeout.h". (main): Call atexit (close_stdout); * bootstrap (modules): Add closeout.

(emit print_*_indent): Likewise, s/int/unsigned int/ --- bootstrap | 1 + fish/fish.c | 3 +++ 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/bootstrap b/bootstrap index 3fd8811..f5f8aaf 100755 --- a/bootstrap +++ b/bootstrap @@ -50,6 +50,7 @@ gnulib_tool=$GNULIB_SRCDIR/gnulib-tool (cd daemon && mkdir -p tests lib && ../$gnulib_tool --update)

modules=' +closeout gitlog-to-changelog gnu-make gnumakefile diff --git a/fish/fish.c b/fish/fish.c index 964c906..1790c1a 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -40,6 +40,7 @@ #include <guestfs.h>

#include "fish.h" +#include "closeout.h" #include "progname.h"

struct mp { @@ -143,6 +144,8 @@ main (int argc, char *argv[]) /* getopt_long uses argv[0], so give it the sanitized name, too. */ argv[0] = bad_cast (program_name);

+ atexit (close_stdout); + enum { HELP_OPTION = CHAR_MAX + 1 };

static const char *options = "a:Df:h::im:nrv?Vx"; -- 1.6.4.378.g88f2f

ACK. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 75 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora

On Mon, Aug 24, 2009 at 12:21:57PM +0200, Jim Meyering wrote:

From: Jim Meyering <meyering@redhat.com>

Now that we're using gnulib in earnest, any manual definition would provoke a redefinition warning. * fish/fish.c (_GNU_SOURCE): Don't define. * fish/destpaths.c (_GNU_SOURCE): Likewise. * src/guestfs.c (_GNU_SOURCE): Likewise. * bootstrap (modules): Add asprintf, strchrnul, strerror, strndup and vasprintf. * fish/fish.c (main): Set argv[0] to sanitized program_name, so functions like getopt_long that use argv[0] use the clean name. --- bootstrap | 7 ++++++- fish/destpaths.c | 2 -- fish/fish.c | 2 -- src/guestfs.c | 1 - 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/bootstrap b/bootstrap index f5f8aaf..2eba6a7 100755 --- a/bootstrap +++ b/bootstrap @@ -50,6 +50,7 @@ gnulib_tool=$GNULIB_SRCDIR/gnulib-tool (cd daemon && mkdir -p tests lib && ../$gnulib_tool --update)

modules=' +asprintf closeout gitlog-to-changelog gnu-make @@ -58,8 +59,12 @@ ignore-value maintainer-makefile manywarnings progname -warnings +strchrnul +strerror +strndup +vasprintf vc-list-files +warnings '

$gnulib_tool \ diff --git a/fish/destpaths.c b/fish/destpaths.c index 275db49..1e42ae8 100644 --- a/fish/destpaths.c +++ b/fish/destpaths.c @@ -18,8 +18,6 @@

#include <config.h>

-#define _GNU_SOURCE // for strndup, asprintf - #include <stdio.h> #include <stdlib.h> #include <stddef.h> diff --git a/fish/fish.c b/fish/fish.c index 1790c1a..9f0f8df 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -18,8 +18,6 @@

#include <config.h>

-#define _GNU_SOURCE // for strchrnul - #include <stdio.h> #include <stdlib.h> #include <string.h> diff --git a/src/guestfs.c b/src/guestfs.c index 04bd4e8..4a9f11a 100644 --- a/src/guestfs.c +++ b/src/guestfs.c @@ -19,7 +19,6 @@ #include <config.h>

#define _BSD_SOURCE /* for mkdtemp, usleep */ -#define _GNU_SOURCE /* for vasprintf, GNU strerror_r, strchrnul */

#include <stdio.h> #include <stdlib.h> -- 1.6.4.378.g88f2f

_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs

Yup, ACK. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v