[16/N] [libcxx] Implement the permissions function for windows

[mstorsjo]

Rebased on top of updated patches.

[amccarth]

libcxx/src/filesystem/operations.cpp

1434

Can you add a comment explaining why reparse points need special handling?

Both branches appear to do the same thing using different APIs. I assume this has to do with whether you're tweaking the reparse point itself or the file it points to, but that's not super obvious.

(And if that is the reason, then I'd expect these branches to be the other way around.)

1445

Is it worth skipping the Set operation if the file (or reparse point) has no effect?

I imagine a common thing to do is to set permissions on zillions of files at once, so avoiding an extra system call when its not necessary seems like it would be worth the cost of an extra conditional branch.

1451

Same question about skipping the Sets when they're not necessary.

[mstorsjo]

libcxx/src/filesystem/operations.cpp

1434

Sure, can add a comment.

The branch is the right way around (a couple tests fail if it's flipped) - the get/set functions that take a filename operate on the reparse point itself. The CreateFile function (wrapped by WinHandle) resolves to the reparse target unless FILE_FLAG_OPEN_REPARSE_POINT is passed.

I guess one could simplify the code by always using the handle codepath but optionally setting the FILE_FLAG_OPEN_REPARSE_POINT flag - but that codepath uses a couple more syscalls, so maybe it's worth keeping both - with some more comments.

1445

Sure, I can change that.

[mstorsjo]

Added comments about the two conditional branches, skipping the set operation if there's no change.

[mstorsjo]

Ping @amccarth, this one is missing follow-up on the earlier discussion.

[amccarth]

LGTM.

[mstorsjo]

Made fchmod and fchmodat helpers in posix_compat.h.

[mstorsjo]

Rebased, retriggering CI.

This is also a bit restructured from the version @amccarth reviewed, as I moved the code out from the __permissions() function into posix-like fchmod() and fchmodat(). That refactoring itself is pretty straightforward, and allowed getting rid of a lot of extra ifdefs.

[amccarth]

LGTM

[ldionne]

libcxx/src/filesystem/operations.cpp

672

I don't understand the purpose of that function. Isn't that equivalent to just static_cast<::mode_t>(prms)?

If so, this function is called only once below. Why don't we simply use:

#if defined(_LIBCPP_WIN32API)
  using ModeT = int;
#else
  using ModeT = ::mode_t;
#endif

const auto real_perms = static_cast<ModeT>(prms);

Well, you get the idea.

[mstorsjo]

libcxx/src/filesystem/operations.cpp

672

Yeah, that clearly seems better.

[mstorsjo]

Updated, rebased, added a ModeT type, replaced the superfluous function with a plain static_cast.

[mstorsjo]

Retrigger CI.

[curdeius]

libcxx/src/filesystem/operations.cpp

672

Is then the bit and & perms::mask irrelevant on POSIX?

[mstorsjo]

libcxx/src/filesystem/operations.cpp

672

Oops, sorry for missing that

[mstorsjo]

Reinstated the missed & perms::mask

[ldionne]

LGTM pending CI.

libcxx/src/filesystem/operations.cpp

 private:
   explicit FileDescriptor(const path* p, int fd = -1) : name(*p), fd(fd) {}
 };
 
 perms posix_get_perms(const StatT& st) noexcept {
   return static_cast<perms>(st.st_mode) & perms::mask;
 }
 
+#if !defined(_LIBCPP_WIN32API)
 ::mode_t posix_convert_perms(perms prms) {
   return static_cast< ::mode_t>(prms & perms::mask);
 }
+#endif
 
 file_status create_file_status(error_code& m_ec, path const& p,
                                const StatT& path_stat, error_code* ec) {
   if (ec)
     *ec = m_ec;
   if (m_ec && (m_ec.value() == ENOENT || m_ec.value() == ENOTDIR)) {
     return file_status(file_type::not_found);
   } else if (m_ec) {
bool posix_ftruncate(const FileDescriptor& fd, off_t to_size, error_code& ec) {
   if (detail::ftruncate(fd.fd, to_size) == -1) {
     ec = capture_errno();
     return true;
   }
   ec.clear();
   return false;
 }
 
+#if !defined(_LIBCPP_WIN32API)
 bool posix_fchmod(const FileDescriptor& fd, const StatT& st, error_code& ec) {
   if (::fchmod(fd.fd, st.st_mode) == -1) {
     ec = capture_errno();
     return true;
   }
   ec.clear();
   return false;
 }
+#endif
 
 bool stat_equivalent(const StatT& st1, const StatT& st2) {
   return (st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
 }
 
 file_status FileDescriptor::refresh_status(error_code& ec) {
   // FD must be open and good.
   m_status = file_status{};
bool __copy_file(const path& from, const path& to, copy_options options,
   if (to_exists) {
     // Check that the file we initially stat'ed is equivalent to the one
     // we opened.
     // FIXME: report this better.
     if (!detail::stat_equivalent(to_stat_path, to_fd.get_stat()))
       return err.report(errc::bad_file_descriptor);
 
     // Set the permissions and truncate the file we opened.
+#if !defined(_LIBCPP_WIN32API)
     if (detail::posix_fchmod(to_fd, from_stat, m_ec))
       return err.report(m_ec);
+#endif
     if (detail::posix_ftruncate(to_fd, 0, m_ec))
       return err.report(m_ec);
   }
 
   if (!copy_file_impl(from_fd, to_fd, m_ec)) {
     // FIXME: Remove the dest file if we failed, and it didn't exist previously.
     return err.report(m_ec);
   }
if (m_ec)
       return err.report(m_ec);
     _LIBCPP_ASSERT(st.permissions() != perms::unknown,
                    "Permissions unexpectedly unknown");
     if (add_perms)
       prms |= st.permissions();
     else if (remove_perms)
       prms = st.permissions() & ~prms;
   }
+#if !defined(_LIBCPP_WIN32API)
   const auto real_perms = detail::posix_convert_perms(prms);
 
 #if defined(AT_SYMLINK_NOFOLLOW) && defined(AT_FDCWD)
   const int flags = set_sym_perms ? AT_SYMLINK_NOFOLLOW : 0;
   if (::fchmodat(AT_FDCWD, p.c_str(), real_perms, flags) == -1) {
     return err.report(capture_errno());
   }
 #else
   if (set_sym_perms)
     return err.report(errc::operation_not_supported);
   if (::chmod(p.c_str(), real_perms) == -1) {
     return err.report(capture_errno());
   }
 #endif
+#else
+  DWORD attributes = GetFileAttributesW(p.c_str());
+  if (attributes == INVALID_FILE_ATTRIBUTES)
+    return err.report(detail::make_windows_error(GetLastError()));
+  if (attributes & FILE_ATTRIBUTE_REPARSE_POINT && resolve_symlinks) {
+    detail::WinHandle h(p.c_str(), FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES,
+                        0);
+    if (!h)
+      return err.report(detail::make_windows_error(GetLastError()));
+    FILE_BASIC_INFO basic;
+    if (!GetFileInformationByHandleEx(h, FileBasicInfo, &basic, sizeof(basic)))
+      return err.report(detail::make_windows_error(GetLastError()));
+    basic.FileAttributes &= ~FILE_ATTRIBUTE_READONLY;
+    if ((static_cast<int>(prms) & 0222) == 0)
+      basic.FileAttributes |= FILE_ATTRIBUTE_READONLY;
+    if (!SetFileInformationByHandle(h, FileBasicInfo, &basic, sizeof(basic)))
+      return err.report(detail::make_windows_error(GetLastError()));
+  } else {
+    attributes &= ~FILE_ATTRIBUTE_READONLY;
+    if ((static_cast<int>(prms) & 0222) == 0)
+      attributes |= FILE_ATTRIBUTE_READONLY;
+    if (!SetFileAttributesW(p.c_str(), attributes))
+      return err.report(detail::make_windows_error(GetLastError()));
+  }
+#endif
 }
 
 path __read_symlink(const path& p, error_code* ec) {
   ErrorHandler<path> err("read_symlink", ec, &p);
 
 #ifdef PATH_MAX
   struct NullDeleter { void operator()(void*) const {} };
   const size_t size = PATH_MAX + 1;