RFR(S) 8216184: CDS/appCDS tests failed on Windows due to long path to a classlist file

[Calvin Cheung]

Hi Ioi, David,

Thanks for your review!

I have added some comment in classListParser.cpp and removed the old 
comment in metaspaceShared.cpp.

Here's an updated webrev:
     http://cr.openjdk.java.net/~ccheung/8216184/webrev.01/

On 1/10/19, 6:25 PM, David Holmes wrote:
> On 11/01/2019 11:59 am, Ioi Lam wrote:
>>
>> On 1/10/19 5:42 PM, David Holmes wrote:
>>> On 11/01/2019 11:23 am, Ioi Lam wrote:
>>>> Hi Calvin,
>>>>
>>>> Maybe you can use os::fopen() instead of 2 calls to overloaded 
>>>> versions of os::open()?
>>>
>>> os::fopen still calls ::fopen so will have the exact same problem on 
>>> Windows. os::fopen only exists to add close-on-exec behaviour.
>>>
>> Ah, maybe we should add a comment in the new code to explain why this 
>> gymnastic is necessary?
>
> +1
>
> That said, from what I've read it should suffice to pre-pend "\\?\" 
> provided we use absolute paths:
>
> https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#maximum-path-length-limitation 
>
The above doc also mentions:
"The Windows API has many functions that also have Unicode versions to 
permit an extended-length path for a maximum total path length of 32,767 
characters."

The path needs to be converted to a wide character string in order to 
use the Unicode versions of the functions. The conversion accounts for 
some of the complexities in the fix for 8188122.

thanks,
Calvin
>
> The os::open fix for 8188122 is rather more complicated though, so 
> perhaps simplest to just use that as-is.
>
> David
>
>> Thanks
>>
>> - Ioi
>>
>>> There are numerous uses of plain fopen in shared code in the VM, 
>>> many of which may also be problematic.
>>>
>>> In relation to CDS, this comment in
>>>
>>> ./share/memory/metaspaceShared.cpp
>>>
>>> seems out-of-place and likely no longer relevant, as I see no fopern 
>>> use there
>>>
>>>     // Preload classes to be shared.
>>>     // Should use some os:: method rather than fopen() here. aB.
>>>
>>> Otherwise looks good to me.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> The rest looks good.
>>>>
>>>> Thanks
>>>>
>>>> - Ioi
>>>>
>>>> On 1/10/19 12:08 PM, Calvin Cheung wrote:
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8216184
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8216184/webrev.00/
>>>>>
>>>>> We are using fopen() to open a classlist file. fopen() can't 
>>>>> handle long path name longer than MAX_PATH.
>>>>>
>>>>> A fix is to use os::open() which is capable of handling long path 
>>>>> name after the fix for JDK-8188122.
>>>>>
>>>>> Testing: mach5 hs-tier{1,2,3}.
>>>>>
>>>>> thanks,
>>>>> Calvin