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

[Thomas Stüfe]

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