On Wed, 5 May 2021 18:20:43 GMT, Ioi Lam <ik...@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
