bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: minor mkdir bug fixes


From: Paul Eggert
Subject: Re: minor mkdir bug fixes
Date: Tue, 14 Jun 2005 17:07:49 -0700
User-agent: Gnus/5.1006 (Gnus v5.10.6) Emacs/21.4 (gnu/linux)

I found some more glitches in the mkdir code.  Occasionally multiple
diagnostics might be generated where one would do.  The diagnostics
could report the error number, but didn't.  In one unlikely case in
'install', the code would install the file into the wrong place.

Also, there's no need for make_dir_parents to invoke 'stat' each time
through the loop.  It can just rely on information gleaned from the
other system calls.  In theory even the first "stat" could go but I'm
not sure that would improve performance on the average, since "mkdir
-p /some/existing/file" must be a common case.

I installed this:

2005-06-14  Paul Eggert  <address@hidden>

        Improve diagnostics for restore_cwd failure.
        * lib/mkdir-p.h (make_dir): Remove.  All uses replaced by mkdir.
        (make_dir_parents): Last arg is now int * (for errno), not bool *.
        * lib/mkdir-p.c (make_dir, make_dir_parents): Likewise.
        Rewrite "mkdir -p" algorithm to avoid the need for "stat"
        each time through the loop.  Do not diagnose restore_cwd failure;
        that is the caller's job (and perhaps the caller does not care).
        * src/install.c (main): Standardize on a diagnostic for
        restore_cwd failure, and report errno.
        (install_file_in_file_parents): Fail if restore_cwd fails and
        one of the files is relative.  This fixes a bug (albeit unlikely).
        * src/mkdir.c (create_parents): Remove static var (now local to 'main').
        (main): Standardize on a diagnostic for restore_cwd failure,
        and report errno.
        Don't bother to check cwd_errno unless create_parents.
        Use mkdir rather than make_dir; it's simpler.

Index: NEWS
===================================================================
RCS file: /fetish/cu/NEWS,v
retrieving revision 1.294
retrieving revision 1.296
diff -p -u -r1.294 -r1.296
--- NEWS        13 Jun 2005 10:19:29 -0000      1.294
+++ NEWS        15 Jun 2005 00:06:34 -0000      1.296
@@ -124,7 +124,8 @@ GNU coreutils NEWS                      
 
   "mkdir -p /tmp/a/b dir" no longer attempts to create the `.'-relative
   directory, dir (in /tmp/a), when, after creating /tmp/a/b, it is unable
-  to return to its initial working directory.
+  to return to its initial working directory.  Similarly for "install -D
+  file /tmp/a/b/file".
 
   "pr -D FORMAT" now accepts the same formats that "date +FORMAT" does.
 
Index: lib/mkdir-p.h
===================================================================
RCS file: /fetish/cu/lib/mkdir-p.h,v
retrieving revision 1.2
diff -p -u -r1.2 mkdir-p.h
--- lib/mkdir-p.h       13 Jun 2005 10:22:26 -0000      1.2
+++ lib/mkdir-p.h       14 Jun 2005 23:45:14 -0000
@@ -29,9 +29,4 @@ bool make_dir_parents (char const *argna
                       gid_t group,
                       bool preserve_existing,
                       char const *verbose_fmt_string,
-                      bool *different_working_dir);
-
-bool make_dir (char const *dir,
-              char const *fulldir,
-              mode_t mode,
-              bool *created_dir_p);
+                      int *cwd_errno);
Index: lib/mkdir-p.c
===================================================================
RCS file: /fetish/cu/lib/mkdir-p.c,v
retrieving revision 1.6
diff -p -u -r1.6 mkdir-p.c
--- lib/mkdir-p.c       14 Jun 2005 18:35:38 -0000      1.6
+++ lib/mkdir-p.c       14 Jun 2005 23:45:15 -0000
@@ -49,59 +49,6 @@
 
 #define WX_USR (S_IWUSR | S_IXUSR)
 
-/* Attempt to create directory DIR (aka FULLDIR) with the specified MODE.
-   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 FULLDIR in any diagnostic, not DIR.
-   Note that if DIR already exists, this function returns true
-   (indicating success) and clears *CREATED_DIR_P.  */
-
-bool
-make_dir (char const *dir, char const *fulldir, mode_t mode,
-         bool *created_dir_p)
-{
-  bool ok = true;
-  bool created_dir;
-
-  created_dir = (mkdir (dir, mode) == 0);
-
-  if (!created_dir)
-    {
-      struct stat stats;
-      int saved_errno = errno;
-
-      /* The mkdir and stat calls below may appear to be reversed.
-        They are not.  It is important to call mkdir first and then to
-        call stat (to distinguish the three cases) only if mkdir fails.
-        The alternative to this approach is to `stat' each directory,
-        then to call mkdir if it doesn't exist.  But if some other process
-        were to create the directory between the stat & mkdir, the mkdir
-        would fail with EEXIST.  */
-
-      if (stat (dir, &stats))
-       {
-         error (0, saved_errno, _("cannot create directory %s"),
-                quote (fulldir));
-         ok = false;
-       }
-      else if (!S_ISDIR (stats.st_mode))
-       {
-         error (0, 0, _("%s exists but is not a directory"), quote (fulldir));
-         ok = false;
-       }
-      else
-       {
-         /* DIR (aka FULLDIR) already exists and is a directory. */
-       }
-    }
-
-  if (created_dir_p)
-    *created_dir_p = created_dir;
-
-  return ok;
-}
-
 /* Ensure that the directory ARG exists.
 
    Create any leading directories that don't already exist, with
@@ -116,9 +63,10 @@ make_dir (char const *dir, char const *f
    If PRESERVE_EXISTING is true and ARG is an existing directory,
    then do not attempt to set its permissions and ownership.
 
-   Set *DIFFERENT_WORKING_DIR to true if this function has changed the
-   current working directory and is unable to restore it to its
-   initial state.  Do not change *DIFFERENT_WORKING_DIR otherwise.
+   Set *CWD_ERRNO to a (nonzero) error number if this
+   function has changed the current working directory and is unable to
+   restore it to its initial state.  Do not change
+   *CWD_ERRNO otherwise.
 
    Return true iff ARG exists as a directory with the proper ownership
    and permissions when done.  Note that this function returns true
@@ -132,7 +80,7 @@ make_dir_parents (char const *arg,
                  gid_t group,
                  bool preserve_existing,
                  char const *verbose_fmt_string,
-                 bool *different_working_dir)
+                 int *cwd_errno)
 {
   struct stat stats;
   bool retval = true;
@@ -225,8 +173,6 @@ make_dir_parents (char const *arg,
 
       while (true)
        {
-         bool newly_created_dir;
-
          /* slash points to the leftmost unprocessed component of dir.  */
          basename_dir = slash;
 
@@ -240,13 +186,7 @@ make_dir_parents (char const *arg,
            basename_dir = dir;
 
          *slash = '\0';
-         if (! make_dir (basename_dir, dir, tmp_mode, &newly_created_dir))
-           {
-             retval = false;
-             break;
-           }
-
-         if (newly_created_dir)
+         if (mkdir (basename_dir, tmp_mode) == 0)
            {
              if (verbose_fmt_string)
                error (0, 0, verbose_fmt_string, quote (dir));
@@ -273,11 +213,22 @@ make_dir_parents (char const *arg,
                  leading_dirs = new;
                }
            }
+         else if (errno == EEXIST)
+           {
+             /* A file is already there.  Perhaps it is a directory.
+                If not, it will be diagnosed later.  */
+           }
+         else
+           {
+             error (0, errno, _("cannot create directory %s"), quote (dir));
+             retval = false;
+             break;
+           }
 
          /* If we were able to save the initial working directory,
             then we can use chdir to change into each directory before
             creating an entry in that directory.  This avoids making
-            stat and mkdir process O(n^2) file name components.  */
+            mkdir process O(n^2) file name components.  */
          if (do_chdir && chdir (basename_dir) < 0)
            {
              error (0, errno, _("cannot chdir to directory %s"),
@@ -288,7 +239,7 @@ make_dir_parents (char const *arg,
 
          *slash++ = '/';
 
-         /* Avoid unnecessary calls to `stat' when given
+         /* Avoid unnecessary calls to mkdir when given
             file names containing multiple adjacent slashes.  */
          while (*slash == '/')
            slash++;
@@ -304,12 +255,12 @@ make_dir_parents (char const *arg,
         Create the final component of the file name.  */
       if (retval)
        {
-         bool newly_created_dir;
-
-         if (! make_dir (basename_dir, dir, mode, &newly_created_dir))
-           retval = false;
-
-         if (newly_created_dir)
+         if (mkdir (basename_dir, mode) != 0)
+           {
+             error (0, errno, _("cannot create directory %s"), quote (dir));
+             retval = false;
+           }
+         else
            {
              if (verbose_fmt_string)
                error (0, 0, verbose_fmt_string, quote (dir));
@@ -355,17 +306,12 @@ make_dir_parents (char const *arg,
 
   if (do_chdir)
     {
-      int saved_errno;
-      cwd_problem = (restore_cwd (&cwd) != 0);
-      saved_errno = errno;
-      free_cwd (&cwd);
-
-      if (cwd_problem)
+      if (restore_cwd (&cwd) != 0)
        {
-         error (0, saved_errno,
-                _("failed to return to initial working directory"));
-         *different_working_dir = true;
+         *cwd_errno = errno;
+         cwd_problem = true;
        }
+      free_cwd (&cwd);
     }
 
   /* If the mode for leading directories didn't include owner "wx"
Index: src/install.c
===================================================================
RCS file: /fetish/cu/src/install.c,v
retrieving revision 1.183
diff -p -u -r1.183 install.c
--- src/install.c       14 Jun 2005 18:35:58 -0000      1.183
+++ src/install.c       14 Jun 2005 23:45:15 -0000
@@ -357,23 +357,19 @@ main (int argc, char **argv)
   if (dir_arg)
     {
       int i;
-      bool cwd_not_restored = false;
+      int cwd_errno = 0;
       for (i = 0; i < n_files; i++)
        {
-         if (cwd_not_restored && IS_RELATIVE_FILE_NAME (argv[optind]))
+         if (cwd_errno != 0 && IS_RELATIVE_FILE_NAME (argv[optind]))
            {
-             error (0, 0,
-                    _("unable to create relative-named directory, %s,"
-                      " due to prior failure to return to working directory"),
-                    quote (argv[optind]));
-             ok = false;;
-             continue;
+             error (0, cwd_errno, _("cannot return to working directory"));
+             ok = false;
            }
-
-         ok &=
-           make_dir_parents (file[i], mode, mode, owner_id, group_id, false,
-                             (x.verbose ? _("creating directory %s") : NULL),
-                             &cwd_not_restored);
+         else
+           ok &=
+             make_dir_parents (file[i], mode, mode, owner_id, group_id, false,
+                               (x.verbose ? _("creating directory %s") : NULL),
+                               &cwd_errno);
        }
     }
   else
@@ -421,12 +417,17 @@ install_file_in_file_parents (char const
         that this option is intended mainly to help installers when the
         distribution doesn't provide proper install rules.  */
       mode_t dir_mode = S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH;
-      bool different_cwd;
+      int cwd_errno = 0;
       ok = make_dir_parents (dest_dir, dir_mode, dir_mode,
                             owner_id, group_id, true,
                             (x->verbose ? _("creating directory %s") : NULL),
-                            &different_cwd);
-      /* Ignore different_cwd, since this function is called at most once.  */
+                            &cwd_errno);
+      if (ok && cwd_errno != 0
+         && (IS_RELATIVE_FILE_NAME (from) || IS_RELATIVE_FILE_NAME (to)))
+       {
+         error (0, cwd_errno, _("cannot return to current directory"));
+         ok = false;
+       }
     }
 
   free (dest_dir);
Index: src/mkdir.c
===================================================================
RCS file: /fetish/cu/src/mkdir.c,v
retrieving revision 1.100
diff -p -u -r1.100 mkdir.c
--- src/mkdir.c 14 Jun 2005 18:36:09 -0000      1.100
+++ src/mkdir.c 14 Jun 2005 23:45:15 -0000
@@ -37,9 +37,6 @@
 /* The name this program was run with. */
 char *program_name;
 
-/* If true, ensure that all parents of the specified directory exist.  */
-static bool create_parents;
-
 static struct option const longopts[] =
 {
   {"mode", required_argument, NULL, 'm'},
@@ -85,9 +82,10 @@ main (int argc, char **argv)
   mode_t parent_mode IF_LINT (= 0);
   const char *specified_mode = NULL;
   const char *verbose_fmt_string = NULL;
+  bool create_parents = false;
   int exit_status = EXIT_SUCCESS;
   int optc;
-  bool cwd_not_restored = false;
+  int cwd_errno = 0;
 
   initialize_main (&argc, &argv);
   program_name = argv[0];
@@ -97,8 +95,6 @@ main (int argc, char **argv)
 
   atexit (close_stdout);
 
-  create_parents = false;
-
   while ((optc = getopt_long (argc, argv, "pm:v", longopts, NULL)) != -1)
     {
       switch (optc)
@@ -148,42 +144,27 @@ main (int argc, char **argv)
 
   for (; optind < argc; ++optind)
     {
+      char *dir = argv[optind];
       bool ok;
 
-      if (cwd_not_restored && IS_RELATIVE_FILE_NAME (argv[optind]))
-       {
-         error (0, 0, _("unable to create relative-named directory, %s,"
-                        " due to prior failure to return to working 
directory"),
-                quote (argv[optind]));
-         exit_status = EXIT_FAILURE;
-         continue;
-       }
-
       if (create_parents)
        {
-         char *dir = argv[optind];
-         ok = make_dir_parents (dir, newmode, parent_mode,
-                                -1, -1, true, verbose_fmt_string,
-                                &cwd_not_restored);
+         if (cwd_errno != 0 && IS_RELATIVE_FILE_NAME (dir))
+           {
+             error (0, cwd_errno, _("cannot return to working directory"));
+             ok = false;
+           }
+         else
+           ok = make_dir_parents (dir, newmode, parent_mode,
+                                  -1, -1, true, verbose_fmt_string,
+                                  &cwd_errno);
        }
       else
        {
-         const char *dir = argv[optind];
-         bool dir_created;
-         ok = make_dir (dir, dir, newmode, &dir_created);
+         ok = (mkdir (dir, newmode) == 0);
+
          if (! ok)
-           {
-             /* make_dir already gave a diagnostic.  */
-           }
-         else if (!create_parents && !dir_created)
-           {
-             /* make_dir `succeeds' when DIR already exists.
-                In that case, mkdir must fail, unless --parents (-p)
-                was specified.  */
-             error (0, EEXIST, _("cannot create directory %s"),
-                    quote (dir));
-             ok = false;
-           }
+           error (0, errno, _("cannot create directory %s"), quote (dir));
          else if (verbose_fmt_string)
            error (0, 0, verbose_fmt_string, quote (dir));
 
@@ -196,7 +177,7 @@ main (int argc, char **argv)
          /* Set the permissions only if this directory has just
             been created.  */
 
-         if (ok && specified_mode && dir_created
+         if (ok && specified_mode
              && chmod (dir, newmode) != 0)
            {
              error (0, errno, _("cannot set permissions of directory %s"),




reply via email to

[Prev in Thread] Current Thread [Next in Thread]