Thread premissions through sys::fs::create_director{y|ies}
Frédéric Riss
friss at apple.com
Wed Aug 5 21:47:54 PDT 2015
> On Aug 5, 2015, at 9:35 PM, Justin Bogner <mail at justinbogner.com> wrote:
>
> Frédéric Riss <friss at apple.com> writes:
>>> On Aug 5, 2015, at 1:16 PM, Justin Bogner <mail at justinbogner.com> wrote:
>>>
>>> Frédéric Riss <friss at apple.com> writes:
>>>> The attached pretty trivial patch thread a permissions argument
>>>> through sys::fs::create_directories.
>>>> I’m sending it for review regardless of the trivialness for 2 reasons:
>>>> - I have no idea how to test that. The passed permission are still
>>>> subject to umask, thus testing the file_status of a created directory
>>>> in a unittest doesn’t seem possible. Ideas welcome.
>>>
>>> Can't you just call umask(2) in the test?
>>
>> That would work. How does that look:
>
> Thorough. It's probably sufficient to just do one of the
> create_directory tests, since we're really just testing that we thread
> the option through properly.
Yeah, I was thinking about testing each bit once on once off, but as we are just passing a raw perm value around this might be a bit overkill. I’ll simplify before I commit.
> Anyways, LGTM.
Thanks!
Fred
>> commit 6c9c836cea3f490a19e2e7e0365d2d3e9560b169
>> Author: Frederic Riss <friss at apple.com>
>> Date: Tue Aug 4 22:48:27 2015 -0700
>>
>> Thread premissions through sys::fs::create_director{y|ies}
>>
>> diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h
>> index a736c32..36c669a 100644
>> --- a/include/llvm/Support/FileSystem.h
>> +++ b/include/llvm/Support/FileSystem.h
>> @@ -285,7 +285,8 @@ std::error_code make_absolute(SmallVectorImpl<char> &path);
>> /// specific error_code. If IgnoreExisting is false, also returns
>> /// error if the directory already existed.
>> std::error_code create_directories(const Twine &path,
>> - bool IgnoreExisting = true);
>> + bool IgnoreExisting = true,
>> + perms Perms = owner_all | group_all);
>>
>> /// @brief Create the directory in path.
>> ///
>> @@ -293,7 +294,8 @@ std::error_code create_directories(const Twine &path,
>> /// @returns errc::success if is_directory(path), otherwise a platform
>> /// specific error_code. If IgnoreExisting is false, also returns
>> /// error if the directory already existed.
>> -std::error_code create_directory(const Twine &path, bool IgnoreExisting = true);
>> +std::error_code create_directory(const Twine &path, bool IgnoreExisting = true,
>> + perms Perms = owner_all | group_all);
>>
>> /// @brief Create a link from \a from to \a to.
>> ///
>> diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp
>> index 985cdbf..54daf24 100644
>> --- a/lib/Support/Path.cpp
>> +++ b/lib/Support/Path.cpp
>> @@ -785,12 +785,13 @@ std::error_code make_absolute(SmallVectorImpl<char> &path) {
>> "occurred above!");
>> }
>>
>> -std::error_code create_directories(const Twine &Path, bool IgnoreExisting) {
>> +std::error_code create_directories(const Twine &Path, bool IgnoreExisting,
>> + perms Perms) {
>> SmallString<128> PathStorage;
>> StringRef P = Path.toStringRef(PathStorage);
>>
>> // Be optimistic and try to create the directory
>> - std::error_code EC = create_directory(P, IgnoreExisting);
>> + std::error_code EC = create_directory(P, IgnoreExisting, Perms);
>> // If we succeeded, or had any error other than the parent not existing, just
>> // return it.
>> if (EC != errc::no_such_file_or_directory)
>> @@ -802,10 +803,10 @@ std::error_code create_directories(const Twine &Path, bool IgnoreExisting) {
>> if (Parent.empty())
>> return EC;
>>
>> - if ((EC = create_directories(Parent)))
>> + if ((EC = create_directories(Parent, IgnoreExisting, Perms)))
>> return EC;
>>
>> - return create_directory(P, IgnoreExisting);
>> + return create_directory(P, IgnoreExisting, Perms);
>> }
>>
>> std::error_code copy_file(const Twine &From, const Twine &To) {
>> diff --git a/lib/Support/Unix/Path.inc b/lib/Support/Unix/Path.inc
>> index 973d010..a77efcc 100644
>> --- a/lib/Support/Unix/Path.inc
>> +++ b/lib/Support/Unix/Path.inc
>> @@ -219,11 +219,12 @@ std::error_code current_path(SmallVectorImpl<char> &result) {
>> return std::error_code();
>> }
>>
>> -std::error_code create_directory(const Twine &path, bool IgnoreExisting) {
>> +std::error_code create_directory(const Twine &path, bool IgnoreExisting,
>> + perms Perms) {
>> SmallString<128> path_storage;
>> StringRef p = path.toNullTerminatedStringRef(path_storage);
>>
>> - if (::mkdir(p.begin(), S_IRWXU | S_IRWXG) == -1) {
>> + if (::mkdir(p.begin(), Perms) == -1) {
>> if (errno != EEXIST || !IgnoreExisting)
>> return std::error_code(errno, std::generic_category());
>> }
>> diff --git a/lib/Support/Windows/Path.inc b/lib/Support/Windows/Path.inc
>> index 72da7c5..3033149 100644
>> --- a/lib/Support/Windows/Path.inc
>> +++ b/lib/Support/Windows/Path.inc
>> @@ -182,7 +182,8 @@ std::error_code current_path(SmallVectorImpl<char> &result) {
>> return UTF16ToUTF8(cur_path.begin(), cur_path.size(), result);
>> }
>>
>> -std::error_code create_directory(const Twine &path, bool IgnoreExisting) {
>> +std::error_code create_directory(const Twine &path, bool IgnoreExisting,
>> + perms Perms) {
>> SmallVector<wchar_t, 128> path_utf16;
>>
>> if (std::error_code ec = widenPath(path, path_utf16))
>> diff --git a/unittests/Support/Path.cpp b/unittests/Support/Path.cpp
>> index 210b3a0..7ac7669 100644
>> --- a/unittests/Support/Path.cpp
>> +++ b/unittests/Support/Path.cpp
>> @@ -20,6 +20,10 @@
>> #include <winerror.h>
>> #endif
>>
>> +#ifdef LLVM_ON_UNIX
>> +#include <sys/stat.h>
>> +#endif
>> +
>> using namespace llvm;
>> using namespace llvm::sys;
>>
>> @@ -458,6 +462,38 @@ TEST_F(FileSystemTest, CreateDir) {
>> errc::file_exists);
>> ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "foo"));
>>
>> +#ifdef LLVM_ON_UNIX
>> + // Set a 0000 umask so that we can test our directory permissions.
>> + mode_t OldUmask = ::umask(0000);
>> +
>> + fs::file_status Status;
>> + ASSERT_NO_ERROR(
>> + fs::create_directory(Twine(TestDirectory) + "baz500", false,
>> + fs::perms::owner_read | fs::perms::owner_exe));
>> + ASSERT_NO_ERROR(fs::status(Twine(TestDirectory) + "baz500", Status));
>> + ASSERT_EQ(Status.permissions() & fs::perms::all_all,
>> + fs::perms::owner_read | fs::perms::owner_exe);
>> + ASSERT_NO_ERROR(
>> + fs::create_directory(Twine(TestDirectory) + "baz050", false,
>> + fs::perms::group_read | fs::perms::group_exe));
>> + ASSERT_NO_ERROR(fs::status(Twine(TestDirectory) + "baz050", Status));
>> + ASSERT_EQ(Status.permissions() & fs::perms::all_all,
>> + fs::perms::group_read | fs::perms::group_exe);
>> + ASSERT_NO_ERROR(
>> + fs::create_directory(Twine(TestDirectory) + "baz005", false,
>> + fs::perms::others_read | fs::perms::others_exe));
>> + ASSERT_NO_ERROR(fs::status(Twine(TestDirectory) + "baz005", Status));
>> + ASSERT_EQ(Status.permissions() & fs::perms::all_all,
>> + fs::perms::others_read | fs::perms::others_exe);
>> + ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory) + "baz777", false,
>> + fs::perms::all_all));
>> + ASSERT_NO_ERROR(fs::status(Twine(TestDirectory) + "baz777", Status));
>> + ASSERT_EQ(Status.permissions() & fs::perms::all_all, fs::perms::all_all);
>> +
>> + // Restore umask to be safe.
>> + ::umask(OldUmask);
>> +#endif
>> +
>> #ifdef LLVM_ON_WIN32
>> // Prove that create_directories() can handle a pathname > 248 characters,
>> // which is the documented limit for CreateDirectory().
>>
>>
>> Fred
>>
>>>> - I suppose Windows doesn’t care about permissions, thus I just
>>>> ignored the argument. Is that right?
>>>>
>>>> commit be795a04ef70e7832b26a46bbfe82fd618ebc701
>>>> Author: Frederic Riss <friss at apple.com>
>>>> Date: Tue Aug 4 22:48:27 2015 -0700
>>>>
>>>> Thread premissions through sys::fs::create_director{y|ies}
>>>>
>>>> diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h
>>>> index a736c32..36c669a 100644
>>>> --- a/include/llvm/Support/FileSystem.h
>>>> +++ b/include/llvm/Support/FileSystem.h
>>>> @@ -285,7 +285,8 @@ std::error_code make_absolute(SmallVectorImpl<char> &path);
>>>> /// specific error_code. If IgnoreExisting is false, also returns
>>>> /// error if the directory already existed.
>>>> std::error_code create_directories(const Twine &path,
>>>> - bool IgnoreExisting = true);
>>>> + bool IgnoreExisting = true,
>>>> + perms Perms = owner_all | group_all);
>>>>
>>>> /// @brief Create the directory in path.
>>>> ///
>>>> @@ -293,7 +294,8 @@ std::error_code create_directories(const Twine &path,
>>>> /// @returns errc::success if is_directory(path), otherwise a platform
>>>> /// specific error_code. If IgnoreExisting is false, also returns
>>>> /// error if the directory already existed.
>>>> -std::error_code create_directory(const Twine &path, bool IgnoreExisting = true);
>>>> +std::error_code create_directory(const Twine &path, bool IgnoreExisting = true,
>>>> + perms Perms = owner_all | group_all);
>>>>
>>>> /// @brief Create a link from \a from to \a to.
>>>> ///
>>>> diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp
>>>> index 985cdbf..54daf24 100644
>>>> --- a/lib/Support/Path.cpp
>>>> +++ b/lib/Support/Path.cpp
>>>> @@ -785,12 +785,13 @@ std::error_code make_absolute(SmallVectorImpl<char> &path) {
>>>> "occurred above!");
>>>> }
>>>>
>>>> -std::error_code create_directories(const Twine &Path, bool IgnoreExisting) {
>>>> +std::error_code create_directories(const Twine &Path, bool IgnoreExisting,
>>>> + perms Perms) {
>>>> SmallString<128> PathStorage;
>>>> StringRef P = Path.toStringRef(PathStorage);
>>>>
>>>> // Be optimistic and try to create the directory
>>>> - std::error_code EC = create_directory(P, IgnoreExisting);
>>>> + std::error_code EC = create_directory(P, IgnoreExisting, Perms);
>>>> // If we succeeded, or had any error other than the parent not existing, just
>>>> // return it.
>>>> if (EC != errc::no_such_file_or_directory)
>>>> @@ -802,10 +803,10 @@ std::error_code create_directories(const Twine &Path, bool IgnoreExisting) {
>>>> if (Parent.empty())
>>>> return EC;
>>>>
>>>> - if ((EC = create_directories(Parent)))
>>>> + if ((EC = create_directories(Parent, IgnoreExisting, Perms)))
>>>> return EC;
>>>>
>>>> - return create_directory(P, IgnoreExisting);
>>>> + return create_directory(P, IgnoreExisting, Perms);
>>>> }
>>>>
>>>> std::error_code copy_file(const Twine &From, const Twine &To) {
>>>> diff --git a/lib/Support/Unix/Path.inc b/lib/Support/Unix/Path.inc
>>>> index 973d010..a77efcc 100644
>>>> --- a/lib/Support/Unix/Path.inc
>>>> +++ b/lib/Support/Unix/Path.inc
>>>> @@ -219,11 +219,12 @@ std::error_code current_path(SmallVectorImpl<char> &result) {
>>>> return std::error_code();
>>>> }
>>>>
>>>> -std::error_code create_directory(const Twine &path, bool IgnoreExisting) {
>>>> +std::error_code create_directory(const Twine &path, bool IgnoreExisting,
>>>> + perms Perms) {
>>>> SmallString<128> path_storage;
>>>> StringRef p = path.toNullTerminatedStringRef(path_storage);
>>>>
>>>> - if (::mkdir(p.begin(), S_IRWXU | S_IRWXG) == -1) {
>>>> + if (::mkdir(p.begin(), Perms) == -1) {
>>>> if (errno != EEXIST || !IgnoreExisting)
>>>> return std::error_code(errno, std::generic_category());
>>>> }
>>>> diff --git a/lib/Support/Windows/Path.inc b/lib/Support/Windows/Path.inc
>>>> index 72da7c5..3033149 100644
>>>> --- a/lib/Support/Windows/Path.inc
>>>> +++ b/lib/Support/Windows/Path.inc
>>>> @@ -182,7 +182,8 @@ std::error_code current_path(SmallVectorImpl<char> &result) {
>>>> return UTF16ToUTF8(cur_path.begin(), cur_path.size(), result);
>>>> }
>>>>
>>>> -std::error_code create_directory(const Twine &path, bool IgnoreExisting) {
>>>> +std::error_code create_directory(const Twine &path, bool IgnoreExisting,
>>>> + perms Perms) {
>>>
>>> Better to omit the unused parameter name, ie, `perms /*Perms*/`. This
>>> way -Wunused-parameter will be happy with it.
>>>
>>>> SmallVector<wchar_t, 128> path_utf16;
>>>>
>>>> if (std::error_code ec = widenPath(path, path_utf16))
>>>>
>>>>
>>>> Fred
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list