On Tue, 5 Oct 2021 22:32:30 GMT, Yumin Qi <[email protected]> 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
