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

[Calvin Cheung]

On 1/11/19, 10:58 AM, Ioi Lam wrote:
> Hi Calvin, I would suggest changing the comment to
>
>   51   // Use os::open() because neither fopen() nor os::fopen()
>        //can handle long path name on Windows.
>
> For conciseness, I think "to open the classlist file" can be omitted.
>
> No need for a new webrev if you decide to take my suggestion.
I have updated the comment.

thanks,
Calvin
>
> Thanks
>
> - Ioi
>
>
>
> On 1/11/19 10:06 AM, Calvin Cheung wrote:
>> 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