This is an archive of the discontinued LLVM Phabricator instance.

Replace file system forbidden symbols in the hostname which passed to the ModuleCache
ClosedPublic

Authored by ovyalov on May 23 2016, 6:17 PM.

Details

Reviewers
clayborg
labath
Summary

ModuleCache hostname may contain symbols that are not allowed in filenames - for example, in Android device_id is used as a hostname and in some cases can have colons ("10.0.0.1:5555").
Replacing forbidden symbols with underscores in order to make hostname filesystem-friendly.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 58178.May 23 2016, 6:17 PM
ovyalov retitled this revision from to Replace file system forbidden symbols in the hostname which passed to the ModuleCache .
ovyalov updated this object.
ovyalov added reviewers: labath, clayborg.
ovyalov added a subscriber: lldb-commits.
ovyalov updated this revision to Diff 58180.May 23 2016, 6:22 PM

Added asterisk symbol

labath accepted this revision.May 24 2016, 1:56 AM
labath edited edge metadata.

According to https://msdn.microsoft.com/en-gb/library/windows/desktop/aa365247%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396, characters with codes 1..31 (basically < ' '), are illegal as well. We might as well convert those, just in case.

I've been sitting on a unit test for the module cache locally for a while now... I guess it's time to put that in, and then we can add a test for this as well.

This revision is now accepted and ready to land.May 24 2016, 1:56 AM
clayborg accepted this revision.May 24 2016, 9:44 AM
clayborg edited edge metadata.

At some point we should probably make a host layer call like:

const char *const char *Host::GetIllegalFilenameCharacters();" so that each host can determine the correct thing for the current host. Then FileSpec can be updated to use this function handle this with a method like "void FileSpec::NormalizePathForHost()". This will probably need to be done elsewhere.

ovyalov updated this revision to Diff 58258.May 24 2016, 9:57 AM
ovyalov edited edge metadata.

Added the check for symbol in [1;31] range.

According to https://msdn.microsoft.com/en-gb/library/windows/desktop/aa365247%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396, characters with codes 1..31 (basically < ' '), are illegal as well. We might as well convert those, just in case.

Agree - done.

I've been sitting on a unit test for the module cache locally for a while now... I guess it's time to put that in, and then we can add a test for this as well.

Sounds good.

At some point we should probably make a host layer call like:

const char *const char *Host::GetIllegalFilenameCharacters();" so that each host can determine the correct thing for the current host. Then FileSpec can be updated to use this function handle this with a method like "void FileSpec::NormalizePathForHost()". This will probably need to be done elsewhere.

I like this idea of generalizing illegal symbols handling on per-platform basis - since Pavel mentioned that 1..31 range of symbols is forbidden in Windows filenames we may have a method like bool Host::IsIllegalFilenameCharacter(char) to tell whether symbol is illegal on this platform.

I like the idea of having a centralized place defining "invalid characters". However, I am not sure if that needs to be (at least in this case) system-dependent. Since presumably the code will need to work on all platforms anyway, having the escaping done in a divergent manner will just complicate testing. I don't see a reason to not define the illegal characters as a union of illegal characters across all platforms (which basically means, using the windows set).

ovyalov closed this revision.May 24 2016, 11:17 AM

Submitted as r270590