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

[Yumin Qi]

On Wed, 5 May 2021 18:20:43 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> 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
>
> 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";

getArchiveFileName also operates on create/delete temp file, so if that fails, throw exception back to caller. Do you think the logic should be kept?

> 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.

Yes, will revert it back.

> 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.

Sure.

> 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`.

Will try this, not sure if the subdir and the file can be created with current code. May be some extra code needed. 
Thanks for the review!

> 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?

Will add detail explanation.

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

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