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

[Ioi Lam]

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.

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