[libc] Refactor statvfs tests

[michaelrj-google]

The previous statvfs tests had several issues, this patch updates them
to meet current standards.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Michael Jones (michaelrj-google)

Changes

The previous statvfs tests had several issues, this patch updates them
to meet current standards.

---

Full diff: https://github.com/llvm/llvm-project/pull/114147.diff

3 Files Affected:

• (modified) libc/test/src/sys/statvfs/linux/CMakeLists.txt (+4-2)
• (modified) libc/test/src/sys/statvfs/linux/fstatvfs_test.cpp (+44-37)
• (modified) libc/test/src/sys/statvfs/linux/statvfs_test.cpp (+30-41)

diff --git a/libc/test/src/sys/statvfs/linux/CMakeLists.txt b/libc/test/src/sys/statvfs/linux/CMakeLists.txt
index 1f8688868e0438..fa1e9052d1cac4 100644
--- a/libc/test/src/sys/statvfs/linux/CMakeLists.txt
+++ b/libc/test/src/sys/statvfs/linux/CMakeLists.txt
@@ -8,8 +8,9 @@ add_libc_unittest(
     statvfs_test.cpp
   DEPENDS
     libc.src.errno.errno
-    libc.src.sys.statvfs.linux.statfs_utils
     libc.src.sys.statvfs.statvfs
+    libc.src.sys.stat.mkdirat
+    libc.src.sys.stat.rmdir
     libc.test.UnitTest.ErrnoSetterMatcher
 )
 
@@ -21,8 +22,9 @@ add_libc_unittest(
     fstatvfs_test.cpp
   DEPENDS
     libc.src.errno.errno
-    libc.src.sys.statvfs.linux.statfs_utils
     libc.src.sys.statvfs.fstatvfs
+    libc.src.sys.stat.mkdirat
+    libc.src.sys.stat.rmdir
     libc.src.fcntl.open
     libc.src.unistd.close
     libc.test.UnitTest.ErrnoSetterMatcher
diff --git a/libc/test/src/sys/statvfs/linux/fstatvfs_test.cpp b/libc/test/src/sys/statvfs/linux/fstatvfs_test.cpp
index 2f3e0b96ff0957..d14df58534e3eb 100644
--- a/libc/test/src/sys/statvfs/linux/fstatvfs_test.cpp
+++ b/libc/test/src/sys/statvfs/linux/fstatvfs_test.cpp
@@ -1,49 +1,56 @@
+//===-- Unittests for fstatvfs --------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
 #include "hdr/fcntl_macros.h"
 #include "src/__support/macros/config.h"
 #include "src/fcntl/open.h"
+#include "src/sys/stat/mkdirat.h"
 #include "src/sys/statvfs/fstatvfs.h"
-#include "src/sys/statvfs/linux/statfs_utils.h"
 #include "src/unistd/close.h"
+#include "src/unistd/rmdir.h"
 #include "test/UnitTest/ErrnoSetterMatcher.h"
-#include "test/UnitTest/LibcTest.h"
-#include <linux/magic.h>
+#include "test/UnitTest/Test.h"
+
 using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
 
-#ifdef SYS_statfs64
-using StatFs = statfs64;
-#else
-using StatFs = statfs;
-#endif
-
-namespace LIBC_NAMESPACE_DECL {
-static int fstatfs(int fd, StatFs *buf) {
-  using namespace statfs_utils;
-  if (cpp::optional<StatFs> result = linux_fstatfs(fd)) {
-    *buf = *result;
-    return 0;
-  }
-  return -1;
-}
-} // namespace LIBC_NAMESPACE_DECL
-
-struct PathFD {
-  int fd;
-  explicit PathFD(const char *path)
-      : fd(LIBC_NAMESPACE::open(path, O_CLOEXEC | O_PATH)) {}
-  ~PathFD() { LIBC_NAMESPACE::close(fd); }
-  operator int() const { return fd; }
-};
-
-TEST(LlvmLibcSysStatvfsTest, FstatfsBasic) {
-  StatFs buf;
-  ASSERT_THAT(LIBC_NAMESPACE::fstatfs(PathFD("/"), &buf), Succeeds());
-  ASSERT_THAT(LIBC_NAMESPACE::fstatfs(PathFD("/proc"), &buf), Succeeds());
-  ASSERT_EQ(buf.f_type, static_cast<decltype(buf.f_type)>(PROC_SUPER_MAGIC));
-  ASSERT_THAT(LIBC_NAMESPACE::fstatfs(PathFD("/sys"), &buf), Succeeds());
-  ASSERT_EQ(buf.f_type, static_cast<decltype(buf.f_type)>(SYSFS_MAGIC));
+TEST(LlvmLibcSysFStatfsTest, FStatfsBasic) {
+  struct statvfs buf;
+
+  int fd = LIBC_NAMESPACE::open("/", O_PATH);
+  ASSERT_ERRNO_SUCCESS();
+  ASSERT_GT(fd, 0);
+
+  // The root of the file directory must always exist
+  ASSERT_THAT(LIBC_NAMESPACE::fstatvfs(fd, &buf), Succeeds());
+  ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
 }
 
-TEST(LlvmLibcSysStatvfsTest, FstatvfsInvalidFD) {
+TEST(LlvmLibcSysFStatfsTest, FStatvfsInvalidPath) {
   struct statvfs buf;
-  ASSERT_THAT(LIBC_NAMESPACE::fstatvfs(-1, &buf), Fails(EBADF));
+
+  constexpr const char *FILENAME = "testdata/statvfs.testdir";
+  auto TEST_DIR = libc_make_test_file_path(FILENAME);
+
+  ASSERT_THAT(LIBC_NAMESPACE::mkdirat(AT_FDCWD, TEST_DIR, S_IRWXU),
+              Succeeds(0));
+
+  int fd = LIBC_NAMESPACE::open(TEST_DIR, O_PATH);
+  ASSERT_ERRNO_SUCCESS();
+  ASSERT_GT(fd, 0);
+
+  // create the file, assert it exists, then delete it and assert it doesn't
+  // exist anymore.
+
+  ASSERT_THAT(LIBC_NAMESPACE::fstatvfs(fd, &buf), Succeeds());
+
+  ASSERT_THAT(LIBC_NAMESPACE::rmdir(TEST_DIR), Succeeds(0));
+
+  ASSERT_THAT(LIBC_NAMESPACE::fstatvfs(fd, &buf), Fails(ENOENT));
+  ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+  ASSERT_THAT(LIBC_NAMESPACE::fstatvfs(fd, &buf), Fails(ENOENT));
 }
diff --git a/libc/test/src/sys/statvfs/linux/statvfs_test.cpp b/libc/test/src/sys/statvfs/linux/statvfs_test.cpp
index 5329adb54d64d0..b4e1d5a3297d9a 100644
--- a/libc/test/src/sys/statvfs/linux/statvfs_test.cpp
+++ b/libc/test/src/sys/statvfs/linux/statvfs_test.cpp
@@ -1,54 +1,43 @@
+//===-- Unittests for statvfs ---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "hdr/fcntl_macros.h"
 #include "src/__support/macros/config.h"
-#include "src/sys/statvfs/linux/statfs_utils.h"
+#include "src/sys/stat/mkdirat.h"
 #include "src/sys/statvfs/statvfs.h"
+#include "src/unistd/rmdir.h"
 #include "test/UnitTest/ErrnoSetterMatcher.h"
-#include "test/UnitTest/LibcTest.h"
-#include <linux/magic.h>
-using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
+#include "test/UnitTest/Test.h"
 
-#ifdef SYS_statfs64
-using StatFs = statfs64;
-#else
-using StatFs = statfs;
-#endif
-
-namespace LIBC_NAMESPACE_DECL {
-static int statfs(const char *path, StatFs *buf) {
-  using namespace statfs_utils;
-  if (cpp::optional<LinuxStatFs> result = linux_statfs(path)) {
-    *buf = *result;
-    return 0;
-  }
-  return -1;
-}
-} // namespace LIBC_NAMESPACE_DECL
+using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
 
 TEST(LlvmLibcSysStatfsTest, StatfsBasic) {
-  StatFs buf;
-  ASSERT_THAT(LIBC_NAMESPACE::statfs("/", &buf), Succeeds());
-  ASSERT_THAT(LIBC_NAMESPACE::statfs("/proc", &buf), Succeeds());
-  ASSERT_EQ(buf.f_type, static_cast<decltype(buf.f_type)>(PROC_SUPER_MAGIC));
-  ASSERT_THAT(LIBC_NAMESPACE::statfs("/sys", &buf), Succeeds());
-  ASSERT_EQ(buf.f_type, static_cast<decltype(buf.f_type)>(SYSFS_MAGIC));
+  struct statvfs buf;
+  // The root of the file directory must always exist
+  ASSERT_THAT(LIBC_NAMESPACE::statvfs("/", &buf), Succeeds());
 }
 
 TEST(LlvmLibcSysStatfsTest, StatvfsInvalidPath) {
   struct statvfs buf;
+
   ASSERT_THAT(LIBC_NAMESPACE::statvfs("", &buf), Fails(ENOENT));
-  ASSERT_THAT(LIBC_NAMESPACE::statvfs("/nonexistent", &buf), Fails(ENOENT));
-  ASSERT_THAT(LIBC_NAMESPACE::statvfs("/dev/null/whatever", &buf),
-              Fails(ENOTDIR));
-  ASSERT_THAT(LIBC_NAMESPACE::statvfs(nullptr, &buf), Fails(EFAULT));
-}
 
-TEST(LlvmLibcSysStatfsTest, StatvfsNameTooLong) {
-  struct statvfs buf;
-  ASSERT_THAT(LIBC_NAMESPACE::statvfs("/", &buf), Succeeds());
-  char *name = static_cast<char *>(__builtin_alloca(buf.f_namemax + 3));
-  name[0] = '/';
-  name[buf.f_namemax + 2] = '\0';
-  for (unsigned i = 1; i < buf.f_namemax + 2; ++i) {
-    name[i] = 'a';
-  }
-  ASSERT_THAT(LIBC_NAMESPACE::statvfs(name, &buf), Fails(ENAMETOOLONG));
+  // create the file, assert it exists, then delete it and assert it doesn't
+  // exist anymore.
+  constexpr const char *FILENAME = "testdata/statvfs.testdir";
+  auto TEST_DIR = libc_make_test_file_path(FILENAME);
+
+  ASSERT_THAT(LIBC_NAMESPACE::mkdirat(AT_FDCWD, TEST_DIR, S_IRWXU),
+              Succeeds(0));
+
+  ASSERT_THAT(LIBC_NAMESPACE::statvfs(TEST_DIR, &buf), Succeeds());
+
+  ASSERT_THAT(LIBC_NAMESPACE::rmdir(TEST_DIR), Succeeds(0));
+
+  ASSERT_THAT(LIBC_NAMESPACE::statvfs(TEST_DIR, &buf), Fails(ENOENT));
 }

[SchrodingerZhu]

The change looks good to me. But I would like a more detailed explanation of the purposes. Is it for portability? Notice that original tests were inside linux directory so linux specific features was tested.

[michaelrj-google]

There are two main reasons I wanted to refactor this. The first is the extra functions being defined and tested in the tests, which aren't actually related to what the user will see. The second is that the global paths were inconsistent. Basically, the test would fail if the user had /nonexistent/ on their system. Somehow the test was also causing this folder to be created on some workstations, which is also bad. It's much better to have paths that we can be confident we control.