[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

[Calvin Cheung]

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
>