On Wed, 5 May 2021 16:34:20 GMT, Yumin Qi <mi...@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