Thread premissions through sys::fs::create_director{y|ies}
Justin Bogner
mail at justinbogner.com
Wed Aug 5 21:35:19 PDT 2015
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.
Anyways, LGTM.
> 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