Page MenuHomePhabricator

[Support] Add fs::getUmask() function and change fs::setPermissions
ClosedPublic

Authored by abrachet on Jun 19 2019, 5:18 PM.

Details

Summary

This patch changes fs::setPermissions to optionally set permissions while respecting the umask. It also adds the function fs::getUmask() which returns the current umask.

Diff Detail

Event Timeline

abrachet created this revision.Jun 19 2019, 5:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 5:18 PM
abrachet marked an inline comment as done.Jun 19 2019, 5:22 PM

This change is not 100% necessary it doesn't need to be in lib/Support, but I am planning on using it for D62718. I figured I would test the waters on this one. Also, fs::getUmask() isn't necessary to have, I think that the change to fs::setPermissions would be enough, but I figured I would add it and remove later than the other way around.

llvm/lib/Support/Windows/Path.inc
741

I don't know if unnamed parameters are allowed per the style guide, it isn't used by the Windows version of this function, this was the thinking behind this.

abrachet updated this revision to Diff 205723.Jun 19 2019, 5:27 PM

Updated doxygen comments for fs::setPermissions to explain new RespectUmask parameter.

I've added a couple of other reviewers who have made recent changes to the Path files.

llvm/lib/Support/Unix/Path.inc
695–698

It's not your fault, but this dance seems ridiculous.

702

clang-format?

710

Remove double-blank line.

llvm/lib/Support/Windows/Path.inc
741

clang-format.

I think this is fine.

llvm/unittests/Support/Path.cpp
1541

You're capitalization of test names is inconsistent. One starts with a leading caps, and the other doesn't.

1555

I think you also need a test case for explicit false for respectUmask.

abrachet updated this revision to Diff 205860.Jun 20 2019, 10:56 AM

Added test case and changed formatting.

abrachet marked 9 inline comments as done.Jun 20 2019, 11:04 AM
abrachet added inline comments.
llvm/lib/Support/Unix/Path.inc
695–698

Agreed, there are quite a few get/set syscalls, not sure why this needed to be the case for umask.

702

Yup sorry about that I should have looked over git clang-formats changes. It did not format the .inc files. This should be proper formatting now, I just copied how it is in the header file which clang-format did.

llvm/unittests/Support/Path.cpp
1541

I think this is consistent with the rest of the file. getUmask is capitalized as such because it is testing a specific function, while RespectUmask is testing behavior. is_local test versus the ReadWriteFileCanReadOrWrite test.

jhenderson accepted this revision.Jun 21 2019, 1:24 AM

LGTM. Maybe give it a couple of days before committing for a chance for others to say something.

This revision is now accepted and ready to land.Jun 21 2019, 1:24 AM
rupprecht accepted this revision.Jun 21 2019, 5:35 PM
rupprecht added inline comments.
llvm/include/llvm/Support/FileSystem.h
654–656

Also note that it is not threadsafe

llvm/lib/Support/Unix/Path.inc
695

nit: ret set -> reset

abrachet updated this revision to Diff 206106.Jun 21 2019, 5:44 PM
abrachet marked 3 inline comments as done.

Fixed typo in comment, added note that getUmask() is not thread safe.

abrachet marked 2 inline comments as done.Jun 21 2019, 5:45 PM
This revision was automatically updated to reflect the committed changes.