RFR: 8265465: jcmd VM.cds should keep already dumped archive when exceptions happens [v2]

[Ioi Lam]

On Wed, 5 May 2021 16:34:20 GMT, Yumin Qi <minqi at openjdk.org> wrote:

>> Hi, Please review 
>>   When using jcmd to dump shared archive, if the archive name exists,  it will be deleted first. If exception happens during dumping process, there is no new archive created. This PR changes to first dump the archive with  a temporary file name. With successful dump, the temporary file will be rename to its given name. This way the old archive will not be touched on exception.
>>   The newly added test case skipped testing on Windows since File operation result is not same as on Linux.
>>   
>>    Tests: tier1,tier2,tier3,tier4
>> 
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JCmdTestFileSecurity.java should be excluded from dynamic test group

Changes requested by iklam (Reviewer).

src/java.base/share/classes/jdk/internal/misc/CDS.java line 284:

> 282: 
> 283:         String tempFileName = getArchiveFileName(archiveFileName);
> 284:         File tempArchiveFile = new File(tempFileName);

I think the logic is too complicated. We can always write the archive to a temp file, and then rename it to the actual file. Also, to be consistent with the other variables, it should be `tempArchiveFileName`


String tempArchiveFileName = archiveFileName + ".tmp";

src/java.base/share/classes/jdk/internal/misc/CDS.java line 288:

> 286:         if (isStatic) {
> 287:             String listFileName = tempFileName + ".classlist";
> 288:             File listFile = new File(listFileName);

There's no need to use tempFileName for the classlist. The list file is always deleted after the dump has finished.

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestFileSecurity.java line 37:

> 35:  * @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
> 36:  * @run main/othervm/timeout=480 -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI JCmdTestFileSecurity
> 37:  */

"Security" is usually used for Java language security. I think it's better to call this test JCmdTestFileSafety.

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestFileSecurity.java line 63:

> 61:         test(localFileName, pid, noBoot,  EXPECT_PASS);
> 62:         File targetFile = CDSTestUtils.getOutputDirAsFile();
> 63:         targetFile.setWritable(false);

getOutputDirAsFile returns the current directory. Other test code may write to this directory. I think it's better to specify:


localFileName = "subdir" + File.separator() + "MyStaticDump.jsa";


and set `subdir` to be non-writeable.

Also, the local variable should be `targetDir` instead of `targetFile`.

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestFileSecurity.java line 92:

> 90:     public static void main(String... args) throws Exception {
> 91:         if (Platform.isWindows()) {
> 92:             // ON windows, File operation resulted difference from other OS.

Could you explain what the differences are?

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

PR: https://git.openjdk.java.net/jdk/pull/3886