RFR: 8273152: Refactor CDS FileMapHeader loading code [v5]

[Ioi Lam]

On Tue, 5 Oct 2021 22:32:30 GMT, Yumin Qi <minqi at openjdk.org> wrote:

>> Please review,
>>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic info from shared archive.
>>   With the change, it makes it possible to read an archive generated by different version of hotspot. Also it is possible to automatically generate a CDS archive If the archive supplied is not readable or fails to pass the check.
>> 
>>   Tests: tier1-4
>>    jtreg on sa.
>> 
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Added a helper class to facilitate checking archive

Looks good overall. Just a few nits.

src/hotspot/share/cds/filemap.cpp line 278:

> 276: }
> 277: 
> 278: // Do not check _magic, it's not been set yet.

I think you can get rid of this comment by moving line 228 .. 231 after line 233.

src/hotspot/share/cds/filemap.cpp line 1099:

> 1097:     lseek(_fd, _header->_base_archive_path_offset, SEEK_SET); // position to correct offset.
> 1098:     size_t n = os::read(_fd, *target, (unsigned int)name_size);
> 1099:     if (n != name_size) {

I think there's no need to do another read. The base name string is already inside the buffer that was read on line 1079. You can just do a `strncpy` into `*target`

src/hotspot/share/cds/filemap.cpp line 1205:

> 1203:   }
> 1204: 
> 1205:   _header = (FileMapHeader*)os::malloc(gen_header->_header_size, mtInternal);

There's no need to allocate and read the header again. It's already in gen_header. This should be enough:


_header = (FileMapHeader*)gen_header;

src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c line 369:

> 367: 
> 368:       // check file magic
> 369:       if (header._generic_header._magic != CDS_ARCHIVE_MAGIC) {

Use `header.magic()` instead?

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

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