On Wed, 5 May 2021 16:34:20 GMT, Yumin Qi <[email protected]> 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
