HDFS-15976. Make mkdtemp cross platform (#2908)

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall.h

@@ -89,7 +89,7 @@ class Syscall {
    * Creates and opens a temporary file with a given {@link pattern}.
    * The {@link pattern} must end with a minimum of 6 'X' characters.
    * This function will first modify the last 6 'X' characters with
-   * random character values, which serve as the temporary file name.
+   * random character values, which serve as the temporary file name
    * Subsequently opens the file and returns the file descriptor for
    * the same. The behaviour of this function is the same as that of
    * POSIX mkstemp function. The file must be later closed by the
@@ -113,6 +113,22 @@ class Syscall {
    */
   static bool CloseFile(int file_descriptor);
 
+  /**
+   * Creates and opens a temporary file with a given {@link pattern}.
+   * The {@link pattern} must end with a minimum of 6 'X' characters.
+   * This function will first modify the last 6 'X' characters with
+   * random character values, which serve as the temporary file name.
+   * Subsequently opens the file and returns the file descriptor for
+   * the same. The behaviour of this function is the same as that of
+   * POSIX mkstemp function. The file must be later closed by the
+   * application and is not handled by this function.
+   *
+   * @param pattern the pattern to be used for the temporary filename.
+   * @returns true if the creation of directory succeeds, false
+   * otherwise.
+   */
+  static bool CreateTempDir(std::vector<char>& pattern);
+
  private:
   static bool WriteToStdoutImpl(const char* message);
 };

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_linux.cc

@@ -62,11 +62,17 @@ bool XPlatform::Syscall::StringCompareIgnoreCase(const std::string& a,
 }
 
 int XPlatform::Syscall::CreateAndOpenTempFile(std::vector<char>& pattern) {
-  // Make space for mkstemp to add NULL character at the end
-  pattern.resize(pattern.size() + 1);
+  // Append NULL so that mkstemp can find the end of string
+  pattern.emplace_back('\0');
   return mkstemp(pattern.data());
 }
 
 bool XPlatform::Syscall::CloseFile(const int file_descriptor) {
   return close(file_descriptor) == 0;
 }
+
+bool XPlatform::Syscall::CreateTempDir(std::vector<char>& pattern) {
+  // Append NULL so that mkdtemp can find the end of string
+  pattern.emplace_back('\0');
+  return mkdtemp(pattern.data()) != nullptr;
+}

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_windows.cc

@@ -19,6 +19,7 @@
 #include <Shlwapi.h>
 #include <WinBase.h>
 #include <Windows.h>
+#include <direct.h>
 #include <fcntl.h>
 #include <io.h>
 #include <share.h>
@@ -77,8 +78,8 @@ int XPlatform::Syscall::CreateAndOpenTempFile(std::vector<char>& pattern) {
     return -1;
   }
 
-  // Make space for _mktemp_s to add NULL character at the end
-  pattern.resize(pattern.size() + 1);
+  // Append NULL so that _mktemp_s can find the end of string
+  pattern.emplace_back('\0');
   if (_mktemp_s(pattern.data(), pattern.size()) != 0) {
     return -1;
   }
@@ -94,3 +95,17 @@ int XPlatform::Syscall::CreateAndOpenTempFile(std::vector<char>& pattern) {
 bool XPlatform::Syscall::CloseFile(const int file_descriptor) {
   return _close(file_descriptor) == 0;
 }
+
+bool XPlatform::Syscall::CreateTempDir(std::vector<char>& pattern) {
+  if (_set_errno(0) != 0) {
+    return false;
+  }
+
+  // Append NULL so that _mktemp_s can find the end of string
+  pattern.emplace_back('\0');
+  if (_mktemp_s(pattern.data(), pattern.size()) != 0) {
+    return false;
+  }
+
+  return _mkdir(pattern.data()) == 0;
+}

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.cc

@@ -335,7 +335,7 @@ TEST(ConfigurationTest, TestFileReads)
 
     ConfigurationLoader config_loader;
     config_loader.ClearSearchPath();
-    optional<Configuration> config = config_loader.LoadFromFile<Configuration>(tempDir.path);
+    optional<Configuration> config = config_loader.LoadFromFile<Configuration>(tempDir.GetPath());
     EXPECT_FALSE(config && "Add directory as file resource");
   }
 
@@ -352,17 +352,18 @@ TEST(ConfigurationTest, TestFileReads)
   // Search path
   {
     TempDir tempDir1;
-    TempFile tempFile1(tempDir1.path + "/file1.xml");
+    TempFile tempFile1(tempDir1.GetPath() + "/file1.xml");
     writeSimpleConfig(tempFile1.GetFileName(), "key1", "value1");
     TempDir tempDir2;
-    TempFile tempFile2(tempDir2.path + "/file2.xml");
+    TempFile tempFile2(tempDir2.GetPath() + "/file2.xml");
     writeSimpleConfig(tempFile2.GetFileName(), "key2", "value2");
     TempDir tempDir3;
-    TempFile tempFile3(tempDir3.path + "/file3.xml");
+    TempFile tempFile3(tempDir3.GetPath() + "/file3.xml");
     writeSimpleConfig(tempFile3.GetFileName(), "key3", "value3");
 
     ConfigurationLoader loader;
-    loader.SetSearchPath(tempDir1.path + ":" + tempDir2.path + ":" + tempDir3.path);
+    loader.SetSearchPath(tempDir1.GetPath() + ":" + tempDir2.GetPath() + ":" +
+                         tempDir3.GetPath());
     optional<Configuration> config1 = loader.LoadFromFile<Configuration>("file1.xml");
     EXPECT_TRUE(config1 && "Parse first stream");
     optional<Configuration> config2 = loader.OverlayResourceFile(*config1, "file2.xml");
@@ -379,11 +380,11 @@ TEST(ConfigurationTest, TestDefaultConfigs) {
   // Search path
   {
     TempDir tempDir;
-    TempFile coreSite(tempDir.path + "/core-site.xml");
+    TempFile coreSite(tempDir.GetPath() + "/core-site.xml");
     writeSimpleConfig(coreSite.GetFileName(), "key1", "value1");
 
     ConfigurationLoader loader;
-    loader.SetSearchPath(tempDir.path);
+    loader.SetSearchPath(tempDir.GetPath());
 
     optional<Configuration> config = loader.LoadDefaultResources<Configuration>();
     EXPECT_TRUE(config && "Parse streams");

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.h

@@ -18,6 +18,8 @@
 #ifndef TESTS_CONFIGURATION_H_
 #define TESTS_CONFIGURATION_H_
 
+#include <algorithm>
+
 #include "hdfspp/config_parser.h"
 #include "common/configuration.h"
 #include "common/configuration_loader.h"
@@ -173,22 +175,44 @@ int nftw_remove(const char *fpath, const struct stat *sb, int typeflag, struct F
 
 // TempDir: is created in ctor and recursively deletes in dtor
 class TempDir {
-public:
-  std::string path;
+ public:
   TempDir() {
-    char        fn_buffer[128];
-    strncpy(fn_buffer, "/tmp/test_dir_XXXXXXXXXX", sizeof(fn_buffer));
-    const char * returned_path = mkdtemp(fn_buffer);
-    EXPECT_NE(nullptr, returned_path);
-    path = returned_path;
+    std::vector<char> path_pattern(path_.begin(), path_.end());
+    is_path_init_ = XPlatform::Syscall::CreateTempDir(path_pattern);
+    EXPECT_TRUE(is_path_init_);
+    path_.assign(path_pattern.data());
   }
-  ~TempDir() {
-    if(!path.empty())
-      nftw(path.c_str(), nftw_remove, 64, FTW_DEPTH | FTW_PHYS);
+
+  TempDir(const TempDir& other) = default;
+
+  TempDir(TempDir&& other) noexcept : path_{std::move(other.path_)} {}
+
+  TempDir& operator=(const TempDir& other) {
+    if (&other != this) {
+      path_ = other.path_;
+    }
+    return *this;
   }
-};
 
+  TempDir& operator=(TempDir&& other) noexcept {
+    if (&other != this) {
+      path_ = std::move(other.path_);
+    }
+    return *this;
+  }
+
+  [[nodiscard]] const std::string& GetPath() const { return path_; }
+
+  ~TempDir() {
+    if (is_path_init_) {
+      nftw(path_.c_str(), nftw_remove, 64, FTW_DEPTH | FTW_PHYS);
+    }
+  }
 
+ private:
+  std::string path_{"/tmp/test_dir_XXXXXXXXXX"};
+  bool is_path_init_{false};
+};
 }
 
 #endif

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_builder_test.cc

@@ -29,7 +29,8 @@ TEST(HdfsBuilderTest, TestStubBuilder) {
   {
     TempDir tempDir1;
 
-    hdfsBuilder * builder = hdfsNewBuilderFromDirectory(tempDir1.path.c_str());
+    hdfsBuilder *builder =
+        hdfsNewBuilderFromDirectory(tempDir1.GetPath().c_str());
     hdfsFreeBuilder(builder);
   }
 
@@ -44,10 +45,11 @@ TEST(HdfsBuilderTest, TestRead)
   // Reading string values
   {
     TempDir tempDir1;
-    TempFile tempFile1(tempDir1.path + "/core-site.xml");
+    TempFile tempFile1(tempDir1.GetPath() + "/core-site.xml");
     writeSimpleConfig(tempFile1.GetFileName(), "key1", "value1");
 
-    hdfsBuilder * builder = hdfsNewBuilderFromDirectory(tempDir1.path.c_str());
+    hdfsBuilder *builder =
+        hdfsNewBuilderFromDirectory(tempDir1.GetPath().c_str());
 
     char * readVal = nullptr;
     int result = hdfsBuilderConfGetStr(builder, "key1", &readVal);
@@ -67,10 +69,11 @@ TEST(HdfsBuilderTest, TestRead)
   // Reading int values
   {
     TempDir tempDir1;
-    TempFile tempFile1(tempDir1.path + "/core-site.xml");
+    TempFile tempFile1(tempDir1.GetPath() + "/core-site.xml");
     writeSimpleConfig(tempFile1.GetFileName(), "key1", "100");
 
-    hdfsBuilder * builder = hdfsNewBuilderFromDirectory(tempDir1.path.c_str());
+    hdfsBuilder *builder =
+        hdfsNewBuilderFromDirectory(tempDir1.GetPath().c_str());
 
     int readVal = -1;
     int result = hdfsBuilderConfGetInt(builder, "key1", &readVal);

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_config_connect_bugs.cc

@@ -23,10 +23,10 @@
 #include <google/protobuf/stubs/common.h>
 
 #include <cstring>
+#include <cstdio>
 #include <chrono>
 #include <exception>
 
-
 static const char *hdfs_11294_core_site_txt =
 "<configuration>\n"
 "  <property name=\"fs.defaultFS\" value=\"hdfs://NAMESERVICE1\"/>\n"
@@ -80,7 +80,7 @@ TEST(ConfigConnectBugs, Test_HDFS_11294) {
   // Directory for hdfs config
   TempDir td;
 
-  const std::string& tempDirPath = td.path;
+  const std::string &tempDirPath = td.GetPath();
   const std::string coreSitePath = tempDirPath + "/core-site.xml";
   const std::string hdfsSitePath = tempDirPath + "/hdfs-site.xml";
 

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_configuration_test.cc

@@ -71,13 +71,13 @@ TEST(HdfsConfigurationTest, TestDefaultConfigs) {
   // Search path
   {
     TempDir tempDir;
-    TempFile coreSite(tempDir.path + "/core-site.xml");
+    TempFile coreSite(tempDir.GetPath() + "/core-site.xml");
     writeSimpleConfig(coreSite.GetFileName(), "key1", "value1");
-    TempFile hdfsSite(tempDir.path + "/hdfs-site.xml");
+    TempFile hdfsSite(tempDir.GetPath() + "/hdfs-site.xml");
     writeSimpleConfig(hdfsSite.GetFileName(), "key2", "value2");
 
     ConfigurationLoader loader;
-    loader.SetSearchPath(tempDir.path);
+    loader.SetSearchPath(tempDir.GetPath());
 
     optional<HdfsConfiguration> config = loader.LoadDefaultResources<HdfsConfiguration>();
     EXPECT_TRUE(config && "Parse streams");
@@ -88,11 +88,11 @@ TEST(HdfsConfigurationTest, TestDefaultConfigs) {
   // Only core-site.xml available
   {
     TempDir tempDir;
-    TempFile coreSite(tempDir.path + "/core-site.xml");
+    TempFile coreSite(tempDir.GetPath() + "/core-site.xml");
     writeSimpleConfig(coreSite.GetFileName(), "key1", "value1");
 
     ConfigurationLoader loader;
-    loader.SetSearchPath(tempDir.path);
+    loader.SetSearchPath(tempDir.GetPath());
 
     optional<HdfsConfiguration> config = loader.LoadDefaultResources<HdfsConfiguration>();
     EXPECT_TRUE(config && "Parse streams");
@@ -102,11 +102,11 @@ TEST(HdfsConfigurationTest, TestDefaultConfigs) {
   // Only hdfs-site available
   {
     TempDir tempDir;
-    TempFile hdfsSite(tempDir.path + "/hdfs-site.xml");
+    TempFile hdfsSite(tempDir.GetPath() + "/hdfs-site.xml");
     writeSimpleConfig(hdfsSite.GetFileName(), "key2", "value2");
 
     ConfigurationLoader loader;
-    loader.SetSearchPath(tempDir.path);
+    loader.SetSearchPath(tempDir.GetPath());
 
     optional<HdfsConfiguration> config = loader.LoadDefaultResources<HdfsConfiguration>();
     EXPECT_TRUE(config && "Parse streams");
@@ -120,12 +120,12 @@ TEST(HdfsConfigurationTest, TestConfigParserAPI) {
   // Config parser API
   {
     TempDir tempDir;
-    TempFile coreSite(tempDir.path + "/core-site.xml");
+    TempFile coreSite(tempDir.GetPath() + "/core-site.xml");
     writeSimpleConfig(coreSite.GetFileName(), "key1", "value1");
-    TempFile hdfsSite(tempDir.path + "/hdfs-site.xml");
+    TempFile hdfsSite(tempDir.GetPath() + "/hdfs-site.xml");
     writeSimpleConfig(hdfsSite.GetFileName(), "key2", "value2");
 
-    ConfigParser parser(tempDir.path);
+    ConfigParser parser(tempDir.GetPath());
 
     EXPECT_EQ("value1", parser.get_string_or("key1", ""));
     EXPECT_EQ("value2", parser.get_string_or("key2", ""));
@@ -141,12 +141,12 @@ TEST(HdfsConfigurationTest, TestConfigParserAPI) {
 
   {
     TempDir tempDir;
-    TempFile coreSite(tempDir.path + "/core-site.xml");
+    TempFile coreSite(tempDir.GetPath() + "/core-site.xml");
     writeSimpleConfig(coreSite.GetFileName(), "key1", "value1");
-    TempFile hdfsSite(tempDir.path + "/hdfs-site.xml");
+    TempFile hdfsSite(tempDir.GetPath() + "/hdfs-site.xml");
     writeDamagedConfig(hdfsSite.GetFileName(), "key2", "value2");
 
-    ConfigParser parser(tempDir.path);
+    ConfigParser parser(tempDir.GetPath());
 
     EXPECT_EQ("value1", parser.get_string_or("key1", ""));
     EXPECT_EQ("", parser.get_string_or("key2", ""));

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/x-platform/syscall_common_test.cc

@@ -103,3 +103,15 @@ TEST(XPlatformSyscall, CreateAndOpenTempFileNegative) {
   EXPECT_EQ(fd, -1);
   EXPECT_FALSE(XPlatform::Syscall::CloseFile(fd));
 }
+
+TEST(XPlatformSyscall, CreateTempDirBasic) {
+  std::string pattern("/tmp/tmp-XXXXXX");
+  std::vector<char> pattern_vec(pattern.begin(), pattern.end());
+  EXPECT_TRUE(XPlatform::Syscall::CreateTempDir(pattern_vec));
+}
+
+TEST(XPlatformSyscall, CreateTempDirNegative) {
+  std::string pattern("does-not-adhere-to-pattern");
+  std::vector<char> pattern_vec(pattern.begin(), pattern.end());
+  EXPECT_FALSE(XPlatform::Syscall::CreateTempDir(pattern_vec));
+}