Libguestfs - Aug 2009 - [0/5] guestfish: detect stdout-write failure

Nearly any program that writes to standard output can
benefit from this sort of fix.

Without it, running e.g., ./guestfish --version > /dev/full
would exit successfully, even though it got ENOSPC
when writing to the full device.  That means regular
output redirected to a file on a full partition may also
fail to be written, and the error ignored.

  Before:
      $ guestfish --version > /dev/full
  After:
      $ ./guestfish --version > /dev/full
      /w/co/libguestfs/fish/.libs/lt-guestfish: write error: No space left on
device

Obviously, including the absolute program name in the diagnostic is a bug.
It should be "guestfish", and there's already code to perform that
sanitization in the patch below.  It fixes everything except the glibc
global variable, program_invocation_name, used by error, called by
atexit+close_stdout.  I'll fix that via gnulib's progname module later
today or tomorrow.  Note that all other diagnostics (e.g., below) do
use the sanitized name.

The second patch is to make guestfish write --help output
to stdout, not stderr, per convention.  Also, since --help
output is pretty long, I find it clearer (e.g., when I misspell
an option) to write a brief diagnostic, like

    $ ./guestfish --fjd
    guestfish: unrecognized option '--fjd'
    Try `guestfish --help' for more information.
    [Exit 1]

The third patch is another small improvement: let getopt diagnose failures.
Before, using --mount with no option-argument would print a
misleading diagnostic:

    $ ./guestfish -m
    guestfish: unexpected command line option 0x3f
    ...all --help output...
    [Exit 1]

Now it prints this:

    $ ./guestfish -m
    guestfish: option requires an argument -- 'm'
    Try `guestfish --help' for more information.
    [Exit 1]

------------------------------
The fourth patch makes the Before/After change demonstrated above.

The last patch avoids new cpp macro redefinition warnings.
Now that we're using a few more gnulib modules, they
end up defining _GNU_SOURCE via a definition in config.h,
so the explicit definitons in various *.c files evoke warnings.


$ wc -l 00*
   45 0001-build-avoid-some-autoconf-warnings.patch
  353 0002-guestfish-write-help-to-stdout-use-gnulib-s-progname.patch
   27 0003-guestfish-don-t-try-to-diagnose-getopt-failure.patch
   53 0004-guestfish-diagnose-stdout-write-failure.patch
   88 0005-build-don-t-define-_GNU_SOURCE-manually.patch
  566 total

$ diffstat 00*
 b/bootstrap        |    1
 b/configure.ac     |   11 ++--
 b/fish/Makefile.am |    3 -
 b/fish/destpaths.c |    2
 b/fish/fish.c      |  125 ++++++++++++++++++++++++++++++++++-------------------
 b/src/guestfs.c    |    2
 bootstrap          |    8 ++-
 fish/fish.c        |    9 +--
 8 files changed, 100 insertions(+), 61 deletions(-)

From: Jim Meyering <meyering at redhat.com>

* configure.ac: Move gl_EARLY and gl_INIT to be earlier.
---
 configure.ac |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index 5a3379b..2284dac 100644
--- a/configure.ac
+++ b/configure.ac
@@ -31,22 +31,22 @@ AM_SILENT_RULES([yes]) # make --enable-silent-rules the
default.

 AC_CONFIG_MACRO_DIR([m4])

-AC_PROG_LIBTOOL
-
 dnl Split up the version string.
 AC_DEFINE([PACKAGE_VERSION_MAJOR],[libguestfs_major],[Major version number])
 AC_DEFINE([PACKAGE_VERSION_MINOR],[libguestfs_minor],[Minor version number])
 AC_DEFINE([PACKAGE_VERSION_RELEASE],[libguestfs_release],[Release number])
 AC_DEFINE([PACKAGE_VERSION_EXTRA],["libguestfs_extra"],[Extra version
string])

+gl_EARLY
+gl_INIT
+
+AC_PROG_LIBTOOL
+
 dnl Check for basic C environment.
 AC_PROG_CC_STDC
 AC_PROG_INSTALL
 AC_PROG_CPP

-gl_EARLY
-gl_INIT
-
 AC_ARG_ENABLE([gcc-warnings],
   [AS_HELP_STRING([--enable-gcc-warnings],
                   [turn on lots of GCC warnings (for developers)])],
-- 
1.6.4.378.g88f2f

From: Jim Meyering <meyering at redhat.com>

* 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      |  124 +++++++++++++++++++++++++++++++++++------------------
 3 files changed, 85 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..8ea1c49 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,37 @@ 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]);
+
+  /* getopt_long uses argv[0], so give it the sanitized name, too.  */
+  argv[0] = bad_cast (program_name);
+
+  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' },
@@ -197,21 +215,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 +258,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 +309,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,11 +333,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 +349,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 +374,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 +395,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 +635,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 +652,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 +680,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 +707,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 +724,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 +753,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

From: Jim Meyering <meyering at 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

From: Jim Meyering <meyering at 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

From: Jim Meyering <meyering at 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