RFR: 8321183: Incorrect warning from cds about the modules file [v2]

[Maxim Kartashev]

On Fri, 1 Dec 2023 22:28:52 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Corrected tests and the fix
>
> If I understand the bug correct, is the original code at line 413 missing a condition?
> `if (_timestamp != st.st_mtime) {` 
> should be 
> `if (has_timestamp() && (_timestamp != st.st_mtime)) {`
> 
> From your reproducing steps in the bug report:
> "3. replace the .../lib/modules file in the first with the corresponding file from the second, making sure they differ in size"
> After replacing the file, would the new file have a different timestamp?
> 
> Since you've changed the log statement from "A jar file..." to "This file...", the following tests under the appcds directory need to be adjusted:
> WrongClasspath.java
> dynamicArchive/MainModuleOnly.java
> dynamicArchive/WrongTopClasspath.java
> jigsaw/modulepath/MainModuleOnly.java
> 
> One suggestion below.

@calvinccheung 

I made the changes you suggested and ran `test/hotspot/jtreg/runtime/cds`. Please, have a look at the updated fix.


==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/hotspot/jtreg/runtime/cds                292   292     0     0   
==============================
TEST SUCCESS

Finished building target 'run-test-only' in configuration 'linux-x86_64-server-release'

> src/hotspot/share/cds/filemap.cpp line 416:
> 
>> 414:           log_warning(cds)("%s timestamp has changed.", name);
>> 415:         } else {
>> 416:           log_warning(cds)("%s size has changed.", name);
> 
> Instead of `else`, maybe you can do the following because a file could differ both in timestamp and size.
>          if (time_differs) {
>            log_warning(cds)("%s timestamp has changed.", name);
>          } 
>          if (size_differs) {
>            log_warning(cds)("%s size has changed.", name);
>          }

Very true, thanks!

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16926#issuecomment-1838040904
PR Review Comment: https://git.openjdk.org/jdk/pull/16926#discussion_r1413508920