Re: coreutils 6.6 c89 build failures

[Paul Eggert]

Matthew Woehlke <address@hidden> writes:

> Is it really so painful to support c89
> when the needed changes are so trivial

But they make the code uglier and harder to maintain.

However, I think most of those problems can be solved by rewriting the
code in a not-so-trivial way.  Here's my attempt to do it.  It does
shrink the overall source code length by 24 lines (including the
change to Makefile.maint and README, but not counting the removal of
src/c99-to-c89.diff), so one could argue that this patch maintains
readability.  (Also, it shrinks the executable text size of 'rm' by a
whopping 96 bytes (0.25%) on my host (x86, Debian stable, GCC 4.1.1,
compiled with -O).  :-)

It's really up to Jim, though.  He might not want to accept a patch
that merely shuffles code around without fixing any real bugs, as the
patch itself may introduce a bug.

(Manually remove src/c99-to-c89.diff after applying this patch.)

diff --git a/ChangeLog b/ChangeLog
index ba53ad1..6c49276 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,27 @@
+2006-11-24  Paul Eggert  <address@hidden>
+
+       Port to C89 without the need for c99-to-c89.diff, while trying
+       to retain the readability of C99 as much as possible.
+       * Makefile.maint (patch-check): Remove; all uses removed.
+       * README: Remove implicit build requirement of C99-style
+       declarations after statements.
+       * src/Makefile.am (EXTRA_DIST): Remove c99-to-c89.diff.
+       * src/c99-to-c89.diff: Remove.
+       * src/remove.c (close_preserve_errno): Remove.
+       (cache_stat_init): Return its argument, for convenience.
+       (pop_dir, AD_stack_pop): Put assertions after declarations.
+       (AD_pop_and_chdir): Return prev_dir rather than storing through
+       a pointer argument.  All uses changed.
+       (AD_insure_initialized): New function.
+       (AD_mark_helper): Use it, to avoid the need for declaration
+       after statement.
+       (fd_to_subdir_p, remove_dir, rm_1, rm): Rewrite to avoid the need for
+       declaration after statement.
+       * src/rm.c (rm_yesno): New function.
+       (main): Use it, to avoid the need for declaration after statement.
+       * src/shred.c (dopass): Use 'verify' at the start of a block,
+       for C89 compatibility.
+
 2006-11-23  Jim Meyering  <address@hidden>
 
        * announce-gen: Remove file.  It's moving to gnulib.
diff --git a/Makefile.maint b/Makefile.maint
index 8fa6f70..d5e8c60 100644
--- a/Makefile.maint
+++ b/Makefile.maint
@@ -73,7 +73,7 @@ # Run `changelog-check' last, as previou
 # new ChangeLog entries.
 local-checks-available = \
   po-check copyright-check writable-files m4-check author_mark_check \
-  changelog-check patch-check strftime-check $(syntax-check-rules) \
+  changelog-check strftime-check $(syntax-check-rules) \
   makefile_path_separator_check \
   makefile-check check-AUTHORS
 .PHONY: $(local-checks-available)
@@ -324,19 +324,6 @@ sc_useless_cpp_parens:
          { echo '$(ME): found useless parentheses in cpp directive'    \
                1>&2; exit 1; } || :
 
-# Ensure that the c99-to-c89 patch applies cleanly.
-patch-check:
-       rm -rf src-c89 address@hidden address@hidden
-       cp -a src src-c89
-       (cd src-c89; patch -V never --fuzz=0) < src/c99-to-c89.diff \
-         > address@hidden 2>&1
-       grep -v '^patching file ' address@hidden > address@hidden || :
-       if test "$${REGEN_PATCH+set}" = set; then \
-         diff -upr src src-c89 > new-diff || : ; fi
-       fail=0; test -s address@hidden && fail=1 || : ; \
-       rm -rf src-c89 address@hidden address@hidden; \
-       test $$fail = 0
-
 # Ensure that date's --help output stays in sync with the info
 # documentation for GNU strftime.  The only exception is %N,
 # which date accepts but GNU strftime does not.
diff --git a/README b/README
index cff225d..e4aa043 100644
--- a/README
+++ b/README
@@ -61,22 +61,6 @@ way Sun implemented link(2) and chmod(2)
 
 
 ***********************
-Pre-C99 build failure
------------------------
-
-There is a new, implicit build requirement:
-To build the coreutils from source, you should have a C99-conforming
-compiler, due to the use of declarations after non-declaration statements
-in several files in src/.  There is code in configure to find and, if
-possible, enable an appropriate compiler.  However, if configure doesn't
-find a C99 compiler, it continues nonetheless, and your build will fail.
-If that happens, simply apply the included patch using the following
-command, and then run make again:
-
-  cd src && patch < c99-to-c89.diff
-
-
-***********************
 HPUX 11.x build failure
 -----------------------
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 8c39ee0..0b17cd7 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -47,7 +47,7 @@ noinst_HEADERS = \
   wheel.h
 
 EXTRA_DIST = dcgen dircolors.hin tac-pipe.c \
-  groups.sh wheel-gen.pl extract-magic c99-to-c89.diff
+  groups.sh wheel-gen.pl extract-magic
 BUILT_SOURCES =
 CLEANFILES = $(SCRIPTS) su
 
diff --git a/src/remove.c b/src/remove.c
index add85dd..e621492 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -148,16 +148,6 @@ struct dirstack_state
 };
 typedef struct dirstack_state Dirstack_state;
 
-/* Just like close(fd), but don't modify errno. */
-static inline int
-close_preserve_errno (int fd)
-{
-  int saved_errno = errno;
-  int result = close (fd);
-  errno = saved_errno;
-  return result;
-}
-
 /* Like fstatat, but cache the result.  If ST->st_size is -1, the
    status has not been gotten yet.  If less than -1, fstatat failed
    with errno == -1 - ST->st_size.  Otherwise, the status has already
@@ -173,11 +163,12 @@ cache_fstatat (int fd, char const *file,
   return -1;
 }
 
-/* Initialize a fstatat cache *ST.  */
-static inline void
+/* Initialize a fstatat cache *ST.  Return ST for convenience.  */
+static inline struct stat *
 cache_stat_init (struct stat *st)
 {
   st->st_size = -1;
+  return st;
 }
 
 /* Return true if *ST has been statted.  */
@@ -245,13 +236,12 @@ pop_dir (Dirstack_state *ds)
 {
   size_t n_lengths = obstack_object_size (&ds->len_stack) / sizeof (size_t);
   size_t *length = obstack_base (&ds->len_stack);
-
-  assert (n_lengths > 0);
   size_t top_len = length[n_lengths - 1];
-  assert (top_len >= 2);
+
+  assert (0 < n_lengths
+         && 2 <= top_len && top_len <= obstack_object_size (&ds->dir_stack));
 
   /* Pop the specified length of file name.  */
-  assert (obstack_object_size (&ds->dir_stack) >= top_len);
   obstack_blank (&ds->dir_stack, -top_len);
 
   /* Pop the length stack, too.  */
@@ -379,10 +369,9 @@ AD_stack_top (Dirstack_state const *ds)
 static void
 AD_stack_pop (Dirstack_state *ds)
 {
-  assert (0 < AD_stack_height (ds));
-
   /* operate on Active_dir.  pop and free top entry */
   struct AD_ent *top = AD_stack_top (ds);
+  assert (0 < AD_stack_height (ds));
   if (top->unremovable)
     hash_free (top->unremovable);
   obstack_blank (&ds->Active_dir, -(int) sizeof (struct AD_ent));
@@ -437,11 +426,11 @@ ds_free (Dirstack_state *ds)
    that the post-chdir dev/ino numbers for `.' match the saved ones.
    If any system call fails or if dev/ino don't match then give a
    diagnostic and longjump out.
-   Set *PREV_DIR to the name (in malloc'd storage) of the
+   Return the name (in malloc'd storage) of the
    directory (usually now empty) from which we're coming, and which
    corresponds to the input value of *DIRP.  */
-static void
-AD_pop_and_chdir (DIR **dirp, Dirstack_state *ds, char **prev_dir)
+static char *
+AD_pop_and_chdir (DIR **dirp, Dirstack_state *ds)
 {
   struct AD_ent *leaf_dir_ent = AD_stack_top(ds);
   struct dev_ino leaf_dev_ino = leaf_dir_ent->dev_ino;
@@ -450,7 +439,7 @@ AD_pop_and_chdir (DIR **dirp, Dirstack_s
 
   /* Get the name of the current (but soon to be `previous') directory
      from the top of the stack.  */
-  *prev_dir = top_dir (ds);
+  char *prev_dir = top_dir (ds);
 
   AD_stack_pop (ds);
   pop_dir (ds);
@@ -478,7 +467,7 @@ AD_pop_and_chdir (DIR **dirp, Dirstack_s
       if (closedir (*dirp) != 0)
        {
          error (0, errno, _("FATAL: failed to close directory %s"),
-                quote (full_filename (*prev_dir)));
+                quote (full_filename (prev_dir)));
          goto next_cmdline_arg;
        }
 
@@ -491,7 +480,7 @@ AD_pop_and_chdir (DIR **dirp, Dirstack_s
       if (fd < 0)
        {
          error (0, errno, _("FATAL: cannot open .. from %s"),
-                quote (full_filename (*prev_dir)));
+                quote (full_filename (prev_dir)));
          goto next_cmdline_arg;
        }
 
@@ -521,7 +510,7 @@ AD_pop_and_chdir (DIR **dirp, Dirstack_s
          close (fd);
 
        next_cmdline_arg:;
-         free (*prev_dir);
+         free (prev_dir);
          longjmp (ds->current_arg_jumpbuf, 1);
        }
     }
@@ -530,17 +519,18 @@ AD_pop_and_chdir (DIR **dirp, Dirstack_s
       if (closedir (*dirp) != 0)
        {
          error (0, errno, _("FATAL: failed to close directory %s"),
-                quote (full_filename (*prev_dir)));
+                quote (full_filename (prev_dir)));
          goto next_cmdline_arg;
        }
       *dirp = NULL;
     }
+
+  return prev_dir;
 }
 
-/* Initialize *HT if it is NULL.
-   Insert FILENAME into HT.  */
-static void
-AD_mark_helper (Hash_table **ht, char *filename)
+/* Initialize *HT if it is NULL.  Return *HT.  */
+static Hash_table *
+AD_insure_initialized (Hash_table **ht)
 {
   if (*ht == NULL)
     {
@@ -549,7 +539,16 @@ AD_mark_helper (Hash_table **ht, char *f
       if (*ht == NULL)
        xalloc_die ();
     }
-  void *ent = hash_insert (*ht, filename);
+
+  return *ht;
+}
+
+/* Initialize *HT if it is NULL.
+   Insert FILENAME into HT.  */
+static void
+AD_mark_helper (Hash_table **ht, char *filename)
+{
+  void *ent = hash_insert (AD_insure_initialized (ht), filename);
   if (ent == NULL)
     xalloc_die ();
   else
@@ -1111,32 +1110,28 @@ fd_to_subdirp (int fd_cwd, char const *f
 {
   int open_flags = O_RDONLY | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK;
   int fd_sub = openat_permissive (fd_cwd, f, open_flags, 0, cwd_errno);
+  int saved_errno;
 
   /* Record dev/ino of F.  We may compare them against saved values
      to thwart any attempt to subvert the traversal.  They are also used
      to detect directory cycles.  */
-  if (fd_sub < 0 || fstat (fd_sub, subdir_sb) != 0)
-    {
-      if (0 <= fd_sub)
-       close_preserve_errno (fd_sub);
-      return NULL;
-    }
-
-  if (! S_ISDIR (subdir_sb->st_mode))
-    {
-      errno = prev_errno ? prev_errno : ENOTDIR;
-      close_preserve_errno (fd_sub);
-      return NULL;
-    }
-
-  DIR *subdir_dirp = fdopendir (fd_sub);
-  if (subdir_dirp == NULL)
+  if (fd_sub < 0)
+    return NULL;
+  else if (fstat (fd_sub, subdir_sb) != 0)
+    saved_errno = errno;
+  else if (S_ISDIR (subdir_sb->st_mode))
     {
-      close_preserve_errno (fd_sub);
-      return NULL;
+      DIR *subdir_dirp = fdopendir (fd_sub);
+      if (subdir_dirp)
+       return subdir_dirp;
+      saved_errno = errno;
     }
+  else
+    saved_errno = (prev_errno ? prev_errno : ENOTDIR);
 
-  return subdir_dirp;
+  close (fd_sub);
+  errno = saved_errno;
+  return NULL;
 }
 
 /* Remove entries in the directory open on DIRP
@@ -1388,9 +1383,7 @@ remove_dir (int fd_cwd, Dirstack_state *
       {
        /* The name of the directory that we have just processed,
           nominally removing all of its contents.  */
-       char *empty_dir;
-
-       AD_pop_and_chdir (&dirp, ds, &empty_dir);
+       char *empty_dir = AD_pop_and_chdir (&dirp, ds);
        int fd = (dirp != NULL ? dirfd (dirp) : AT_FDCWD);
        assert (dirp != NULL || AD_stack_height (ds) == 1);
 
@@ -1403,8 +1396,8 @@ remove_dir (int fd_cwd, Dirstack_state *
               But that's no big deal since we're interactive.  */
            struct stat empty_st;
            Ternary is_empty;
-           cache_stat_init (&empty_st);
-           enum RM_status s = prompt (fd, ds, empty_dir, &empty_st, x,
+           enum RM_status s = prompt (fd, ds, empty_dir,
+                                      cache_stat_init (&empty_st), x,
                                       PA_REMOVE_DIR, &is_empty);
 
            if (s != RM_OK)
@@ -1468,46 +1461,50 @@ rm_1 (Dirstack_state *ds, char const *fi
             quote_n (0, base), quote_n (1, filename));
       return RM_ERROR;
     }
-
-  struct stat st;
-  cache_stat_init (&st);
-  if (x->root_dev_ino)
+  else
     {
-      if (cache_fstatat (AT_FDCWD, filename, &st, AT_SYMLINK_NOFOLLOW) != 0)
-       {
-         if (ignorable_missing (x, errno))
-           return RM_OK;
-         error (0, errno, _("cannot remove %s"), quote (filename));
-         return RM_ERROR;
-       }
-      if (SAME_INODE (st, *(x->root_dev_ino)))
+      enum RM_status status;
+      struct stat st;
+      cache_stat_init (&st);
+      cycle_check_init (&ds->cycle_check_state);
+      if (x->root_dev_ino)
        {
-         error (0, 0, _("cannot remove root directory %s"), quote (filename));
-         return RM_ERROR;
+         if (cache_fstatat (AT_FDCWD, filename, &st, AT_SYMLINK_NOFOLLOW)
+             != 0)
+           {
+             if (ignorable_missing (x, errno))
+               return RM_OK;
+             error (0, errno, _("cannot remove %s"), quote (filename));
+             return RM_ERROR;
+           }
+         if (SAME_INODE (st, *(x->root_dev_ino)))
+           {
+             error (0, 0, _("cannot remove root directory %s"),
+                    quote (filename));
+             return RM_ERROR;
+           }
        }
-    }
-
-  AD_push_initial (ds);
-  AD_INIT_OTHER_MEMBERS ();
 
-  int fd_cwd = AT_FDCWD;
-  enum RM_status status = remove_entry (fd_cwd, ds, filename, &st, x, NULL);
-  if (status == RM_NONEMPTY_DIR)
-    {
-      /* In the event that remove_dir->remove_cwd_entries detects
-        a directory cycle, arrange to fail, give up on this FILE, but
-        continue on with any other arguments.  */
-      if (setjmp (ds->current_arg_jumpbuf))
-       status = RM_ERROR;
-      else
-       status = remove_dir (fd_cwd, ds, filename, &st, x, cwd_errno);
+      AD_push_initial (ds);
+      AD_INIT_OTHER_MEMBERS ();
 
-      AD_stack_clear (ds);
-    }
+      status = remove_entry (AT_FDCWD, ds, filename, &st, x, NULL);
+      if (status == RM_NONEMPTY_DIR)
+       {
+         /* In the event that remove_dir->remove_cwd_entries detects
+            a directory cycle, arrange to fail, give up on this FILE, but
+            continue on with any other arguments.  */
+         if (setjmp (ds->current_arg_jumpbuf))
+           status = RM_ERROR;
+         else
+           status = remove_dir (AT_FDCWD, ds, filename, &st, x, cwd_errno);
 
-  ds_clear (ds);
+         AD_stack_clear (ds);
+       }
 
-  return status;
+      ds_clear (ds);
+      return status;
+    }
 }
 
 /* Remove all files and/or directories specified by N_FILES and FILE.
@@ -1526,13 +1523,13 @@ rm (size_t n_files, char const *const *f
        {
          error (0, 0, _("cannot remove relative-named %s"), quote (file[i]));
          status = RM_ERROR;
-         continue;
        }
-
-      cycle_check_init (&ds->cycle_check_state);
-      enum RM_status s = rm_1 (ds, file[i], x, &cwd_errno);
-      assert (VALID_STATUS (s));
-      UPDATE_STATUS (status, s);
+      else
+       {
+         enum RM_status s = rm_1 (ds, file[i], x, &cwd_errno);
+         assert (VALID_STATUS (s));
+         UPDATE_STATUS (status, s);
+       }
     }
 
   if (x->require_restore_cwd && cwd_errno)
diff --git a/src/rm.c b/src/rm.c
index 0c93a04..4881472 100644
--- a/src/rm.c
+++ b/src/rm.c
@@ -225,6 +225,19 @@ rm_option_init (struct rm_options *x)
   x->require_restore_cwd = false;
 }
 
+/* Ask the user whether to remove all arguments, and return true
+   if the user said yes.  */
+static bool
+rm_yesno (struct rm_options *x)
+{
+  fprintf (stderr,
+          (x->recursive
+           ? _("%s: remove all arguments recursively? ")
+           : _("%s: remove all arguments? ")),
+          program_name);
+  return yesno ();
+}
+
 int
 main (int argc, char **argv)
 {
@@ -359,18 +372,13 @@ main (int argc, char **argv)
     size_t n_files = argc - optind;
     char const *const *file = (char const *const *) argv + optind;
 
-    if (prompt_once && (x.recursive || 3 < n_files))
+    if (prompt_once && (x.recursive || 3 < n_files) && !rm_yesno (&x))
+      exit (EXIT_SUCCESS);
+    else
       {
-       fprintf (stderr,
-                (x.recursive
-                 ? _("%s: remove all arguments recursively? ")
-                 : _("%s: remove all arguments? ")),
-                program_name);
-       if (!yesno ())
-         exit (EXIT_SUCCESS);
+       enum RM_status status = rm (n_files, file, &x);
+       assert (VALID_STATUS (status));
+       exit (status == RM_ERROR ? EXIT_FAILURE : EXIT_SUCCESS);
       }
-    enum RM_status status = rm (n_files, file, &x);
-    assert (VALID_STATUS (status));
-    exit (status == RM_ERROR ? EXIT_FAILURE : EXIT_SUCCESS);
   }
 }
diff --git a/src/shred.c b/src/shred.c
index 4c1819c..5e3f62e 100644
--- a/src/shred.c
+++ b/src/shred.c
@@ -460,9 +460,9 @@ dopass (int fd, char const *qname, off_t
                     out.  Thus, it shouldn't give up on bad blocks.  This
                     code works because lim is always a multiple of
                     SECTOR_SIZE, except at the end.  */
-                 verify (sizeof r % SECTOR_SIZE == 0);
                  if (errnum == EIO && 0 <= size && (soff | SECTOR_MASK) < lim)
                    {
+                     verify (sizeof r % SECTOR_SIZE == 0);
                      size_t soff1 = (soff | SECTOR_MASK) + 1;
                      if (lseek (fd, offset + soff1, SEEK_SET) != -1)
                        {