[libcxx-commits] [PATCH] D91171: [16/N] [libcxx] Implement the permissions function for windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/filesystem/operations.cpp:1434
+    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,
----------------
amccarth wrote:
> 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.)
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.


================
Comment at: libcxx/src/filesystem/operations.cpp:1445
+      basic.FileAttributes |= FILE_ATTRIBUTE_READONLY;
+    if (!SetFileInformationByHandle(h, FileBasicInfo, &basic, sizeof(basic)))
+      return err.report(detail::make_windows_error(GetLastError()));
----------------
amccarth wrote:
> 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.
Sure, I can change that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91171/new/

https://reviews.llvm.org/D91171