Thread premissions through sys::fs::create_director{y|ies}

[Frédéric Riss]

> 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:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: create_directories.patch
Type: application/octet-stream
Size: 6413 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150805/aeb93eac/attachment.obj>
-------------- next part --------------


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