[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]

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