Re: bug in yesterday's changes to install.c

[Paul Eggert]

John Paul Wallington <address@hidden> writes:

> After Paul's int to bool cleanup (revision 1.164 of install.c) the
> testcase /tests/install/create-leading fails.

Sorry about that: that problem was introduced because I've been trying
to split up the int cleanup patches instead of installing one huge
one.  I missed the dependency of the earlier patch on the makepath
changes.  I installed the following further patch, which should fix
things.

This patch fixes a portability problem that I think is only
theoretical: the current makepath/mkdir code assumes that all mode_t
values fit into int.  POSIX doesn't guarantee this property but I
don't know of any platform where it's false.

Other than that, this is just a code cleanup.

2004-07-30  Paul Eggert  <address@hidden>

        * lib/makepath.h: Include <stdbool.h>.
        (make_path, make_dir): Use bool, not int, since we're not setting errno.
        Use mode_t for modes, not int.  All uses changed.
        * lib/makepath.c: Likewise.
        (errno): Remove decl; no longer needed since we assume C89.
        * src/mkdir.c (create_parents, main): Use bool when appropriate.
        (main): Use EXIT_SUCCESS/EXIT_FAILURE instead of 0/1.

Index: lib/makepath.h
===================================================================
RCS file: /home/eggert/coreutils/cu/lib/makepath.h,v
retrieving revision 1.9
diff -p -u -r1.9 makepath.h
--- lib/makepath.h      10 Sep 2003 09:33:02 -0000      1.9
+++ lib/makepath.h      30 Jul 2004 20:18:51 -0000
@@ -1,7 +1,7 @@
 /* makepath.c -- Ensure that a directory path exists.
 
-   Copyright (C) 1994, 1995, 1996, 1997, 2000, 2003 Free Software
-   Foundation, Inc.
+   Copyright (C) 1994, 1995, 1996, 1997, 2000, 2003, 2004 Free
+   Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -19,17 +19,18 @@
 
 /* Written by David MacKenzie <address@hidden> and Jim Meyering.  */
 
+#include <stdbool.h>
 #include <sys/types.h>
 
-int make_path (const char *_argpath,
-              int _mode,
-              int _parent_mode,
-              uid_t _owner,
-              gid_t _group,
-              int _preserve_existing,
-              const char *_verbose_fmt_string);
-
-int make_dir (const char *dir,
-             const char *dirpath,
-             mode_t mode,
-             int *created_dir_p);
+bool make_path (const char *argpath,
+               mode_t mode,
+               mode_t parent_mode,
+               uid_t owner,
+               gid_t group,
+               bool preserve_existing,
+               const char *verbose_fmt_string);
+
+bool make_dir (const char *dir,
+              const char *dirpath,
+              mode_t mode,
+              bool *created_dir_p);
Index: lib/makepath.c
===================================================================
RCS file: /home/eggert/coreutils/cu/lib/makepath.c,v
retrieving revision 1.59
diff -p -u -r1.59 makepath.c
--- lib/makepath.c      19 Jun 2004 12:26:53 -0000      1.59
+++ lib/makepath.c      17 Jul 2004 03:39:10 -0000
@@ -35,13 +35,7 @@
 #endif
 
 #include <stdlib.h>
-
 #include <errno.h>
-
-#ifndef errno
-extern int errno;
-#endif
-
 #include <string.h>
 
 #include "gettext.h"
@@ -85,18 +79,19 @@ extern int errno;
   while (0)
 
 /* Attempt to create directory DIR (aka DIRPATH) with the specified MODE.
-   If CREATED_DIR_P is non-NULL, set *CREATED_DIR_P to non-zero if this
-   function creates DIR and to zero otherwise.  Give a diagnostic and
-   return non-zero if DIR cannot be created or cannot be determined to
+   If CREATED_DIR_P is non-NULL, set *CREATED_DIR_P if this
+   function creates DIR and clear it otherwise.  Give a diagnostic and
+   return false if DIR cannot be created or cannot be determined to
    exist already.  Use DIRPATH in any diagnostic, not DIR.
-   Note that if DIR already exists, this function returns zero
-   (indicating success) and sets *CREATED_DIR_P to zero.  */
+   Note that if DIR already exists, this function returns true
+   (indicating success) and clears *CREATED_DIR_P.  */
 
-int
-make_dir (const char *dir, const char *dirpath, mode_t mode, int 
*created_dir_p)
+bool
+make_dir (const char *dir, const char *dirpath, mode_t mode,
+         bool *created_dir_p)
 {
-  int fail = 0;
-  int created_dir;
+  bool ok = true;
+  bool created_dir;
 
   created_dir = (mkdir (dir, mode) == 0);
 
@@ -117,12 +112,12 @@ make_dir (const char *dir, const char *d
        {
          error (0, saved_errno, _("cannot create directory %s"),
                 quote (dirpath));
-         fail = 1;
+         ok = false;
        }
       else if (!S_ISDIR (stats.st_mode))
        {
          error (0, 0, _("%s exists but is not a directory"), quote (dirpath));
-         fail = 1;
+         ok = false;
        }
       else
        {
@@ -133,7 +128,7 @@ make_dir (const char *dir, const char *d
   if (created_dir_p)
     *created_dir_p = created_dir;
 
-  return fail;
+  return ok;
 }
 
 /* Ensure that the directory ARGPATH exists.
@@ -147,36 +142,36 @@ make_dir (const char *dir, const char *d
    If VERBOSE_FMT_STRING is nonzero, use it as a printf format
    string for printing a message after successfully making a directory,
    with the name of the directory that was just made as an argument.
-   If PRESERVE_EXISTING is non-zero and ARGPATH is an existing directory,
+   If PRESERVE_EXISTING is true and ARGPATH is an existing directory,
    then do not attempt to set its permissions and ownership.
 
-   Return 0 if ARGPATH exists as a directory with the proper
-   ownership and permissions when done, otherwise 1.  */
+   Return true iff ARGPATH exists as a directory with the proper
+   ownership and permissions when done.  */
 
-int
+bool
 make_path (const char *argpath,
-          int mode,
-          int parent_mode,
+          mode_t mode,
+          mode_t parent_mode,
           uid_t owner,
           gid_t group,
-          int preserve_existing,
+          bool preserve_existing,
           const char *verbose_fmt_string)
 {
   struct stat stats;
-  int retval = 0;
+  bool retval = true;
 
   if (stat (argpath, &stats))
     {
       char *slash;
-      int tmp_mode;            /* Initial perms for leading dirs.  */
-      int re_protect;          /* Should leading dirs be unwritable? */
+      mode_t tmp_mode;         /* Initial perms for leading dirs.  */
+      bool re_protect;         /* Should leading dirs be unwritable? */
       struct ptr_list
       {
        char *dirname_end;
        struct ptr_list *next;
       };
       struct ptr_list *p, *leading_dirs = NULL;
-      int do_chdir;            /* Whether to chdir before each mkdir.  */
+      bool do_chdir;           /* Whether to chdir before each mkdir.  */
       struct saved_cwd cwd;
       char *basename_dir;
       char *dirpath;
@@ -197,23 +192,23 @@ make_path (const char *argpath,
              && (parent_mode & (S_ISUID | S_ISGID | S_ISVTX)) != 0))
        {
          tmp_mode = S_IRWXU;
-         re_protect = 1;
+         re_protect = true;
        }
       else
        {
          tmp_mode = parent_mode;
-         re_protect = 0;
+         re_protect = false;
        }
 
       /* If we can record the current working directory, we may be able
         to do the chdir optimization.  */
-      do_chdir = !save_cwd (&cwd);
+      do_chdir = (save_cwd (&cwd) == 0);
 
       /* If we've saved the cwd and DIRPATH is an absolute pathname,
         we must chdir to `/' in order to enable the chdir optimization.
          So if chdir ("/") fails, turn off the optimization.  */
       if (do_chdir && *dirpath == '/' && chdir ("/") < 0)
-       do_chdir = 0;
+       do_chdir = false;
 
       slash = dirpath;
 
@@ -223,8 +218,7 @@ make_path (const char *argpath,
 
       while (1)
        {
-         int newly_created_dir;
-         int fail;
+         bool newly_created_dir;
 
          /* slash points to the leftmost unprocessed component of dirpath.  */
          basename_dir = slash;
@@ -239,11 +233,10 @@ make_path (const char *argpath,
            basename_dir = dirpath;
 
          *slash = '\0';
-         fail = make_dir (basename_dir, dirpath, tmp_mode, &newly_created_dir);
-         if (fail)
+         if (! make_dir (basename_dir, dirpath, tmp_mode, &newly_created_dir))
            {
              CLEANUP;
-             return 1;
+             return false;
            }
 
          if (newly_created_dir)
@@ -261,7 +254,7 @@ make_path (const char *argpath,
                  error (0, errno, _("cannot change owner and/or group of %s"),
                         quote (dirpath));
                  CLEANUP;
-                 return 1;
+                 return false;
                }
 
              if (re_protect)
@@ -283,7 +276,7 @@ make_path (const char *argpath,
              error (0, errno, _("cannot chdir to directory %s"),
                     quote (dirpath));
              CLEANUP;
-             return 1;
+             return false;
            }
 
          *slash++ = '/';
@@ -303,10 +296,10 @@ make_path (const char *argpath,
       /* We're done making leading directories.
         Create the final component of the path.  */
 
-      if (make_dir (basename_dir, dirpath, mode, NULL))
+      if (! make_dir (basename_dir, dirpath, mode, NULL))
        {
          CLEANUP;
-         return 1;
+         return false;
        }
 
       if (verbose_fmt_string != NULL)
@@ -322,7 +315,7 @@ make_path (const char *argpath,
            {
              error (0, errno, _("cannot change owner and/or group of %s"),
                     quote (dirpath));
-             retval = 1;
+             retval = false;
            }
        }
 
@@ -336,7 +329,7 @@ make_path (const char *argpath,
        {
          error (0, errno, _("cannot change permissions of %s"),
                 quote (dirpath));
-         retval = 1;
+         retval = false;
        }
 
       CLEANUP_CWD;
@@ -351,7 +344,7 @@ make_path (const char *argpath,
            {
              error (0, errno, _("cannot change permissions of %s"),
                     quote (dirpath));
-             retval = 1;
+             retval = false;
            }
        }
     }
@@ -364,7 +357,7 @@ make_path (const char *argpath,
       if (!S_ISDIR (stats.st_mode))
        {
          error (0, 0, _("%s exists but is not a directory"), quote (dirpath));
-         return 1;
+         return false;
        }
 
       if (!preserve_existing)
@@ -384,13 +377,13 @@ make_path (const char *argpath,
            {
              error (0, errno, _("cannot change owner and/or group of %s"),
                     quote (dirpath));
-             retval = 1;
+             retval = false;
            }
          if (chmod (dirpath, mode))
            {
              error (0, errno, _("cannot change permissions of %s"),
                                 quote (dirpath));
-             retval = 1;
+             retval = false;
            }
        }
     }
Index: src/mkdir.c
===================================================================
RCS file: /home/eggert/coreutils/cu/src/mkdir.c,v
retrieving revision 1.89
diff -p -u -r1.89 mkdir.c
--- src/mkdir.c 21 Jun 2004 15:03:35 -0000      1.89
+++ src/mkdir.c 16 Jul 2004 20:28:47 -0000
@@ -37,8 +37,8 @@
 /* The name this program was run with. */
 char *program_name;
 
-/* If nonzero, ensure that all parents of the specified directory exist.  */
-static int create_parents;
+/* If true, ensure that all parents of the specified directory exist.  */
+static bool create_parents;
 
 static struct option const longopts[] =
 {
@@ -85,7 +85,7 @@ main (int argc, char **argv)
   mode_t parent_mode;
   const char *specified_mode = NULL;
   const char *verbose_fmt_string = NULL;
-  int errors = 0;
+  int exit_status = EXIT_SUCCESS;
   int optc;
 
   initialize_main (&argc, &argv);
@@ -96,7 +96,7 @@ main (int argc, char **argv)
 
   atexit (close_stdout);
 
-  create_parents = 0;
+  create_parents = false;
 
   while ((optc = getopt_long (argc, argv, "pm:v", longopts, NULL)) != -1)
     {
@@ -105,7 +105,7 @@ main (int argc, char **argv)
        case 0:                 /* Long option. */
          break;
        case 'p':
-         create_parents = 1;
+         create_parents = true;
          break;
        case 'm':
          specified_mode = optarg;
@@ -146,20 +146,20 @@ main (int argc, char **argv)
 
   for (; optind < argc; ++optind)
     {
-      int fail = 0;
+      bool ok;
 
       if (create_parents)
        {
          char *dir = argv[optind];
-         fail = make_path (dir, newmode, parent_mode,
-                           -1, -1, 1, verbose_fmt_string);
+         ok = make_path (dir, newmode, parent_mode,
+                         -1, -1, true, verbose_fmt_string);
        }
       else
        {
          const char *dir = argv[optind];
-         int dir_created;
-         fail = make_dir (dir, dir, newmode, &dir_created);
-         if (fail)
+         bool dir_created;
+         ok = make_dir (dir, dir, newmode, &dir_created);
+         if (! ok)
            {
              /* make_dir already gave a diagnostic.  */
            }
@@ -170,7 +170,7 @@ main (int argc, char **argv)
                 was specified.  */
              error (0, EEXIST, _("cannot create directory %s"),
                     quote (dir));
-             fail = 1;
+             ok = false;
            }
          else if (verbose_fmt_string)
            error (0, 0, verbose_fmt_string, quote (dir));
@@ -184,18 +184,18 @@ main (int argc, char **argv)
          /* Set the permissions only if this directory has just
             been created.  */
 
-         if (fail == 0 && specified_mode && dir_created)
+         if (ok && specified_mode && dir_created
+             && chmod (dir, newmode) != 0)
            {
-             fail = chmod (dir, newmode);
-             if (fail)
-               error (0, errno, _("cannot set permissions of directory %s"),
-                      quote (dir));
+             error (0, errno, _("cannot set permissions of directory %s"),
+                    quote (dir));
+             ok = false;
            }
        }
 
-      if (fail)
-       errors = 1;
+      if (! ok)
+       exit_status = EXIT_FAILURE;
     }
 
-  exit (errors);
+  exit (exit_status);
 }