RFR(S): 8203354: assert in ClassLoader::update_module_path_entry_list() could have incorrect message

[Ioi Lam]

Looks good. Thanks!

- Ioi


On 5/23/18 11:11 AM, Calvin Cheung wrote:
> Hi Ioi,
>
> On 5/22/18, 10:16 PM, Ioi Lam wrote:
>> Hi Calvin,
>>
>> The C code changes look good.
> Thanks for your review.
>>
>> However, I am not sure if the test case is correct on all file 
>> systems. Does the test assume that the max path length is about 2048?
> Yes, it is because the *nix vresions of the os::stat has an artificial 
> limit of 2K:
> int os::stat(const char *path, struct stat *sbuf) {
>   char pathbuf[MAX_PATH];
>   if (strlen(path) > MAX_PATH - 1) {
>     errno = ENAMETOOLONG;
>     return -1;
>   }
>   os::native_path(strcpy(pathbuf, path));
>   return ::stat(pathbuf, sbuf);
> }
>
>>
>> I think some file systems could allow more than 2048 chars. See 
>> https://serverfault.com/questions/9546/filename-length-limits-on-linux
>>
>> On my Linux workstation:
>> $ getconf PATH_MAX /
>> 4096
> After the fix for JDK-8203680 (filed by Thomas), we should be able to 
> handle path length more than 2K.
>>
>> I think it's better to rewrite the test so that it's OK for the dump 
>> to succeed. If it fails, it must be "os::stat error", but there's no 
>> guarantee that the error code is ENAMETOOLONG. How about this:
>>
>> if (output.getExitValue() != 0) {
>>     output.shouldMatch("os::stat error.*CDS dump aborted");
>> }
> I think it's a good idea to make the test more flexible. Here's an 
> updated webrev:
>     http://cr.openjdk.java.net/~ccheung/8203354/webrev.02/
>
> thanks,
> Calvin
>>
>> Thanks
>> - Ioi
>>
>>
>> On 5/22/18 3:49 PM, Calvin Cheung wrote:
>>> Hi Thomas,
>>>
>>> Thanks for your quick review.
>>>
>>> On 5/22/18, 1:36 PM, Thomas Stüfe wrote:
>>>> Hi Calvin,
>>>>
>>>> some small nits:
>>>>
>>>> - could you use vm_exit_during_initialization instead of raw exit?
>>>> - "Bad path %s" - that is an assumption, stat() may fail for a number
>>>> of reasons. So I would make the message more neutral. I also always
>>>> enclose such an output in quotes, to catch if the caller had
>>>> leading/trailing spaces. So, maybe this: "CDS dump aborted (path was
>>>> \"%s\")." ?
>>> I've fixed the above.
>>> Updated webrev: http://cr.openjdk.java.net/~ccheung/8203354/webrev.01/
>>>>
>>>> Side note: I looked at the various os::stat implementations and see a
>>>> number of issues. We copy the input path around, which is on Posix
>>>> platforms unnecessary since os::native_path() returns just the path;
>>>> and since we use a fixed sized buffer we artificially limit the path
>>>> os::stat() recognizes. That is bad. Also, on windows, we return the
>>>> Win32 error code (GetLastError) as errno, which is just plain wrong.
>>>>
>>>> But these are issues beyond your patch.
>>> Could you file bugs on the above issues?
>>>
>>> thanks,
>>> Calvin
>>>>
>>>> Kind Regards, Thomas
>>>>
>>>>
>>>> On Tue, May 22, 2018 at 9:08 PM, Calvin 
>>>> Cheung<calvin.cheung at oracle.com>  wrote:
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8203354
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8203354/webrev.00/
>>>>>
>>>>> Converting the assert in 
>>>>> ClassLoader::update_module_path_entry_list() to a
>>>>> meaningful error message before aborting the CDS dump.
>>>>>
>>>>> With a module path with very long length (>  2K), the error 
>>>>> message will be
>>>>> like the following:
>>>>>
>>>>> os::stat error 36 (ENAMETOOLONG). CDS dump aborted. Bad path
>>>>> /scratch/myDir/JTwork/scratch/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx<snip> 
>>>>>
>>>>>
>>>>> Note that tty->print_cr() has a buffer size limit of 2000, so the 
>>>>> full path
>>>>> won't be shown entirely. That's why I'd like to put the error code 
>>>>> up front
>>>>> before the path.
>>>>>
>>>>> Ran the modified tests on Oracle platforms.
>>>>>
>>>>> thanks,
>>>>> Calvin
>>