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

[Calvin Cheung]

Hi Thomas,

Thanks for taking another look and filing the bug.

thanks,
Calvin

On 5/22/18, 10:02 PM, Thomas Stüfe wrote:
> Hi Calvin,
>
> On Wed, May 23, 2018 at 12:49 AM, Calvin Cheung
> <calvin.cheung at oracle.com>  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/
> Looks good now.
>
>>>
>>> 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?
>>
> Done: https://bugs.openjdk.java.net/browse/JDK-8203680
>
> Thanks!
>
> ..Thomas
>
>> 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