RFR 8167546: enhance os::file_name_strncmp() on Mac OSX

[Ioi Lam]

On 10/4/18 8:40 AM, Gerard Ziemski wrote:
> Thank you for the review.
>
>> On Oct 3, 2018, at 7:00 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>> Hi Gerard,
>>
>> I don't know much about Mac OS, so I am just commenting from a C/C++ perspective:
>>
>>   39 #include <unistd.h>
>>
>> This file has already been included above.
> I missed that, fixed.
>
>
>>    46  if (path[0] != '/') {
>>    47     // The path is relative, so use the current working directory
>>    48     char cwd[PATH_MAX];
>>    49     os::get_current_directory(cwd, PATH_MAX);
>>    50     return (pathconf(cwd, _PC_CASE_SENSITIVE) != 1);
>>    51   }
>>
>>
>> https://lists.apple.com/archives/darwin-dev/2007/Apr/msg00036.html suggests that pathconf is not reliable on NFS, SMBFS, etc.
>>
>> Also, what happens if path is a symbolic link that spans across two file systems, where one of them is case sensitive and the other is not?
> I tested symbolic links (on Mac) out of curiosity before and they just don’t work. Whether CDS wants to start supporting them or not is outside the scope here.
>
Hi Gerard,

Could you provide more details? Maybe file a bug?

>> I think is_filesystem_case_insensitive could potentially return a false positive, which would cause two "unequal" file names to appear "equal". This would defeat the pathname validation in CDS.
> The false positive can be fixed by us explicitly checking for pathconf(_PC_CASE_SENSITIVE) returning 0, so the line 50 (and the other case) should be:
>
>   50     return (pathconf(cwd, _PC_CASE_SENSITIVE) == 0);
>
> That means we check using case insensitive comparison only if we are explicitly told by “pathconf” that the underlying file system does not support case sensitive names. If it does, then “pathconf” will return 1, and we know for sure that it’s case sensitive. In all other cases (like on NFS, SMBFS) it returns “-1” and we assume it’s case sensitive too (which may or may not lead to false negative, but is what we do now).
>

That sounds safer.

For the relative path, though, I think we still have a problem:

   46  if (path[0] != '/') {
   47     // The path is relative, so use the current working directory
   48     char cwd[PATH_MAX];
   49     os::get_current_directory(cwd, PATH_MAX);


If your CWD is  /Volumes/case_insensitive_disk/foo

and the path is ../../case_sensitive_disk/bar

Wouldn't you get an incorrect answer?

Another case is, when / is case insensitive

CDW = /
path is Volumes/case_sensitive_disk/foo



Thanks
- Ioi
>> So my recommendation is to not go ahead with this, until we have an actual bug report for it.
> I believe that with the proposed fix above, we only behave differently when we can positively prove that the filesystem is case insensitive, so in all other cases we behave same as today.
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8167546
> webrev: http://cr.openjdk.java.net/~gziemski/8167546_rev2
> testing: Mach5 hs-tier1,2,3,4,5,6,7 (in progress…)
>
>
> cheers
>
>> Thanks
>> - Ioi
>>
>>
>>
>> On 10/03/2018 10:44 AM, Gerard Ziemski wrote:
>>> Hi all,
>>>
>>> Please review this fix, which allows CDS archives to be opened by using case insensitive path name on case insensitive filesystems on Mac OS X.
>>>
>>> Background: this issue only affects Mac OS X, with case insensitive filesystem, for those archives which use “ExtraSharedClassListFile” option to embed the name of the main class, when referring to the original class path using name with different letter case (in other words, uncommon).
>>>
>>> Example: we create archive using:
>>>
>>>   java -cp hello.jar -Xshare:dump -XX:ExtraSharedClassListFile=my.list -XX:SharedArchiveFile=my.jsa
>>>
>>> (where “my.list” contains "Hello”), but then later try running it using this command:
>>>
>>>   java -cp Hello.jar -Xshare:on -XX:SharedArchiveFile=my.jsa Hello
>>>
>>> (notice 1st command refers to “hello.jar”, but 2nd uses “Hello.jar”)
>>>
>>> On Mac with filesystem with case insensitive paths, this currently erroneously fails. I believe that this is technically an issue, but we have not received any real world reports complaining about this. A developer who uses different case path names and assumes that the underlaying system is case insensitive, and therefore such use case should work, is in my opinion asking for trouble, but given the time I spent on this issue so far, I’m inclined to push the fix, so that no one else needs to address it again in the future. Plus, the fix seems reasonably simple.
>>>
>>> CDS archives can be referred to by using relative paths, as in this example (i.e. “hello.jar”), but also using absolute paths (i.e. “/Volumes/Work/hello.jar”) and this fix accounts for both.
>>>
>>> Side note: interestingly enough, one can not create an archive using a relative path to a jar, and then later refer to that jar by using its absolute path, even when those are in fact the same jar file.
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8167546
>>> webrev: http://cr.openjdk.java.net/~gziemski/8167546_rev1
>>> testing: Mach5 hs-tier1,2,3,4,5,6,7
>>>