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

[Calvin Cheung]

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
>