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

Reply via email to