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

Ioi Lam ioi.lam at oracle.com
Fri Oct 5 03:01:44 UTC 2018

Hi Gerard,

I can think of another case of mixed file systems. Assuming / is case sensitive, I think your function will treat

/Volume/case_insensitive_disk/foo.jar

/volume/case_insensitive_disk/foo.jar

as the same file, while they are actually different.

Thanks

On 10/4/18 9:56 AM, Gerard Ziemski wrote:

On Oct 4, 2018, at 10:49 AM, Ioi Lam <ioi.lam at oracle.com> wrote:

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[PATHMAX]; 49 os::getcurrentdirectory(cwd, PATHMAX); 50 return (pathconf(cwd, PCCASESENSITIVE) != 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? Filed https://bugs.openjdk.java.net/browse/JDK-8211723, there are other corner cases I listed there that do not work. I think isfilesystemcaseinsensitive 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(PCCASESENSITIVE) returning 0, so the line 50 (and the other case) should be: 50 return (pathconf(cwd, PCCASESENSITIVE) == 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[PATHMAX]; 49 os::getcurrentdirectory(cwd, PATHMAX); If your CWD is /Volumes/caseinsensitivedisk/foo and the path is ../../casesensitivedisk/bar Wouldn't you get an incorrect answer? Another case is, when / is case insensitive CDW = / path is Volumes/casesensitivedisk/foo Interesting corner cases - fixed by turning a relative path into a full path using CWD - please see rev3. bug: https://bugs.openjdk.java.net/browse/JDK-8167546 webrev: http://cr.openjdk.java.net/~gziemski/8167546rev3 testing: Mach5 hs-tier1,2,3,4,5,6,7 (in progress…) cheers

More information about the hotspot-runtime-dev mailing list