On Mon, 4 Oct 2021 17:50:27 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: > > Fixed comment
Changes requested by iklam (Reviewer). src/hotspot/share/cds/filemap.cpp line 173: > 171: memset((void*)this, 0, sizeof(FileMapInfo)); > 172: _is_static = is_static; > 173: size_t c_header_size; FileMapInfo::FileMapInfo() is called in two places: (1) When reading an archive -- _header should not be initialized here. It should be done inside FileMapInfo::init_from_file. (2) When writing an archive -- I think we should move the _header initialization to FileMapInfo::populate_header(). src/hotspot/share/cds/filemap.cpp line 197: > 195: _header->set_base_archive_path_offset(0); > 196: _header->set_base_archive_name_size(0); > 197: _header->set_header_size((unsigned int)header_size); The above 3 lines can be replaced by lines 203 .. 205, and 203 .. 205 can be deleted. src/hotspot/share/cds/filemap.cpp line 200: > 198: _header->set_has_platform_or_app_classes(true); > 199: if (!is_static) { > 200: > _header->set_base_archive_is_default(FLAG_IS_DEFAULT(SharedArchiveFile)); There's no need for a separate _base_archive_is_default field. We are using the base archive if base_archive_name_size == 0. src/hotspot/share/cds/filemap.cpp line 1057: > 1055: GenericCDSFileMapHeader* header = > (GenericCDSFileMapHeader*)os::malloc(sz, mtInternal); > 1056: memset((void*)header, 0, sz); > 1057: size_t n = os::read(fd, (void*)header, (unsigned int)sz); There are currently 3 different places where we read the GenericCDSFileMapHeader, find out the size, and then read the real header. - FileMapInfo::get_base_archive_name_from_header - FileMapInfo::check_archive - FileMapInfo::init_from_file I think these should be consolidated into a single function. ------------- PR: https://git.openjdk.java.net/jdk/pull/5768