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

[Ioi Lam]

Hi Calvin,

The C code changes look good.

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?

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

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");
}

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