RFR(S) 8190737: use unicode version of the canonicalize() function to handle long path on windows

[Calvin Cheung]

Hi Sherman,

I'll change the comment to:

130             fname,              /* path name in multibyte char */

thanks,
Calvin

On 9/12/18, 6:52 PM, Xueming Shen wrote:
> Hi Calvin,
>
> I believe I was thinking of keeping the "getPrefixed" as an 
> implementation details and
> instead exporting a "A" version of winFileHandleOpen() back then. But 
> I don't have a
> strong opinion on this one, so your approach looks fine.
>
> No, I don't have a better idea for now to avoid those mb->ws->mb as 
> long as the canonicalize()
> and zip_open() are two separate invocations. But since they are only 
> for > max_path path. I'm
> fine with it.
>
> Yes, I think it's time to consider to migrate from interpreting the 
> char* as "mb chars" to probably
> "utf8 chars" (take a step back, I don't think it's easy to do to 
> actually a wchar interface) for windows
> platform, but that would be a separate and big rfes.
>
> nit:
>
> 130             fname,              /* Ascii char path name */
>
> the comment probably should be "path name in mb char", or ANSI charset.
>
> Thanks,
>
> -Sherman
>
> On 9/12/18, 4:16 PM, Calvin Cheung wrote:
>> Hi Sherman,
>>
>> Thanks for your review.
>> Please refer to my reply below...
>>
>> On 9/10/18, 5:05 PM, Xueming Shen wrote:
>>> On 9/10/18, 11:42 AM, Calvin Cheung wrote:
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8190737
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ccheung/8190737/webrev.00/
>>>>
>>>> Please review this change for handling long path specified in the 
>>>> -Xbootclasspath/a on windows.
>>>>
>>>> Highlight of changes:
>>>> - canonicalize_md.c
>>>>     it makes use of the unicode version of canonicalize 
>>>> (wcanonicalize) if the original path is >= MAX_PATH.
>>> So we are converting mb->wc->mb in wcanonicalize (when > max_path), 
>>> and then feed the
>>> resulting path into ZFILE_Open, in which we again do mb->wc-> + 
>>> prefix -> CreateFileW(...)?
>> That's the minimal change I could come up with. Let me know if you 
>> have other suggestions.
>>>
>>> Maybe it's time to consider a "wchar" interface between vm and 
>>> libraries.
>> Good idea. I think it should be done in a separate RFE.
>>> Maybe it's fine here given we
>>> are only do this for > max_path case?
>> This was done so that this change has no impact on the <= MAX_PATH case.
>>>
>>> Alan, now reading the win version canonicalize(...), remind me 
>>> what's the real purpose of doing
>>> FindFirstFile/FindClose() here? the API appears to suggest it is 
>>> used to find the first match if there
>>> is/are wildcards but we actually have checked/rejected the wildcards 
>>> at the very beginning ? To
>>> get the "real name" for case?
>>>
>>>>
>>>> - zip_util.c
>>>>     it uses the unicode version of CreateFile (CreateFileW) if the 
>>>> original path is >= MAX_PATH.
>>>>
>>>> - io_util_md.c
>>>>     Since zip_util.c (libzip) calls the getPrefixed() function in 
>>>> canonicalize_md.c (libjava), the getPrefixed() function needs to be 
>>>> exported.
>>>
>>> I kinda remembered that we once wanted to avoid export an variant of 
>>> winFileHandleOpen() from
>>> libjava to libzip ... in this case the alternative is to simply 
>>> copy/paste the getPrefix into libzip ...
>>> but I don't remember the exact concern back then.
>> I also thought of copy/paste the getPrefix function into libzip.
>> After looking at the lib/CoreLibraries.gmk, I think libzip already 
>> has a dependency on libjava:
>> $(eval $(call SetupJdkLibrary, BUILD_LIBZIP, \
>>     NAME := zip, \
>>     OPTIMIZATION := LOW, \
>>     EXCLUDES := $(LIBZIP_EXCLUDES), \
>>     CFLAGS := $(CFLAGS_JDKLIB) \
>>         $(LIBZ_CFLAGS), \
>>     CFLAGS_unix := $(BUILD_LIBZIP_MMAP) -UDEBUG, \
>>     LDFLAGS := $(LDFLAGS_JDKLIB) \
>>         $(call SET_SHARED_LIBRARY_ORIGIN), \
>>     LIBS_unix := -ljvm -ljava $(LIBZ_LIBS), \
>>     LIBS_windows := jvm.lib $(WIN_JAVA_LIB), \
>> ))
>>
>> I've done tier1,2,3 testing and didn't see any new failures.
>> I can do higher tier testing if needed.
>>
>> thanks,
>> Calvin
>>
>> p.s. I didn't see this email until Ioi forwarded it to me since I 
>> wasn't in the core-libs-dev alias. (I've subscribed to the alias this 
>> morning.)
>>
>>>
>>> -Sherman
>>>
>>>
>>>
>>>>
>>>>
>>>> - java_md.h
>>>>     The JVM_MAXPATHLEN has been increased from _MAX_PATH to 1024. 
>>>> It is because the the current usage of the canonicalize() function 
>>>> from vm code is to preallocate the output buffer as follows:
>>>>             char* canonical_path = 
>>>> NEW_RESOURCE_ARRAY_IN_THREAD(thread, char, JVM_MAXPATHLEN);
>>>>             if (!get_canonical_path(path, canonical_path, 
>>>> JVM_MAXPATHLEN)) {
>>>>     Also the unix version of canonicalize() function has the 
>>>> following check:
>>>>             int
>>>>            canonicalize(char *original, char *resolved, int len)
>>>>            {
>>>>                if (len < PATH_MAX) {
>>>>                errno = EINVAL;
>>>>                return -1;
>>>>            }
>>>>    So dynamically allocating the output buffer requires more work 
>>>> beyond the scope of this change.
>>>>
>>>> - LongBCP.java
>>>>     added a scenario to the test - a long path to a jar file in 
>>>> -Xbootclasspath/a.
>>>>
>>>> Testing: tier-[1,2,3].
>>>>
>>>> thanks,
>>>> Calvin
>>>
>