[llvm] r185126 - Improvements to unique_file and createUniqueDirectory.

Rafael Espindola rafael.espindola at gmail.com
Thu Jun 27 20:48:48 PDT 2013


Author: rafael
Date: Thu Jun 27 22:48:47 2013
New Revision: 185126

URL: http://llvm.org/viewvc/llvm-project?rev=185126&view=rev
Log:
Improvements to unique_file and createUniqueDirectory.

* Don't try to create parent directories in unique_file. It had two problem:
   * It violates the contract that it is atomic. If the directory creation
     success and the file creation fails, we would return an error but the
     file system was modified.
   * When creating a temporary file clang would have to first check if the
     parent directory existed or not to avoid creating one when it was not
     supposed to.

* More efficient implementations of createUniqueDirectory and the unique_file
  that produces only the file name. Now all 3 just call into a static
  function passing what they want (name, file or directory).

Clang also has to be updated, so tests might fail if a bot picks up this commit
and not the corresponding clang one.

Modified:
    llvm/trunk/lib/Support/Path.cpp
    llvm/trunk/lib/Support/Unix/Path.inc
    llvm/trunk/lib/Support/Windows/Path.inc

Modified: llvm/trunk/lib/Support/Path.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Path.cpp?rev=185126&r1=185125&r2=185126&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Path.cpp (original)
+++ llvm/trunk/lib/Support/Path.cpp Thu Jun 27 22:48:47 2013
@@ -153,6 +153,18 @@ namespace {
   }
 } // end unnamed namespace
 
+enum FSEntity {
+  FS_Dir,
+  FS_File,
+  FS_Name
+};
+
+// Implemented in Unix/Path.inc and Windows/Path.inc.
+static llvm::error_code
+createUniqueEntity(const llvm::Twine &Model, int &ResultFD,
+                   llvm::SmallVectorImpl<char> &ResultPath,
+                   bool MakeAbsolute, unsigned Mode, FSEntity Type);
+
 namespace llvm {
 namespace sys  {
 namespace path {
@@ -625,32 +637,31 @@ bool is_relative(const Twine &path) {
 
 namespace fs {
 
+// This is a mkostemps with a different pattern. Unfortunatelly OS X (ond *BSD)
+// don't have it. It might be worth experimenting with mkostemps on systems
+// that have it.
+error_code unique_file(const Twine &Model, int &ResultFD,
+                       SmallVectorImpl<char> &ResultPath, bool MakeAbsolute,
+                       unsigned Mode) {
+  return createUniqueEntity(Model, ResultFD, ResultPath, MakeAbsolute, Mode,
+                            FS_File);
+}
+
+// This is a mktemp with a differet pattern. We use createUniqueEntity mostly
+// for consistency. It might be worth it experimenting with mktemp.
 error_code unique_file(const Twine &Model, SmallVectorImpl<char> &ResultPath,
                        bool MakeAbsolute) {
-  // FIXME: This is really inefficient. unique_path creates a path an tries to
-  // open it. We should factor the code so that we just don't create/open the
-  // file when we don't need it.
-  int FD;
-  error_code Ret = unique_file(Model, FD, ResultPath, MakeAbsolute, all_read);
-  if (Ret)
-    return Ret;
-
-  if (close(FD))
-    return error_code(errno, system_category());
-
-  StringRef P(ResultPath.begin(), ResultPath.size());
-  return fs::remove(P);
+  int Dummy;
+  return createUniqueEntity(Model, Dummy, ResultPath, MakeAbsolute, 0, FS_Name);
 }
 
+// This is a mkdtemp with a different pattern. We use createUniqueEntity mostly
+// for consistency. It might be worth it experimenting with mkdtemp.
 error_code createUniqueDirectory(const Twine &Prefix,
                                  SmallVectorImpl<char> &ResultPath) {
-  // FIXME: This is double inefficient. We compute a unique file name, created
-  // it, delete it and keep only the directory.
-  error_code EC = unique_file(Prefix + "-%%%%%%/dummy", ResultPath);
-  if (EC)
-    return EC;
-  path::remove_filename(ResultPath);
-  return error_code::success();
+  int Dummy;
+  return createUniqueEntity(Prefix + "-%%%%%%", Dummy, ResultPath,
+                            true, 0, FS_Dir);
 }
 
 error_code make_absolute(SmallVectorImpl<char> &path) {

Modified: llvm/trunk/lib/Support/Unix/Path.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Unix/Path.inc?rev=185126&r1=185125&r2=185126&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Unix/Path.inc (original)
+++ llvm/trunk/lib/Support/Unix/Path.inc Thu Jun 27 22:48:47 2013
@@ -110,6 +110,76 @@ namespace {
   }
 }
 
+static error_code createUniqueEntity(const Twine &Model, int &ResultFD,
+                                     SmallVectorImpl<char> &ResultPath,
+                                     bool MakeAbsolute, unsigned Mode,
+                                     FSEntity Type) {
+  SmallString<128> ModelStorage;
+  Model.toVector(ModelStorage);
+
+  if (MakeAbsolute) {
+    // Make model absolute by prepending a temp directory if it's not already.
+    bool absolute = sys::path::is_absolute(Twine(ModelStorage));
+    if (!absolute) {
+      SmallString<128> TDir;
+      if (error_code ec = TempDir(TDir)) return ec;
+      sys::path::append(TDir, Twine(ModelStorage));
+      ModelStorage.swap(TDir);
+    }
+  }
+
+  // From here on, DO NOT modify model. It may be needed if the randomly chosen
+  // path already exists.
+  ResultPath = ModelStorage;
+  // Null terminate.
+  ResultPath.push_back(0);
+  ResultPath.pop_back();
+
+retry_random_path:
+  // Replace '%' with random chars.
+  for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
+    if (ModelStorage[i] == '%')
+      ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
+  }
+
+  // Try to open + create the file.
+  switch (Type) {
+  case FS_File: {
+    int RandomFD = ::open(ResultPath.begin(), O_RDWR | O_CREAT | O_EXCL, Mode);
+    if (RandomFD == -1) {
+      int SavedErrno = errno;
+      // If the file existed, try again, otherwise, error.
+      if (SavedErrno == errc::file_exists)
+        goto retry_random_path;
+      return error_code(SavedErrno, system_category());
+    }
+
+    ResultFD = RandomFD;
+    return error_code::success();
+  }
+
+  case FS_Name: {
+    bool Exists;
+    error_code EC = sys::fs::exists(ResultPath.begin(), Exists);
+    if (EC)
+      return EC;
+    if (Exists)
+      goto retry_random_path;
+    return error_code::success();
+  }
+
+  case FS_Dir: {
+    bool Existed;
+    error_code EC = sys::fs::create_directory(ResultPath.begin(), Existed);
+    if (EC)
+      return EC;
+    if (Existed)
+      goto retry_random_path;
+    return error_code::success();
+  }
+  }
+}
+
 namespace llvm {
 namespace sys  {
 namespace fs {
@@ -564,92 +634,6 @@ error_code setLastModificationAndAccessT
   return error_code::success();
 }
 
-error_code unique_file(const Twine &model, int &result_fd,
-                       SmallVectorImpl<char> &result_path,
-                       bool makeAbsolute, unsigned mode) {
-  SmallString<128> Model;
-  model.toVector(Model);
-  // Null terminate.
-  Model.c_str();
-
-  if (makeAbsolute) {
-    // Make model absolute by prepending a temp directory if it's not already.
-    bool absolute = path::is_absolute(Twine(Model));
-    if (!absolute) {
-      SmallString<128> TDir;
-      if (error_code ec = TempDir(TDir)) return ec;
-      path::append(TDir, Twine(Model));
-      Model.swap(TDir);
-    }
-  }
-
-  // From here on, DO NOT modify model. It may be needed if the randomly chosen
-  // path already exists.
-  SmallString<128> RandomPath = Model;
-
-retry_random_path:
-  // Replace '%' with random chars.
-  for (unsigned i = 0, e = Model.size(); i != e; ++i) {
-    if (Model[i] == '%')
-      RandomPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
-  }
-
-  // Make sure we don't fall into an infinite loop by constantly trying
-  // to create the parent path.
-  bool TriedToCreateParent = false;
-
-  // Try to open + create the file.
-rety_open_create:
-  int RandomFD = ::open(RandomPath.c_str(), O_RDWR | O_CREAT | O_EXCL, mode);
-  if (RandomFD == -1) {
-    int SavedErrno = errno;
-    // If the file existed, try again, otherwise, error.
-    if (SavedErrno == errc::file_exists)
-      goto retry_random_path;
-    // If path prefix doesn't exist, try to create it.
-    if (SavedErrno == errc::no_such_file_or_directory && !TriedToCreateParent) {
-      TriedToCreateParent = true;
-      StringRef p(RandomPath);
-      SmallString<64> dir_to_create;
-      for (path::const_iterator i = path::begin(p),
-                                e = --path::end(p); i != e; ++i) {
-        path::append(dir_to_create, *i);
-        bool Exists;
-        if (error_code ec = exists(Twine(dir_to_create), Exists)) return ec;
-        if (!Exists) {
-          // Don't try to create network paths.
-          if (i->size() > 2 && (*i)[0] == '/' &&
-                               (*i)[1] == '/' &&
-                               (*i)[2] != '/')
-            return make_error_code(errc::no_such_file_or_directory);
-          if (::mkdir(dir_to_create.c_str(), 0700) == -1 &&
-              errno != errc::file_exists)
-            return error_code(errno, system_category());
-        }
-      }
-      goto rety_open_create;
-    }
-
-    return error_code(SavedErrno, system_category());
-  }
-
-   // Make the path absolute.
-  char real_path_buff[PATH_MAX + 1];
-  if (realpath(RandomPath.c_str(), real_path_buff) == NULL) {
-    int error = errno;
-    ::close(RandomFD);
-    ::unlink(RandomPath.c_str());
-    return error_code(error, system_category());
-  }
-
-  result_path.clear();
-  StringRef d(real_path_buff);
-  result_path.append(d.begin(), d.end());
-
-  result_fd = RandomFD;
-  return error_code::success();
-}
-
 error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) {
   AutoFD ScopedFD(FD);
   if (!CloseFD)

Modified: llvm/trunk/lib/Support/Windows/Path.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Path.inc?rev=185126&r1=185125&r2=185126&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Windows/Path.inc (original)
+++ llvm/trunk/lib/Support/Windows/Path.inc Thu Jun 27 22:48:47 2013
@@ -124,6 +124,140 @@ namespace {
   }
 }
 
+// FIXME: mode should be used here and default to user r/w only,
+// it currently comes in as a UNIX mode.
+static error_code createUniqueEntity(const Twine &model, int &result_fd,
+                                     SmallVectorImpl<char> &result_path,
+                                     bool makeAbsolute, unsigned mode,
+                                     FSEntity Type) {
+  // Use result_path as temp storage.
+  result_path.set_size(0);
+  StringRef m = model.toStringRef(result_path);
+
+  SmallVector<wchar_t, 128> model_utf16;
+  if (error_code ec = UTF8ToUTF16(m, model_utf16)) return ec;
+
+  if (makeAbsolute) {
+    // Make model absolute by prepending a temp directory if it's not already.
+    bool absolute = sys::path::is_absolute(m);
+
+    if (!absolute) {
+      SmallVector<wchar_t, 64> temp_dir;
+      if (error_code ec = TempDir(temp_dir)) return ec;
+      // Handle c: by removing it.
+      if (model_utf16.size() > 2 && model_utf16[1] == L':') {
+        model_utf16.erase(model_utf16.begin(), model_utf16.begin() + 2);
+      }
+      model_utf16.insert(model_utf16.begin(), temp_dir.begin(), temp_dir.end());
+    }
+  }
+
+  // Replace '%' with random chars. From here on, DO NOT modify model. It may be
+  // needed if the randomly chosen path already exists.
+  SmallVector<wchar_t, 128> random_path_utf16;
+
+  // Get a Crypto Provider for CryptGenRandom.
+  HCRYPTPROV HCPC;
+  if (!::CryptAcquireContextW(&HCPC,
+                              NULL,
+                              NULL,
+                              PROV_RSA_FULL,
+                              CRYPT_VERIFYCONTEXT))
+    return windows_error(::GetLastError());
+  ScopedCryptContext CryptoProvider(HCPC);
+
+retry_random_path:
+  random_path_utf16.set_size(0);
+  for (SmallVectorImpl<wchar_t>::const_iterator i = model_utf16.begin(),
+                                                e = model_utf16.end();
+                                                i != e; ++i) {
+    if (*i == L'%') {
+      BYTE val = 0;
+      if (!::CryptGenRandom(CryptoProvider, 1, &val))
+          return windows_error(::GetLastError());
+      random_path_utf16.push_back("0123456789abcdef"[val & 15]);
+    }
+    else
+      random_path_utf16.push_back(*i);
+  }
+  // Make random_path_utf16 null terminated.
+  random_path_utf16.push_back(0);
+  random_path_utf16.pop_back();
+
+  HANDLE TempFileHandle;
+
+  switch (Type) {
+  case FS_File: {
+    // Try to create + open the path.
+    TempFileHandle =
+        ::CreateFileW(random_path_utf16.begin(), GENERIC_READ | GENERIC_WRITE,
+                      FILE_SHARE_READ, NULL,
+                      // Return ERROR_FILE_EXISTS if the file
+                      // already exists.
+                      CREATE_NEW, FILE_ATTRIBUTE_TEMPORARY, NULL);
+    if (TempFileHandle == INVALID_HANDLE_VALUE) {
+      // If the file existed, try again, otherwise, error.
+      error_code ec = windows_error(::GetLastError());
+      if (ec == windows_error::file_exists)
+        goto retry_random_path;
+
+      return ec;
+    }
+
+    // Convert the Windows API file handle into a C-runtime handle.
+    int fd = ::_open_osfhandle(intptr_t(TempFileHandle), 0);
+    if (fd == -1) {
+      ::CloseHandle(TempFileHandle);
+      ::DeleteFileW(random_path_utf16.begin());
+      // MSDN doesn't say anything about _open_osfhandle setting errno or
+      // GetLastError(), so just return invalid_handle.
+      return windows_error::invalid_handle;
+    }
+
+    result_fd = fd;
+    break;
+  }
+
+  case FS_Name: {
+    DWORD attributes = ::GetFileAttributesW(random_path_utf16.begin());
+    if (attributes != INVALID_FILE_ATTRIBUTES)
+      goto retry_random_path;
+    error_code EC = make_error_code(windows_error(::GetLastError()));
+    if (EC != windows_error::file_not_found &&
+        EC != windows_error::path_not_found)
+      return EC;
+    break;
+  }
+
+  case FS_Dir:
+    if (!::CreateDirectoryW(random_path_utf16.begin(), NULL)) {
+      error_code EC = windows_error(::GetLastError());
+      if (EC != windows_error::already_exists)
+        return EC;
+      goto retry_random_path;
+    }
+    break;
+  }
+
+  // Set result_path to the utf-8 representation of the path.
+  if (error_code ec = UTF16ToUTF8(random_path_utf16.begin(),
+                                  random_path_utf16.size(), result_path)) {
+    switch (Type) {
+    case FS_File:
+      ::CloseHandle(TempFileHandle);
+      ::DeleteFileW(random_path_utf16.begin());
+    case FS_Name:
+      break;
+    case FS_Dir:
+      ::RemoveDirectoryW(random_path_utf16.begin());
+      break;
+    }
+    return ec;
+  }
+
+  return error_code::success();
+}
+
 namespace llvm {
 namespace sys  {
 namespace fs {
@@ -604,141 +738,6 @@ error_code setLastModificationAndAccessT
   return error_code::success();
 }
 
-// FIXME: mode should be used here and default to user r/w only,
-// it currently comes in as a UNIX mode.
-error_code unique_file(const Twine &model, int &result_fd,
-                       SmallVectorImpl<char> &result_path,
-                       bool makeAbsolute, unsigned mode) {
-  // Use result_path as temp storage.
-  result_path.set_size(0);
-  StringRef m = model.toStringRef(result_path);
-
-  SmallVector<wchar_t, 128> model_utf16;
-  if (error_code ec = UTF8ToUTF16(m, model_utf16)) return ec;
-
-  if (makeAbsolute) {
-    // Make model absolute by prepending a temp directory if it's not already.
-    bool absolute = path::is_absolute(m);
-
-    if (!absolute) {
-      SmallVector<wchar_t, 64> temp_dir;
-      if (error_code ec = TempDir(temp_dir)) return ec;
-      // Handle c: by removing it.
-      if (model_utf16.size() > 2 && model_utf16[1] == L':') {
-        model_utf16.erase(model_utf16.begin(), model_utf16.begin() + 2);
-      }
-      model_utf16.insert(model_utf16.begin(), temp_dir.begin(), temp_dir.end());
-    }
-  }
-
-  // Replace '%' with random chars. From here on, DO NOT modify model. It may be
-  // needed if the randomly chosen path already exists.
-  SmallVector<wchar_t, 128> random_path_utf16;
-
-  // Get a Crypto Provider for CryptGenRandom.
-  HCRYPTPROV HCPC;
-  if (!::CryptAcquireContextW(&HCPC,
-                              NULL,
-                              NULL,
-                              PROV_RSA_FULL,
-                              CRYPT_VERIFYCONTEXT))
-    return windows_error(::GetLastError());
-  ScopedCryptContext CryptoProvider(HCPC);
-
-retry_random_path:
-  random_path_utf16.set_size(0);
-  for (SmallVectorImpl<wchar_t>::const_iterator i = model_utf16.begin(),
-                                                e = model_utf16.end();
-                                                i != e; ++i) {
-    if (*i == L'%') {
-      BYTE val = 0;
-      if (!::CryptGenRandom(CryptoProvider, 1, &val))
-          return windows_error(::GetLastError());
-      random_path_utf16.push_back("0123456789abcdef"[val & 15]);
-    }
-    else
-      random_path_utf16.push_back(*i);
-  }
-  // Make random_path_utf16 null terminated.
-  random_path_utf16.push_back(0);
-  random_path_utf16.pop_back();
-
-  // Make sure we don't fall into an infinite loop by constantly trying
-  // to create the parent path.
-  bool TriedToCreateParent = false;
-
-  // Try to create + open the path.
-retry_create_file:
-  HANDLE TempFileHandle = ::CreateFileW(random_path_utf16.begin(),
-                                        GENERIC_READ | GENERIC_WRITE,
-                                        FILE_SHARE_READ,
-                                        NULL,
-                                        // Return ERROR_FILE_EXISTS if the file
-                                        // already exists.
-                                        CREATE_NEW,
-                                        FILE_ATTRIBUTE_TEMPORARY,
-                                        NULL);
-  if (TempFileHandle == INVALID_HANDLE_VALUE) {
-    // If the file existed, try again, otherwise, error.
-    error_code ec = windows_error(::GetLastError());
-    if (ec == windows_error::file_exists)
-      goto retry_random_path;
-    // Check for non-existing parent directories.
-    if (ec == windows_error::path_not_found && !TriedToCreateParent) {
-      TriedToCreateParent = true;
-
-      // Create the directories using result_path as temp storage.
-      if (error_code ec = UTF16ToUTF8(random_path_utf16.begin(),
-                                      random_path_utf16.size(), result_path))
-        return ec;
-      StringRef p(result_path.begin(), result_path.size());
-      SmallString<64> dir_to_create;
-      for (path::const_iterator i = path::begin(p),
-                                e = --path::end(p); i != e; ++i) {
-        path::append(dir_to_create, *i);
-        bool Exists;
-        if (error_code ec = exists(Twine(dir_to_create), Exists)) return ec;
-        if (!Exists) {
-          // If c: doesn't exist, bail.
-          if (i->endswith(":"))
-            return ec;
-
-          SmallVector<wchar_t, 64> dir_to_create_utf16;
-          if (error_code ec = UTF8ToUTF16(dir_to_create, dir_to_create_utf16))
-            return ec;
-
-          // Create the directory.
-          if (!::CreateDirectoryW(dir_to_create_utf16.begin(), NULL))
-            return windows_error(::GetLastError());
-        }
-      }
-      goto retry_create_file;
-    }
-    return ec;
-  }
-
-  // Set result_path to the utf-8 representation of the path.
-  if (error_code ec = UTF16ToUTF8(random_path_utf16.begin(),
-                                  random_path_utf16.size(), result_path)) {
-    ::CloseHandle(TempFileHandle);
-    ::DeleteFileW(random_path_utf16.begin());
-    return ec;
-  }
-
-  // Convert the Windows API file handle into a C-runtime handle.
-  int fd = ::_open_osfhandle(intptr_t(TempFileHandle), 0);
-  if (fd == -1) {
-    ::CloseHandle(TempFileHandle);
-    ::DeleteFileW(random_path_utf16.begin());
-    // MSDN doesn't say anything about _open_osfhandle setting errno or
-    // GetLastError(), so just return invalid_handle.
-    return windows_error::invalid_handle;
-  }
-
-  result_fd = fd;
-  return error_code::success();
-}
-
 error_code get_magic(const Twine &path, uint32_t len,
                      SmallVectorImpl<char> &result) {
   SmallString<128> path_storage;





More information about the llvm-commits mailing list