[14] RFR(M): 8211723: AppCDS: referring to a jar file in any way other than exactly how it was done during dumping doesn't work

[Ioi Lam]

Looks good. Thanks
- Ioi

On 6/24/19 5:31 PM, Calvin Cheung wrote:
> Hi Ioi,
>
> Thanks for your review!
>
> I've made all your suggested changes.
> I also excluded the runtime/appcds/RelativePath.java from the 
> hotspot_appcds_dynamic test group because:
> 1. it fails in TestCommon.patchJarForDynamicDump():
>     if (!firstJar.startsWith(expected1) && 
> !firstJar.startsWith(expected2)) {
>             throw new RuntimeException("FIXME: jar file not at a 
> supported location ('"
>                                        + expected1 + "', or '" + 
> expected2 + "'): " + firstJar);
>         }
> 2. similar test exists in 
> runtime/appcds/dynamicArchive/RelativePath.java.
>
> Updated webrevs:
>     incremental: http://cr.openjdk.java.net/~ccheung/8211723/delta_00_01/
>     full: http://cr.openjdk.java.net/~ccheung/8211723/webrev.01/
>
> thanks,
> Calvin
>
> On 6/24/19, 11:25 AM, Ioi Lam wrote:
>> Hi Calvin,
>>
>> The changes look good to me (I've already reviewed a preliminary of 
>> this patch internally). Just a few small comments:
>>
>>
>> filemap.cpp:
>>
>>  573   while (end_ptr != NULL) {
>>  574     if ((end_ptr - begin_ptr) > 1 || *end_ptr == '\0') {
>>  575       struct stat st;
>>  576       char* temp_name = NEW_RESOURCE_ARRAY(char, 
>> (size_t)(end_ptr - begin_ptr + 1));
>>  577       strncpy(temp_name, begin_ptr, end_ptr - begin_ptr);
>>  578       temp_name[end_ptr - begin_ptr] = '\0';
>>  579       if (os::stat(temp_name, &st) == 0) {
>>  580         path_array->append(temp_name);
>>  581       }
>>
>> I think the (*end_ptr == '\0') is not needed. Otherwise if you
>> have -cp "foo.jar:", you will end up having temp_name == "".
>>
>> ----
>>
>> bool FileMapInfo::validate_shared_path_table(bool is_static);
>> bool FileMapInfo::validate_boot_class_paths(bool is_static);
>>
>> Instead of passing is_static, how about making these two functions a 
>> member function?
>> Then, in dynamicArchive.cpp, you can do this:
>>
>>   if (!dynamic_info->validate_shared_path_table()) {
>>
>> and in here:
>>
>> bool FileMapInfo::validate_boot_class_paths() {
>>   ....
>> bool relaxed_check = header()->has_platform_or_app_classes() :
>>
>>
>> ----
>>
>> I'd suggest adding the following comment:
>>
>> bool FileMapInfo::check_paths(int shared_path_start_idx, int 
>> num_paths, GrowableArray<char*>* rp_array) {
>>   int i = 0;
>>   int j = shared_path_start_idx;
>>   bool mismatch = false;
>>   while (i < num_paths && !mismatch) {
>>     while (shared_path(j)->from_class_path_attr()) {
>>       // shared_path(j) was expanded from the JAR file attribute 
>> "Class-Path:"
>>       // during dump time. It's not included in the -classpath VM 
>> argument.
>>       j++;
>>     }
>>
>>
>> ----
>> os_windows.cpp:
>>
>> static HANDLE open_file(const char* file) {
>>
>> I think this should be renamed to something like 
>> "create_read_only_file_handle". That way it
>> will be clear that this function is not a generic function for 
>> "opening a file" so it's less
>> likely to be mis-used in the future.
>>
>> Thanks
>> - Ioi
>>
>>
>>
>>
>> On 6/20/19 3:32 PM, Calvin Cheung wrote:
>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8211723
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8211723/webrev.00/
>>>
>>> Currently, the class paths comparison between AppCDS dump time and 
>>> runtime is done via the os::file_name_strncmp() function. It works 
>>> well only for absolute paths. This RFE enhances it by allowing 
>>> relative path and sym link.
>>>
>>> Highlight of changes:
>>> - the os::file_name_strncmp() and its usages have been removed;
>>>
>>> - a _from_class_path_attr field of type bool has been added to the 
>>> ClassPathZipEntry to indicate if the entry is from the “Class-path” 
>>> attribute of a jar file;
>>>
>>> - FileMapInfo::same_files() has been separated into posix 
>>> (os_posix.cpp) and windows (os_windows.cpp) versions.
>>>
>>> - checking of the boot and the app class paths have been moved from 
>>> SharedPathsMiscInfo::check() to 
>>> FileMapInfo::validate_shared_path_table();
>>>
>>> - for checking the runtime app class paths against the dump time, 
>>> Arguments::get_appclasspath() is used for getting the runtime app 
>>> class paths and are stored into an array. The array of runtime app 
>>> class paths will be checked against the shared_path from the shared 
>>> archive one by one. If a shared_path is from the “Class-path” 
>>> attribute of a jar file, it will be skipped. The same is being done 
>>> currently; i.e. we don’t expand the class paths obtained from 
>>> Arguments::get_appclasspath() if any of the jar refers to other jars 
>>> by means of the “Class-path” attribute. Some sanity checks including 
>>> file size and modification time is being done currently for all the 
>>> jars in the shared_path. This remains the same with this change.
>>>
>>> - boot class paths are being checked in a similar way to app class 
>>> paths. Some specifics such as the notion of “relaxed_check” remains 
>>> unchanged;
>>>
>>> - test utilities and new tests.
>>>
>>> Testing: tier1 through tier3 on linux, windows, macosx, solaris.
>>>
>>> thanks,
>>> Calvin
>>