Page MenuHomePhabricator

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

Authored by mstorsjo on Nov 10 2020, 8:38 AM.

Details

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 10 2020, 8:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 8:38 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo updated this revision to Diff 305083.Nov 13 2020, 3:46 AM
mstorsjo added a reviewer: amccarth.

Rebased on top of updated patches.

amccarth requested changes to this revision.Dec 4 2020, 11:18 AM
amccarth added inline comments.
libcxx/src/filesystem/operations.cpp
1229

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.)

1240

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.

1246

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

This revision now requires changes to proceed.Dec 4 2020, 11:18 AM
mstorsjo added inline comments.Dec 4 2020, 12:37 PM
libcxx/src/filesystem/operations.cpp
1229

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.

1240

Sure, I can change that.

mstorsjo updated this revision to Diff 309867.Dec 7 2020, 4:08 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

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

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

amccarth accepted this revision.Jan 8 2021, 11:43 AM

LGTM.

mstorsjo updated this revision to Diff 317583.Jan 19 2021, 8:27 AM

Made fchmod and fchmodat helpers in posix_compat.h.

mstorsjo edited the summary of this revision. (Show Details)Jan 19 2021, 8:28 AM
ldionne set the repository for this revision to rG LLVM Github Monorepo.Jan 28 2021, 3:19 PM
mstorsjo removed rG LLVM Github Monorepo as the repository for this revision.Jan 29 2021, 4:39 AM
mstorsjo updated this revision to Diff 320104.Jan 29 2021, 4:41 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

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.

ldionne requested changes to this revision.Tue, Feb 2, 12:35 PM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/src/filesystem/operations.cpp
457–458

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.

This revision now requires changes to proceed.Tue, Feb 2, 12:35 PM
mstorsjo added inline comments.Tue, Feb 2, 1:05 PM
libcxx/src/filesystem/operations.cpp
457–458

Yeah, that clearly seems better.

mstorsjo updated this revision to Diff 320892.Tue, Feb 2, 1:06 PM

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

mstorsjo updated this revision to Diff 320893.Tue, Feb 2, 1:07 PM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Retrigger CI.

curdeius added inline comments.
libcxx/src/filesystem/operations.cpp
457–458

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

mstorsjo added inline comments.Tue, Feb 2, 1:17 PM
libcxx/src/filesystem/operations.cpp
457–458

Oops, sorry for missing that

mstorsjo updated this revision to Diff 320895.Tue, Feb 2, 1:17 PM
mstorsjo removed rG LLVM Github Monorepo as the repository for this revision.
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Reinstated the missed & perms::mask

ldionne accepted this revision.Tue, Feb 2, 1:20 PM

LGTM pending CI.

This revision is now accepted and ready to land.Tue, Feb 2, 1:20 PM
This revision was landed with ongoing or failed builds.Tue, Feb 2, 11:28 PM
This revision was automatically updated to reflect the committed changes.