Remove FileSystem::MakeDirectory
ClosedPublic

Authored by zturner on Mar 17 2017, 10:28 AM.

Details

Summary

Use the LLVM function instead.

There are two subtle behavioral changes here which I want to make clear so someone can determine whether this matters on their platform.

  1. Previously all LLDB callers were passing eFilePermissionsDirectoryDefault which is equal to eFilePermissionsUserRWX (0o700). The LLVM default is equivalent to eFilePermissionsUserRWX | eFilePermissionsGroupRWX (0o770). If this is a problem it's easy to update all callsites to explicitly pass 0o700, but I don't think it is.
  1. The implementation of MakeDirectory would first try to create the directory, then if it failed due to ENOENT would try to create the parent directory. But it only went up one level. So if /foo existed but not /foo/bar, and you tried to create /foo/bar/baz, it would create /foo/bar first and then /foo/bar/baz. On the other hand, if not even /foo existed, the function would fail.

This seems like very strange behavior to me, but I don't know if anyone depends on it. I imagine if the tests pass everywhere, then nobody does. They pass on Windows. If someone could confirm that they pass on other platforms after this patch it would be helpful.

Diff Detail

Repository
rL LLVM
zturner created this revision.Mar 17 2017, 10:28 AM

I think you missed one occurence in ModuleCache.cpp. This one creates a full directory structure, so you will probably need recursion there (unlike in the rest of these cases, which seem fine). Also unlike the rest of these cases, that one is actually covered by a unit test, so you should be able to verify that. :)

That one is calling a file static function MakeDirectory, not FileSystem::MakeDirectory, and the implementation of that function already calls llvm::sys::fs::create_directories() to create the whole tree, so it should be fine.

That one is calling a file static function MakeDirectory, not FileSystem::MakeDirectory, and the implementation of that function already calls llvm::sys::fs::create_directories() to create the whole tree, so it should be fine.

Right, sorry about that. I was looking at an old checkout. I'm not sure about the PlatformDarwin stuff, but the rest seems fine.

sas accepted this revision.Mar 17 2017, 5:03 PM
sas added a subscriber: sas.

The second behavioral change seems good but the first thing you described is a bit odd. Creating folders with 770 does not seem like a common behavior, and 700 or 755 is usually more standard.

Either way, I think this change is good.

This revision is now accepted and ready to land.Mar 17 2017, 5:03 PM
This revision was automatically updated to reflect the committed changes.
In D31086#704518, @sas wrote:

The second behavioral change seems good but the first thing you described is a bit odd. Creating folders with 770 does not seem like a common behavior, and 700 or 755 is usually more standard.

Don't forget that the real permissions will also reflect the user's umask(2). The completely standard way would be to use 777 or 666 for the permissions (except perhaps for some temporary/sensitive files), and then let the user choose what are the real permissions he wants by setting umask in his bashrc. I guess we are being a bit paranoid here and disallowing the "others" permissions just in case...

sas added a comment.Mar 20 2017, 11:53 AM
In D31086#704518, @sas wrote:

The second behavioral change seems good but the first thing you described is a bit odd. Creating folders with 770 does not seem like a common behavior, and 700 or 755 is usually more standard.

Don't forget that the real permissions will also reflect the user's umask(2). The completely standard way would be to use 777 or 666 for the permissions (except perhaps for some temporary/sensitive files), and then let the user choose what are the real permissions he wants by setting umask in his bashrc. I guess we are being a bit paranoid here and disallowing the "others" permissions just in case...

Didn't know that. If these respect the user's umask(2) then this is good.