From 77383a4d5525a135f0f3d6ed6d41ae94adb88a18 Mon Sep 17 00:00:00 2001 From: Gautham B A Date: Tue, 10 Aug 2021 21:53:28 +0530 Subject: [PATCH] HDFS-15976. Make mkdtemp cross platform (#2908) --- .../native/libhdfspp/lib/x-platform/syscall.h | 18 +++++++- .../libhdfspp/lib/x-platform/syscall_linux.cc | 10 +++- .../lib/x-platform/syscall_windows.cc | 19 +++++++- .../libhdfspp/tests/configuration_test.cc | 15 +++--- .../libhdfspp/tests/configuration_test.h | 46 ++++++++++++++----- .../libhdfspp/tests/hdfs_builder_test.cc | 13 ++++-- .../tests/hdfs_config_connect_bugs.cc | 4 +- .../tests/hdfs_configuration_test.cc | 26 +++++------ .../tests/x-platform/syscall_common_test.cc | 12 +++++ 9 files changed, 120 insertions(+), 43 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall.h b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall.h index 9959f215b20..e17eba42162 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall.h +++ b/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& pattern); + private: static bool WriteToStdoutImpl(const char* message); }; diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_linux.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_linux.cc index ff02d2fa65c..15ec620c9ab 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_linux.cc +++ b/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& 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& pattern) { + // Append NULL so that mkdtemp can find the end of string + pattern.emplace_back('\0'); + return mkdtemp(pattern.data()) != nullptr; +} diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_windows.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_windows.cc index b5ddd043738..cdbcff2d4e4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_windows.cc +++ b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/syscall_windows.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -77,8 +78,8 @@ int XPlatform::Syscall::CreateAndOpenTempFile(std::vector& 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& pattern) { bool XPlatform::Syscall::CloseFile(const int file_descriptor) { return _close(file_descriptor) == 0; } + +bool XPlatform::Syscall::CreateTempDir(std::vector& 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; +} diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.cc index 3bf2524354b..d25f0a26f2f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.cc +++ b/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 config = config_loader.LoadFromFile(tempDir.path); + optional config = config_loader.LoadFromFile(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 config1 = loader.LoadFromFile("file1.xml"); EXPECT_TRUE(config1 && "Parse first stream"); optional 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 config = loader.LoadDefaultResources(); EXPECT_TRUE(config && "Parse streams"); diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.h b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.h index 23fc0d31047..11ca460d2cd 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.h +++ b/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 + #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 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(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(!path.empty()) - nftw(path.c_str(), nftw_remove, 64, FTW_DEPTH | FTW_PHYS); + 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 diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_builder_test.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_builder_test.cc index 147cfee6be8..8b3ef3c660c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_builder_test.cc +++ b/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); diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_config_connect_bugs.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_config_connect_bugs.cc index fc3122796a4..e7de62b9f97 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_config_connect_bugs.cc +++ b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_config_connect_bugs.cc @@ -23,10 +23,10 @@ #include #include +#include #include #include - static const char *hdfs_11294_core_site_txt = "\n" " \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"; diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_configuration_test.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_configuration_test.cc index 4e1bc3b65ae..dc2c0c20d6c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/hdfs_configuration_test.cc +++ b/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 config = loader.LoadDefaultResources(); 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 config = loader.LoadDefaultResources(); 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 config = loader.LoadDefaultResources(); 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", "")); diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/x-platform/syscall_common_test.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/x-platform/syscall_common_test.cc index a7847e1db35..c46f4dd66b4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/x-platform/syscall_common_test.cc +++ b/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 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 pattern_vec(pattern.begin(), pattern.end()); + EXPECT_FALSE(XPlatform::Syscall::CreateTempDir(pattern_vec)); +}