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

[David Holmes]

Looks fine to me.

Thanks,
David

On 12/01/2019 4: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