Re: [Bug-tar] [tar 1.23] inconsistent error messages if no space on device

[Paul Eggert]

On 10/12/2010 09:27 AM, Hauke Laging wrote:
> From a technical point of view this error message is not really wrong as it 
> is 
> the result of a failed open() call but it is IMHO easily misunderstood as 
> "the 
> file you requested to be restored is not found in the archive". And this 
> ambigious error message occurs only because the previously failed mkdir() 
> call 
> does not cause a program abortion.

Thanks for the bug report.  To some extent tar is at the mercy of the underlying
operating system here: if the system call fails with an errno value E such that
strerror(E) says "jumped off a cliff", tar must to report "jumped off a cliff",
even if that makes little sense to the user, because we don't know what all
the E values might be and it's hard to translate random failures into useful
diagnostics.

That being said, we can make things a bit easier on the user by reporting more
carefully what actually happened.  In this case tar tried both a mkdir and an
open, and it's more helpful to report the mkdir failure than to suppress it.
I installed the following patch to try to do it that way.  This patch also
cleans up some messy and not-that-portable code for detecting the mkdir
failure.

>From e590b1d0d770c06a4d8d8e647890759788d96011 Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Fri, 15 Oct 2010 22:26:14 -0700
Subject: [PATCH] tar: use more-accurate diagnostic when intermediate mkdir fails

Without this change, if tar tried to extract a file A/B/C, noticed
that A/B didn't exist, attempted to mkdir A/B, and the mkdir
failed, it did not diagnose the mkdir failure, but simply reported
the failure to open A/B/C.  This sometimes led to confusion
because it wasn't clear what tar was trying to do, in particular
that tar tried to mkdir A/B.  With this patch, tar issues two
diagnostics in this case: one for A/B and the other for A/B/C.
Problem reported by Hauke Laging in
<http://lists.gnu.org/archive/html/bug-tar/2010-10/msg00020.html>.
* gnulib.modules: Remove faccessat.
* src/extract.c (make_directories): New arg INTERDIR_MADE.
Diagnose mkdir failure.  Return 0 on success, nonzero on failure,
as opposed to nonzero iff some directory was created.  All callers
changed.  Simplify the code when mkdir fails, by checking whether
the desired file exists unless errno==EEXIST: this is more robust.
* tests/extrac15.at: New test, to check this.
* tests/Makefile.am (TESTSUITE_AT): Add it.
* tests/testsuite.at: Include it.
---
 gnulib.modules     |    1 -
 src/extract.c      |   58 ++++++++++++++++++++++++++-------------------------
 tests/Makefile.am  |    1 +
 tests/extrac15.at  |   44 +++++++++++++++++++++++++++++++++++++++
 tests/testsuite.at |    1 +
 5 files changed, 76 insertions(+), 29 deletions(-)
 create mode 100644 tests/extrac15.at

diff --git a/gnulib.modules b/gnulib.modules
index fbf7078..49eab65 100644
--- a/gnulib.modules
+++ b/gnulib.modules
@@ -12,7 +12,6 @@ dirname
 error
 exclude
 exitfail
-faccessat
 fdopendir
 fdutimensat
 fileblocks
diff --git a/src/extract.c b/src/extract.c
index 0a1a032..0d23d4a 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -478,20 +478,20 @@ repair_delayed_set_stat (char const *dir,
 
 /* After a file/link/directory creation has failed, see if
    it's because some required directory was not present, and if so,
-   create all required directories.  Return non-zero if a directory
-   was created.  */
+   create all required directories.  Return zero if all the required
+   directories were created, nonzero (issuing a diagnostic) otherwise.
+   Set *INTERDIR_MADE if at least one directory was created.  */
 static int
-make_directories (char *file_name)
+make_directories (char *file_name, bool *interdir_made)
 {
   char *cursor0 = file_name + FILE_SYSTEM_PREFIX_LEN (file_name);
   char *cursor;                        /* points into the file name */
-  int did_something = 0;       /* did we do anything yet? */
-  int status;
 
   for (cursor = cursor0; *cursor; cursor++)
     {
       mode_t mode;
       mode_t desired_mode;
+      int status;
 
       if (! ISSLASH (*cursor))
        continue;
@@ -524,28 +524,32 @@ make_directories (char *file_name)
                          desired_mode, AT_SYMLINK_NOFOLLOW);
 
          print_for_mkdir (file_name, cursor - file_name, desired_mode);
-         did_something = 1;
-
-         *cursor = '/';
-         continue;
+         *interdir_made = true;
+       }
+      else if (errno == EEXIST)
+       status = 0;
+      else
+       {
+         /* Check whether the desired file exists.  Even when the
+            file exists, mkdir can fail with some errno value E other
+            than EEXIST, so long as E describes an error condition
+            that also applies.  */
+         int e = errno;
+         struct stat st;
+         status = fstatat (chdir_fd, file_name, &st, 0);
+         if (status)
+           {
+             errno = e;
+             mkdir_error (file_name);
+           }
        }
 
       *cursor = '/';
-
-      if (errno == EEXIST)
-       continue;               /* Directory already exists.  */
-      else if ((errno == ENOSYS /* Automounted dirs on Solaris return
-                                  this. Reported by Warren Hyde
-                                  <address@hidden> */
-              || ERRNO_IS_EACCES)  /* Turbo C mkdir gives a funny errno.  */
-              && faccessat (chdir_fd, file_name, W_OK, AT_EACCESS) == 0)
-       continue;
-
-      /* Some other error in the mkdir.  We return to the caller.  */
-      break;
+      if (status)
+       return status;
     }
 
-  return did_something;                /* tell them to retry if we made one */
+  return 0;
 }
 
 /* Return true if FILE_NAME (with status *STP, if STP) is not a
@@ -644,11 +648,8 @@ maybe_recoverable (char *file_name, bool regular, bool 
*interdir_made)
 
     case ENOENT:
       /* Attempt creating missing intermediate directories.  */
-      if (make_directories (file_name))
-       {
-         *interdir_made = true;
-         return RECOVER_OK;
-       }
+      if (make_directories (file_name, interdir_made) == 0 && *interdir_made)
+       return RECOVER_OK;
       break;
 
     default:
@@ -1544,11 +1545,12 @@ rename_directory (char *src, char *dst)
   if (renameat (chdir_fd, src, chdir_fd, dst) != 0)
     {
       int e = errno;
+      bool interdir_made;
 
       switch (e)
        {
        case ENOENT:
-         if (make_directories (dst))
+         if (make_directories (dst, &interdir_made) == 0)
            {
              if (renameat (chdir_fd, src, chdir_fd, dst) == 0)
                return true;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 4bf3ff0..d29563a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -81,6 +81,7 @@ TESTSUITE_AT = \
  extrac12.at\
  extrac13.at\
  extrac14.at\
+ extrac15.at\
  filerem01.at\
  filerem02.at\
  gzip.at\
diff --git a/tests/extrac15.at b/tests/extrac15.at
new file mode 100644
index 0000000..bf8d1cf
--- /dev/null
+++ b/tests/extrac15.at
@@ -0,0 +1,44 @@
+# Process this file with autom4te to create testsuite. -*- Autotest -*-
+
+# Test suite for GNU tar.
+# Copyright (C) 2010 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
+# the Free Software Foundation; either version 3, or (at your option)
+# any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# written by Paul Eggert
+
+# Check diagnostic of 'tar -x a/b/c' when b cannot be created.
+
+AT_SETUP([extract parent mkdir failure])
+AT_KEYWORDS([extract extrac15])
+
+AT_TAR_CHECK([
+AT_UNPRIVILEGED_PREREQ
+
+mkdir src src/a src/a/b dest dest/a
+touch src/a/b/c
+chmod a-w dest/a
+
+tar -cf archive.tar -C src a/b/c &&
+if tar -xf archive.tar -C dest a/b/c
+then (exit 1)
+else (exit 0)
+fi
+],
+[0],[],[tar: a/b: Cannot mkdir: Permission denied
+tar: a/b/c: Cannot open: No such file or directory
+tar: Exiting with failure status due to previous errors
+],[],[],[gnu])
+
+AT_CLEANUP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index c047bd2..c386892 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -153,6 +153,7 @@ m4_include([extrac11.at])
 m4_include([extrac12.at])
 m4_include([extrac13.at])
 m4_include([extrac14.at])
+m4_include([extrac15.at])
 
 m4_include([label01.at])
 m4_include([label02.at])
-- 
1.7.2