Re: mkfifoat, renameat

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> Part of the problem on mingw is that the fchdir module relies on
> canonicalize doing the right thing, but mingw's paths (with drive letters
> and \, rather than leading / for absolute) throws canonicalize for a loop;
> and cross-compilation tries to replace getcwd, even though mingw's getcwd
> always works.  At that point, the populated dirs[fd].name mapping have
> invalid entries, which hurt attempts to do fstat(dfd).  It doesn't help
> that mingw stat() reports st_ino == 0 for every directory, rendering
> SAME_INODE useless.  Oh well, I'll have to spend more time porting those
> pieces to mingw.

Among other things, testing shows that mingw supports getcwd(NULL,0), so the 
cross-compilation guess needs to be improved there.  stat() has the annoying 
property that a trailing slash changes semantics (but / and \\ are 
interchangeable), and in an inconsistent manner (you can't always stat the 
result of getcwd!):

getcwd reports: c:\
stat("c:",&st) => fails
stat("c:/",&st) => passes

getcwd reports: c:\dir
stat("c:/dir",&st) => passes
stat("c:/dir/",&st) => fails

getcwd reports: \\machine\share
stat("\\\\machine\\share",&st) => fails
stat("\\\\machine\\share\\",&st) => passes

getcwd reports: \\machine\share\dir
stat("\\\\machine\\share\\dir",&st) => passes
stat("\\\\machine\\share\\dir\\",&st) => fails

And it doesn't help that canonicalize is not consistently using ISSLASH macros 
to munge '/' and '\\' into a canonical form.  Yep, definitely some groundwork 
to lay here before linkat and renameat will work on mingw.

Meanwhile, I noticed that the link module has a bug on mingw, where link
("a","b/.") created the regular file "b" rather than failing with ENOENT (I 
noticed it because cygwin 1.5 had the same bug, and I noticed that while 
enhancing the link module to work around Solaris 8 handling of trailing / in 
link).  I'll probably be applying this soon...


From: Eric Blake <address@hidden>
Date: Wed, 9 Sep 2009 11:06:44 -0600
Subject: [PATCH 1/2] test-link: consolidate into single C program, test more 
cases

* tests/test-link.sh: Delete.
* tests/test-link.c: Test more error conditions.  Exposes bugs on
at least Cygwin and Solaris.
* modules/link-tests (Files): Remove unused file.
(Depends-on): Add errno, sys_stat.
(Makefile.am): Simplify.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog          |    8 ++++
 modules/link-tests |    6 ++--
 tests/test-link.c  |  103 +++++++++++++++++++++++++++++++++++++++++++++++++---
 tests/test-link.sh |   37 -------------------
 4 files changed, 109 insertions(+), 45 deletions(-)
 delete mode 100755 tests/test-link.sh

diff --git a/ChangeLog b/ChangeLog
index 08fe19a..8372ebe 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
 2009-09-09  Eric Blake  <address@hidden>

+       test-link: consolidate into single C program, test more cases
+       * tests/test-link.sh: Delete.
+       * tests/test-link.c: Test more error conditions.  Exposes bugs on
+       at least Cygwin and Solaris.
+       * modules/link-tests (Files): Remove unused file.
+       (Depends-on): Add errno, sys_stat.
+       (Makefile.am): Simplify.
+
        dirname: add library-safe mdir_name
        * lib/dirname.h (mdir_name): New prototype.
        * lib/dirname.c (dir_name): Move guts...
diff --git a/modules/link-tests b/modules/link-tests
index c1ec52c..ca61deb 100644
--- a/modules/link-tests
+++ b/modules/link-tests
@@ -1,12 +1,12 @@
 Files:
-tests/test-link.sh
 tests/test-link.c

 Depends-on:
+errno
+sys_stat

 configure.ac:

 Makefile.am:
-TESTS += test-link.sh
-TESTS_ENVIRONMENT += EXEEXT='@EXEEXT@'
+TESTS += test-link
 check_PROGRAMS += test-link
diff --git a/tests/test-link.c b/tests/test-link.c
index 250a821..fbc794a 100644
--- a/tests/test-link.c
+++ b/tests/test-link.c
@@ -19,8 +19,12 @@
 #include <unistd.h>

 #include <errno.h>
+#include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>

 #define ASSERT(expr) \
   do                                                                         \
@@ -34,30 +38,119 @@
     }                                                                        \
   while (0)

+#define BASE "test-link.t"
+
 int
 main (int argc, char **argv)
 {
+  int fd;
   int ret;

-  ASSERT (argc == 3);
+  /* Remove any garbage left from previous partial runs.  */
+  ASSERT (system ("rm -rf " BASE "*") == 0);
+
+  /* Create first file.  */
+  fd = open (BASE "a", O_CREAT | O_EXCL | O_WRONLY, 0600);
+  ASSERT (0 <= fd);
+  ASSERT (write (fd, "hello", 5) == 5);
+  ASSERT (close (fd) == 0);

-  ret = link (argv[1], argv[2]);
-  if (ret < 0)
+  /* Not all file systems support link.  Mingw doesn't have reliable
+     st_nlink on hard links, but our implementation does fail with
+     EPERM on poor file systems, and we can detect the inferior stat()
+     via st_ino.  Cygwin 1.5.x copies rather than links files on those
+     file systems, but there, st_nlink and st_ino are reliable.  */
+  ret = link (BASE "a", BASE "b");
+  if (!ret)
+  {
+    struct stat st;
+    ASSERT (stat (BASE "b", &st) == 0);
+    if (st.st_ino && st.st_nlink != 2)
+      {
+        ASSERT (unlink (BASE "b") == 0);
+        errno = EPERM;
+        ret = -1;
+      }
+  }
+  if (ret == -1)
     {
       /* If the device does not support hard links, errno is
         EPERM on Linux, EOPNOTSUPP on FreeBSD.  */
       switch (errno)
        {
        case EPERM:
-#ifdef EOPNOTSUPP
        case EOPNOTSUPP:
-#endif
+          fputs ("skipping test: "
+                 "hard links are not supported on this file system\n", stderr);
+          ASSERT (unlink (BASE "a") == 0);
          return 77;
        default:
          perror ("link");
          return 1;
        }
     }
+  ASSERT (ret == 0);
+
+  /* Now, for some behavior tests.  Modify the contents of 'b', and
+     ensure that 'a' can see it, both while 'b' exists and after.  */
+  fd = open (BASE "b", O_APPEND | O_WRONLY);
+  ASSERT (0 <= fd);
+  ASSERT (write (fd, "world", 5) == 5);
+  ASSERT (close (fd) == 0);
+  {
+    char buf[11] = { 0 };
+    fd = open (BASE "a", O_RDONLY);
+    ASSERT (0 <= fd);
+    ASSERT (read (fd, buf, 10) == 10);
+    ASSERT (strcmp (buf, "helloworld") == 0);
+    ASSERT (close (fd) == 0);
+    ASSERT (unlink (BASE "b") == 0);
+    fd = open (BASE "a", O_RDONLY);
+    ASSERT (0 <= fd);
+    ASSERT (read (fd, buf, 10) == 10);
+    ASSERT (strcmp (buf, "helloworld") == 0);
+    ASSERT (close (fd) == 0);
+  }
+
+  /* Test for various error conditions.  Assumes hard links to
+     directories are not permitted.  */
+  ASSERT (mkdir (BASE "d", 0700) == 0);
+  errno = 0;
+  ASSERT (link (BASE "a", ".") == -1);
+  ASSERT (errno == EEXIST || errno == EINVAL);
+  errno = 0;
+  ASSERT (link (BASE "a", BASE "a") == -1);
+  ASSERT (errno == EEXIST);
+  ASSERT (link (BASE "a", BASE "b") == 0);
+  errno = 0;
+  ASSERT (link (BASE "a", BASE "b") == -1);
+  ASSERT (errno == EEXIST);
+  errno = 0;
+  ASSERT (link (BASE "a", BASE "d") == -1);
+  ASSERT (errno == EEXIST);
+  errno = 0;
+  ASSERT (link (BASE "c", BASE "e") == -1);
+  ASSERT (errno == ENOENT);
+  errno = 0;
+  ASSERT (link (BASE "a", BASE "c/.") == -1);
+  ASSERT (errno == ENOENT);
+  errno = 0;
+  ASSERT (link (BASE "a/", BASE "c") == -1);
+  ASSERT (errno == ENOTDIR);
+  errno = 0;
+  ASSERT (link (BASE "a", BASE "c/") == -1);
+  ASSERT (errno == ENOTDIR);
+  errno = 0;
+  ASSERT (link (BASE "d", BASE "c") == -1);
+  ASSERT (errno == EPERM || errno == EACCES);
+
+  /* Clean up.  */
+  ASSERT (unlink (BASE "a") == 0);
+  ASSERT (unlink (BASE "b") == 0);
+  errno = 0;
+  ASSERT (unlink (BASE "c") == -1);
+  ASSERT (errno == ENOENT);
+  ASSERT (rmdir (BASE "d") == 0);

   return 0;
 }
diff --git a/tests/test-link.sh b/tests/test-link.sh
deleted file mode 100755
index 1519f9d..0000000
--- a/tests/test-link.sh
+++ /dev/null
@@ -1,37 +0,0 @@
-#!/bin/sh
-
-tmpfiles="test-link-a.txt test-link-b.txt test-link-c.txt"
-trap 'rm -fr $tmpfiles' 1 2 3 15
-
-# Create a file.
-echo "hello" > test-link-a.txt || exit 1
-
-# Use link() to create a new name for it.
-./test-link${EXEEXT} test-link-a.txt test-link-b.txt
-case $? in
-  0) ;;
-  77)
-    echo "Skipping test: hard links are not supported on this file system"
-    rm -fr $tmpfiles
-    exit 77
-    ;;
-  *) exit 1 ;;
-esac
-cmp test-link-a.txt test-link-b.txt || exit 1
-
-# Modify the contents of the first file.
-echo "world" >> test-link-a.txt || exit 1
-cmp test-link-a.txt test-link-b.txt || exit 1
-
-# Modify the contents of the second file.
-echo "some text" >> test-link-b.txt || exit 1
-cmp test-link-a.txt test-link-b.txt || exit 1
-
-# Delete the first file, then verity the second file still has the same
-# contents.
-cp test-link-a.txt test-link-c.txt || exit 1
-rm test-link-a.txt || exit 1
-cmp test-link-b.txt test-link-c.txt || exit 1
-
-rm -fr $tmpfiles
-exit 0
-- 
1.6.3.2


>From 9101691fafa16a95b3fb0a95abe9c43da26fa907 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Wed, 9 Sep 2009 15:25:26 -0600
Subject: [PATCH 2/2] link: fix platform bugs

* m4/link.m4 (gl_FUNC_LINK): Detect Solaris and Cygwin bugs.
* lib/link.c (link): Work around them.  Fix related mingw bug.
* m4/unistd_h.m4 (gl_UNISTD_H_DEFAULTS): Add REPLACE_LINK.
* modules/unistd (Makefile.am): Substitute it.
* lib/unistd.in.h (link): Declare replacement.
* doc/posix-functions/link.texi (link): Document this.
* modules/link (Depends-on): Add strdup-posix, sys_stat.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                     |    9 +++
 doc/posix-functions/link.texi |    4 ++
 lib/link.c                    |  110 +++++++++++++++++++++++++++++++++++++---
 lib/unistd.in.h               |    5 ++-
 m4/link.m4                    |   21 ++++++--
 m4/unistd_h.m4                |    3 +-
 modules/link                  |    4 +-
 modules/unistd                |    1 +
 8 files changed, 140 insertions(+), 17 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8372ebe..d35aba7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2009-09-09  Eric Blake  <address@hidden>

+       link: fix platform bugs
+       * m4/link.m4 (gl_FUNC_LINK): Detect Solaris and Cygwin bugs.
+       * lib/link.c (link): Work around them.  Fix related mingw bug.
+       * m4/unistd_h.m4 (gl_UNISTD_H_DEFAULTS): Add REPLACE_LINK.
+       * modules/unistd (Makefile.am): Substitute it.
+       * lib/unistd.in.h (link): Declare replacement.
+       * doc/posix-functions/link.texi (link): Document this.
+       * modules/link (Depends-on): Add strdup-posix, sys_stat.
+
        test-link: consolidate into single C program, test more cases
        * tests/test-link.sh: Delete.
        * tests/test-link.c: Test more error conditions.  Exposes bugs on
diff --git a/doc/posix-functions/link.texi b/doc/posix-functions/link.texi
index ace07cd..c785371 100644
--- a/doc/posix-functions/link.texi
+++ b/doc/posix-functions/link.texi
@@ -11,6 +11,10 @@ link
 @item
 This function is missing on some platforms:
 mingw.
address@hidden
+This function fails to reject trailing slashes on non-directories on
+some platforms:
+Solaris, Cygwin 1.5.x.
 @end itemize

 Portability problems not fixed by Gnulib:
diff --git a/lib/link.c b/lib/link.c
index 2b5a97f..72b8600 100644
--- a/lib/link.c
+++ b/lib/link.c
@@ -18,13 +18,18 @@

 #include <config.h>

-#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
-
-#define WIN32_LEAN_AND_MEAN
 #include <unistd.h>
-#include <windows.h>

 #include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+
+#if !HAVE_LINK
+# if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+
+#  define WIN32_LEAN_AND_MEAN
+#  include <windows.h>

 /* CreateHardLink was introduced only in Windows 2000.  */
 typedef BOOL (WINAPI * CreateHardLinkFuncType) (LPCTSTR lpFileName,
@@ -46,8 +51,11 @@ initialize (void)
 }

 int
-link (const char *path1, const char *path2)
+link (const char *file1, const char *file2)
 {
+  char *dir;
+  size_t len1 = strlen (file1);
+  size_t len2 = strlen (file2);
   if (!initialized)
     initialize ();
   if (CreateHardLinkFunc == NULL)
@@ -56,7 +64,39 @@ link (const char *path1, const char *path2)
       errno = EPERM;
       return -1;
     }
-  if (CreateHardLinkFunc (path2, path1, NULL) == 0)
+  /* Reject trailing slashes on non-directories; mingw does not
+     support hard-linking directories.  */
+  if ((len1 && (file1[len1 - 1] == '/' || file1[len1 - 1] == '\\'))
+      || (len2 && (file2[len2 - 1] == '/' || file2[len2 - 1] == '\\')))
+    {
+      struct stat st;
+      if (stat (file1, &st) == 0 && S_ISDIR (st.st_mode))
+        errno = EPERM;
+      else
+        errno = ENOTDIR;
+      return -1;
+    }
+  /* CreateHardLink("b/.","a",NULL) creates file "b", so we must check
+     that dirname(file2) exists.  */
+  dir = strdup (file2);
+  if (!dir)
+    return -1;
+  {
+    struct stat st;
+    char *p = strchr (dir, '\0');
+    while (dir < p && (*--p != '/' && *p != '\\'));
+    *p = '\0';
+    if (p != dir && stat (dir, &st) == -1)
+      {
+        int saved_errno = errno;
+        free (dir);
+        errno = saved_errno;
+        return -1;
+      }
+    free (dir);
+  }
+  /* Now create the link.  */
+  if (CreateHardLinkFunc (file2, file1, NULL) == 0)
     {
       /* It is not documented which errors CreateHardLink() can produce.
        * The following conversions are based on tests on a Windows XP SP2
@@ -102,8 +142,60 @@ link (const char *path1, const char *path2)
   return 0;
 }

-#else /* !Windows */
+# else /* !Windows */
+
+#  error "This platform lacks a link function, and Gnulib doesn't provide a 
replacement. This is a bug in Gnulib."
+
+# endif /* !Windows */
+#else /* HAVE_LINK */

-#error "This platform lacks a link function, and Gnulib doesn't provide a 
replacement. This is a bug in Gnulib."
+# undef link

-#endif /* !Windows */
+/* Create a hard link from FILE1 to FILE2, working around platform bugs.  */
+int
+rpl_link (char const *file1, char const *file2)
+{
+  /* Reject trailing slashes on non-directories.  */
+  size_t len1 = strlen (file1);
+  size_t len2 = strlen (file2);
+  if ((len1 && file1[len1 - 1] == '/')
+      || (len2 && file2[len2 - 1] == '/'))
+    {
+      /* Let link() decide whether hard-linking directories is legal.
+         If stat() fails, link() will probably fail for the same
+         reason; so we only have to worry about successful stat() and
+         non-directory.  */
+      struct stat st;
+      if (stat (file1, &st) == 0 && !S_ISDIR (st.st_mode))
+        {
+          errno = ENOTDIR;
+          return -1;
+        }
+    }
+  else
+    {
+      /* Fix Cygwin 1.5.x bug where link("a","b/.") creates file "b".  */
+      char *dir = strdup (file2);
+      struct stat st;
+      char *p;
+      if (!dir)
+        return -1;
+      /* We already know file2 does not end in slash.  Strip off the
+         basename, then check that the dirname exists.  */
+      p = strrchr (dir, '/');
+      if (p)
+        {
+          *p = '\0';
+          if (stat (dir, &st) == -1)
+            {
+              int saved_errno = errno;
+              free (dir);
+              errno = saved_errno;
+              return -1;
+            }
+        }
+      free (dir);
+    }
+  return link (file1, file2);
+}
+#endif /* HAVE_LINK */
diff --git a/lib/unistd.in.h b/lib/unistd.in.h
index 902b025..f191412 100644
--- a/lib/unistd.in.h
+++ b/lib/unistd.in.h
@@ -595,11 +595,14 @@ extern int lchown (char const *file, uid_t owner, gid_t 
group);


 #if @GNULIB_LINK@
+# if @REPLACE_LINK@
+#  define link rpl_link
+# endif
 /* Create a new hard link for an existing file.
    Return 0 if successful, otherwise -1 and errno set.
    See POSIX:2001 specification
    <http://www.opengroup.org/susv3xsh/link.html>.  */
-# if address@hidden@
+# if address@hidden@ || @REPLACE_LINK@
 extern int link (const char *path1, const char *path2);
 # endif
 #elif defined GNULIB_POSIXCHECK
diff --git a/m4/link.m4 b/m4/link.m4
index 349d537..fc071cd 100644
--- a/m4/link.m4
+++ b/m4/link.m4
@@ -1,4 +1,4 @@
-# link.m4 serial 1
+# link.m4 serial 2
 dnl Copyright (C) 2009 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -11,9 +11,20 @@ AC_DEFUN([gl_FUNC_LINK],
   if test $ac_cv_func_link = no; then
     HAVE_LINK=0
     AC_LIBOBJ([link])
-    gl_PREREQ_LINK
+  else
+    AC_CACHE_CHECK([whether link handles trailing slash correctly],
+      [gl_cv_func_link_works],
+      [touch conftest.a
+       AC_RUN_IFELSE(
+         [AC_LANG_PROGRAM(
+           [[#include <unistd.h>
+]], [[return !link ("conftest.a", "conftest.b/");]])],
+         [gl_cv_func_link_works=yes], [gl_cv_func_link_works=no],
+         [gl_cv_func_link_works="guessing no"])
+       rm -f conftest.a conftest.b])
+    if test $gl_cv_func_link_works != yes; then
+      REPLACE_LINK=1
+      AC_LIBOBJ([link])
+    fi
   fi
 ])
-
-# Prerequisites of lib/link.c.
-AC_DEFUN([gl_PREREQ_LINK], [:])
diff --git a/m4/unistd_h.m4 b/m4/unistd_h.m4
index 84f0755..7347e38 100644
--- a/m4/unistd_h.m4
+++ b/m4/unistd_h.m4
@@ -1,4 +1,4 @@
-# unistd_h.m4 serial 24
+# unistd_h.m4 serial 25
 dnl Copyright (C) 2006-2009 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -94,6 +94,7 @@ AC_DEFUN([gl_UNISTD_H_DEFAULTS],
   REPLACE_GETCWD=0;       AC_SUBST([REPLACE_GETCWD])
   REPLACE_GETPAGESIZE=0;  AC_SUBST([REPLACE_GETPAGESIZE])
   REPLACE_LCHOWN=0;       AC_SUBST([REPLACE_LCHOWN])
+  REPLACE_LINK=0;         AC_SUBST([REPLACE_LINK])
   REPLACE_LSEEK=0;        AC_SUBST([REPLACE_LSEEK])
   REPLACE_WRITE=0;        AC_SUBST([REPLACE_WRITE])
   UNISTD_H_HAVE_WINSOCK2_H=0; AC_SUBST([UNISTD_H_HAVE_WINSOCK2_H])
diff --git a/modules/link b/modules/link
index 31d1af4..9492950 100644
--- a/modules/link
+++ b/modules/link
@@ -6,6 +6,8 @@ lib/link.c
 m4/link.m4

 Depends-on:
+strdup-posix
+sys_stat
 unistd

 configure.ac:
@@ -21,4 +23,4 @@ License:
 LGPLv2+

 Maintainer:
-Martin Lambers
+Martin Lambers, Eric Blake
diff --git a/modules/unistd b/modules/unistd
index 1f8b29e..37ecaaa 100644
--- a/modules/unistd
+++ b/modules/unistd
@@ -86,6 +86,7 @@ unistd.h: unistd.in.h
              -e 's|@''REPLACE_GETCWD''@|$(REPLACE_GETCWD)|g' \
              -e 's|@''REPLACE_GETPAGESIZE''@|$(REPLACE_GETPAGESIZE)|g' \
              -e 's|@''REPLACE_LCHOWN''@|$(REPLACE_LCHOWN)|g' \
+             -e 's|@''REPLACE_LINK''@|$(REPLACE_LINK)|g' \
              -e 's|@''REPLACE_LSEEK''@|$(REPLACE_LSEEK)|g' \
              -e 's|@''REPLACE_WRITE''@|$(REPLACE_WRITE)|g' \
              -e 's|@''UNISTD_H_HAVE_WINSOCK2_H''@|$(UNISTD_H_HAVE_WINSOCK2_H)
|g' \
-- 
1.6.3.2