[Support] Implement is_local_impl with AIX mntctl

[hubert.reinterpretcast]

Summary

On AIX, we can determine whether a filesystem is remote using mntctl.

If the information is not found, then claim that the file is remote (since that is the more restrictive case).

[hubert.reinterpretcast]

lib/Support/Unix/Path.inc

446 ↗

(On Diff #188810)

There's no testing for this. The previous attempt at having testing for this interface was removed in http://llvm.org/viewvc/llvm-project?view=revision&revision=297260. It seems that a reasonable expectation is that a newly created file and its parent directory would produce the same answer when queried. I am not sure if there is a desire to pursue adding such a test.

[hubert.reinterpretcast]

Ping. I am looking for some input on whether we want to pursue additional testing.

[Hahnfeld]

In D58801#1420179, @hubert.reinterpretcast wrote:

Ping. I am looking for some input on whether we want to pursue additional testing.

Yes, testing this is hard because we can have all possible combinations of the source and the build directory being remote / local.

I think creating a directory and checking that a contained file returns the same value as the directory should work. However I'm not sure how much value this adds. If it's reasonably easy to implement I think it makes sense, but personally I wouldn't see it as requirement for this patch

[apaprocki]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

sys/statfs.h includes sys/types.h, which contains this typedef. Was there a reason why it was put here between the two headers?

[hubert.reinterpretcast]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

Yes, the header is defective. It needs a typedef that is only conditionally defined.

[apaprocki]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

I guess I am not seeing that.. is it OS version dependent?

$ cat test.c
#include <sys/statfs.h>
#include <sys/vmount.h>
$ xlc -E -c test.c | grep "typedef.*uint;"
typedef         uint_t          uint;
$

[hubert.reinterpretcast]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

Try with -D_XOPEN_SOURCE=700 or anything that suppresses _ALL_SOURCE:

> xlclang -D_XOPEN_SOURCE=700 test.c -c
In file included from test.c:2:
/usr/include/sys/vmount.h:195:2: error: unknown type name 'uint'; did you mean 'int'?
        uint    vmt_revision;   /* I revision level, currently 1        */
        ^
/usr/include/sys/vmount.h:196:2: error: unknown type name 'uint'; did you mean 'int'?
        uint    vmt_length;     /* I total length of structure and data */
        ^
/usr/include/sys/vmount.h:203:2: error: unknown type name 'uint'; did you mean 'int'?
        uint    vmt_time;       /* O time of mount                      */
        ^
/usr/include/sys/vmount.h:204:2: error: unknown type name 'uint'; did you mean 'int'?
        uint    vmt_timepad;    /* O (in future, time is 2 longs)       */
        ^
4 errors generated.
Error while processing test.c.
Return:  0x01:1

[xingxue]

LGTM. Thanks!

[krytarowski]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

This typedef looks suspicious, there shall be a way to workaround it.

[hubert.reinterpretcast]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

What is your proposed solution to otherwise provide a type name required by the system header that might not be defined? Note that this code is guarded under defined(_AIX), and it has been demonstrated that this typedef might be defined anyway by the system headers on that platform to be an alias for uint_t (which is, in turn, an alias for unsigned int).

[krytarowski]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

I don't have access right now to AIX to check it myself, but on UNIX it's typically needed to include <sys/types.h> and/or specify -D flags like -D_POSIX_SOURCE.

[hubert.reinterpretcast]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

Yes, and <sys/types.h> needs -D_ALL_SOURCE before uint is provided, and _ALL_SOURCE on AIX produces a lot of stray macros. I'd like to avoid needing _ALL_SOURCE for the build if at all possible.

[krytarowski]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

How about restricting _ALL_SOURCE to this file only?

#define _ALL_SOURCE // Needed for uint typedef
#include <sys/statfs.h>

[hubert.reinterpretcast]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

That would not work if sys/types.h is included by an earlier header, and defining _ALL_SOURCE earlier in Path.cpp would expose the LLVM headers included by that file to the stray macros that come with _ALL_SOURCE.

[hubert.reinterpretcast]

Address review comments, add test, and apply style changes

• Add comment re: the uint typedef
• Add modified version of the unit test from rL295768
• Use CamelCase for variable names; improve comments for error cases

[hubert.reinterpretcast]

lib/Support/Unix/Path.inc

81 ↗

(On Diff #188810)

Comment added as requested on the review thread on the mailing list.

446 ↗

(On Diff #188810)

The test is now added.

[hubert.reinterpretcast]

@xingxue, thanks for the review. I've updated the patch; please let me know if you have concerns with the updates. I'll probably commit this later this week.

[mstojanovic]

llvm/trunk/unittests/Support/Path.cpp

1495

This test fails if /tmp in on an nfs. I believe the original test was removed in rL297260 for the same reason.

[hubert.reinterpretcast]

llvm/trunk/unittests/Support/Path.cpp

1495

This checks that a file and its containing directory are equally local or remote. If you are noticing a failure, then it seems the implementation of the query on your system may be reporting on the mountpoint instead of the mounted filesystem.

[mstojanovic]

llvm/trunk/unittests/Support/Path.cpp

1495

The problem isn't in the actual test, it's in the cleanup process afterwards. The temp file isn't really deleted, only silly renamed into .nfsXXXXX and when the program tries to delete the file-system-test-YYYYY directory it outputs an error message: Directory not empty.
The simplest way to reproduce this is to hard code the TestDirectory path to a directory created on an nfs.

[mstojanovic]

llvm/trunk/unittests/Support/Path.cpp

1495

Adding ::close(FD) at the end seems to fix it.

[hubert.reinterpretcast]

llvm/trunk/unittests/Support/Path.cpp

1495

@mstojanovic, that seems to be the right solution. Thanks for diagnosing the problem. Would you be committing that directly? I certainly would not mind if you did.

[mstojanovic]

llvm/trunk/unittests/Support/Path.cpp

1495

Ok I'll commit it directly.

llvm/trunk/lib/Support/Unix/Path.inc

 // For GNU Hurd
 #if defined(__GNU__) && !defined(PATH_MAX)
 # define PATH_MAX 4096
 # define MAXPATHLEN 4096
 #endif
 
 #include <sys/types.h>
 #if !defined(__APPLE__) && !defined(__OpenBSD__) && !defined(__FreeBSD__) &&   \
-    !defined(__linux__) && !defined(__FreeBSD_kernel__)
+    !defined(__linux__) && !defined(__FreeBSD_kernel__) && !defined(_AIX)
 #include <sys/statvfs.h>
 #define STATVFS statvfs
 #define FSTATVFS fstatvfs
 #define STATVFS_F_FRSIZE(vfs) vfs.f_frsize
 #else
 #if defined(__OpenBSD__) || defined(__FreeBSD__)
 #include <sys/mount.h>
 #include <sys/param.h>
 #elif defined(__linux__)
 #if defined(HAVE_LINUX_MAGIC_H)
 #include <linux/magic.h>
 #else
 #if defined(HAVE_LINUX_NFS_FS_H)
 #include <linux/nfs_fs.h>
 #endif
 #if defined(HAVE_LINUX_SMB_H)
 #include <linux/smb.h>
 #endif
 #endif
 #include <sys/vfs.h>
+#elif defined(_AIX)
+#include <sys/statfs.h>
+
+// <sys/vmount.h> depends on `uint` to be a typedef from <sys/types.h> to
+// `uint_t`; however, <sys/types.h> does not always declare `uint`. We provide
+// the typedef prior to including <sys/vmount.h> to work around this issue.
+typedef uint_t uint;
+#include <sys/vmount.h>
 #else
 #include <sys/mount.h>
 #endif
 #define STATVFS statfs
 #define FSTATVFS fstatfs
 #define STATVFS_F_FRSIZE(vfs) static_cast<uint64_t>(vfs.f_bsize)
 #endif
 

 }
 
 uint32_t file_status::getLinkCount() const {
   return fs_st_nlinks;
 }
 
 ErrorOr<space_info> disk_space(const Twine &Path) {
   struct STATVFS Vfs;
-  if (::STATVFS(Path.str().c_str(), &Vfs))
+  if (::STATVFS(const_cast<char *>(Path.str().c_str()), &Vfs))
     return std::error_code(errno, std::generic_category());
   auto FrSize = STATVFS_F_FRSIZE(Vfs);
   space_info SpaceInfo;
   SpaceInfo.capacity = static_cast<uint64_t>(Vfs.f_blocks) * FrSize;
   SpaceInfo.free = static_cast<uint64_t>(Vfs.f_bfree) * FrSize;
   SpaceInfo.available = static_cast<uint64_t>(Vfs.f_bavail) * FrSize;
   return SpaceInfo;
 }

 #elif defined(__HAIKU__)
   // Haiku doesn't expose this information.
   return false;
 #elif defined(__sun)
   // statvfs::f_basetype contains a null-terminated FSType name of the mounted target
   StringRef fstype(Vfs.f_basetype);
   // NFS is the only non-local fstype??
   return !fstype.equals("nfs");
+#elif defined(_AIX)
+  // Call mntctl; try more than twice in case of timing issues with a concurrent
+  // mount.
+  int Ret;
+  size_t BufSize = 2048u;
+  std::unique_ptr<char[]> Buf;
+  int Tries = 3;
+  while (Tries--) {
+    Buf = llvm::make_unique<char[]>(BufSize);
+    Ret = mntctl(MCTL_QUERY, BufSize, Buf.get());
+    if (Ret != 0)
+      break;
+    BufSize = *reinterpret_cast<unsigned int *>(Buf.get());
+    Buf.reset();
+  }
+
+  if (Ret == -1)
+    // There was an error; "remote" is the conservative answer.
+    return false;
+
+  // Look for the correct vmount entry.
+  char *CurObjPtr = Buf.get();
+  while (Ret--) {
+    struct vmount *Vp = reinterpret_cast<struct vmount *>(CurObjPtr);
+    static_assert(sizeof(Vfs.f_fsid) == sizeof(Vp->vmt_fsid),
+                  "fsid length mismatch");
+    if (memcmp(&Vfs.f_fsid, &Vp->vmt_fsid, sizeof Vfs.f_fsid) == 0)
+      return (Vp->vmt_flags & MNT_REMOTE) == 0;
+
+    CurObjPtr += Vp->vmt_length;
+  }
+
+  // vmount entry not found; "remote" is the conservative answer.
+  return false;
 #else
   return !!(STATVFS_F_FLAG(Vfs) & MNT_LOCAL);
 #endif
 }
 
 std::error_code is_local(const Twine &Path, bool &Result) {
   struct STATVFS Vfs;
-  if (::STATVFS(Path.str().c_str(), &Vfs))
+  if (::STATVFS(const_cast<char *>(Path.str().c_str()), &Vfs))
     return std::error_code(errno, std::generic_category());
 
   Result = is_local_impl(Vfs);
   return std::error_code();
 }
 
 std::error_code is_local(int FD, bool &Result) {
   struct STATVFS Vfs;

llvm/trunk/unittests/Support/Path.cpp

TEST_F(FileSystemTest, ReadWriteFileCanReadOrWrite) {
   int FD;
   ASSERT_NO_ERROR(fs::openFileForReadWrite(NonExistantFile, FD,
                                            fs::CD_OpenExisting, fs::OF_None));
   FileDescriptorCloser Closer(FD);
   verifyRead(FD, "Fizz", true);
   verifyWrite(FD, "Buzz", true);
 }
 
+TEST_F(FileSystemTest, is_local) {
+  bool TestDirectoryIsLocal;
+  ASSERT_NO_ERROR(fs::is_local(TestDirectory, TestDirectoryIsLocal));
+  EXPECT_EQ(TestDirectoryIsLocal, fs::is_local(TestDirectory));
+
+  int FD;
+  SmallString<128> TempPath;
+  ASSERT_NO_ERROR(
+      fs::createUniqueFile(Twine(TestDirectory) + "/temp", FD, TempPath));
+  FileRemover Cleanup(TempPath);
+
+  // Make sure it exists.
+  ASSERT_TRUE(sys::fs::exists(Twine(TempPath)));
+
+  bool TempFileIsLocal;
+  ASSERT_NO_ERROR(fs::is_local(FD, TempFileIsLocal));
+  EXPECT_EQ(TempFileIsLocal, fs::is_local(FD));
+
+  // Expect that the file and its parent directory are equally local or equally
+  // remote.
+  EXPECT_EQ(TestDirectoryIsLocal, TempFileIsLocal);
+}
+
 TEST_F(FileSystemTest, set_current_path) {
   SmallString<128> path;
 
   ASSERT_NO_ERROR(fs::current_path(path));
   ASSERT_NE(TestDirectory, path);
 
   struct RestorePath {
     SmallString<128> path;