On Tue, 5 Oct 2021 22:32:30 GMT, Yumin Qi <mi...@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