Log hfs volume identifier#386
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Apple HFS support: APM partition parsing, HFS on-disk definitions, sector-size emulation reader (512B specialization), MacRoman/Pascal string helpers, System::Type::DISK and SystemHFS, and wiring in build and info to choose the correct reader per system type. ChangesHFS File System Support Implementation
Sequence DiagramsequenceDiagram
participant SystemHFS
participant APM_Browser
participant DataReader
participant StringUtils
SystemHFS->>APM_Browser: getPartitions(data_reader)
APM_Browser->>DataReader: read DriveDescriptor and partition entries
DataReader-->>APM_Browser: partition map bytes
APM_Browser-->>SystemHFS: Apple HFS partition entries
SystemHFS->>DataReader: read MDB at partition_start + MDB_OFFSET
DataReader-->>SystemHFS: MDB bytes
SystemHFS->>SystemHFS: validate signature (endian-swap)
SystemHFS->>StringUtils: mac_roman_to_utf8(pascal_volume_name)
StringUtils-->>SystemHFS: UTF-8 volume name
SystemHFS->>SystemHFS: print volume identifier
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@filesystem/apm/apm_browser.ixx`:
- Line 19: The method Browser::getPartitions has clang-format style violations;
fix the formatting inside the Browser class (and the related lines referenced)
so the declaration/definition and access specifier follow the project's
clang-format rules (spacing, brace placement, indentation). Run clang-format on
filesystem/apm/apm_browser.ixx and update the Browser::getPartitions
implementation (and nearby public: sections) to match the formatter output so CI
passes.
- Around line 34-40: The partition entry reads use PARTITION_MAP_OFFSET +
entry_index directly which ignores the data_reader base; update the read offset
to include data_reader->sectorsBase() (e.g. use data_reader->sectorsBase() +
PARTITION_MAP_OFFSET + entry_index) when calling data_reader->read so
partition_map_entry reads respect the reader base. Edit the block around
partition_map_entry, data_reader, PARTITION_MAP_OFFSET and the loop that checks
endian_swap(partition_map_entry.entry_count) to use the base-adjusted offset and
keep the existing signature check with
to_string_view(partition_map_entry.signature).
In `@filesystem/apm/apm_defs.ixx`:
- Line 87: The file has a clang-format style violation on the declaration of the
variable additional_driver_descriptors; run clang-format (or your repo's format
tool) on filesystem/apm/apm_defs.ixx to reformat the file and commit the updated
file so the declaration of uint8_t additional_driver_descriptors[484]; (and any
nearby lines) conform to project formatting rules and unblock CI.
In `@filesystem/apm/apm.ixx`:
- Line 4: The module export line "export import :defs;" in
filesystem/apm/apm.ixx violates clang-format; run the project's
clang-format/style tool on that file, remove any trailing whitespace, ensure the
line matches the repository's module export style (correct spacing and a single
newline at EOF), and commit the reformatted file so CI formatting checks pass.
In `@filesystem/hfs/hfs_defs.ixx`:
- Line 2: Clang-format flagged the include line; edit hfs_defs.ixx so the single
line '`#include` <cstdint>' is properly formatted with a trailing newline (ensure
the file ends with a newline and no extraneous whitespace or blank lines around
the include), then run clang-format (or save with your editor's format hook) to
verify the violation is resolved.
- Around line 34-68: Add a compile-time check that sizeof(MasterDirectoryBlock)
equals 512 bytes by inserting a static assertion referencing
MasterDirectoryBlock (e.g., a static_assert that sizeof(MasterDirectoryBlock) ==
512 with a clear message) immediately after the struct definition so the binary
contract is explicit; if the assertion fails, adjust fields/padding in
MasterDirectoryBlock (or packing) until the size is 512.
In `@filesystem/hfs/hfs.ixx`:
- Line 3: The module export line "export import :defs;" is violating
clang-format; run clang-format (or adjust whitespace) so the export appears on
its own properly formatted line and ensure a terminating newline/EOF;
specifically update the "export import :defs;" line to match the project's
clang-format rules (or reformat the file) so CI formatting checks pass.
In `@info.ixx`:
- Around line 68-80: The code currently constructs emulated_disk_reader =
std::make_shared<Image_Disk_Reader>(form1_reader) unconditionally which can
dereference a null form1_reader; change it to only construct Image_Disk_Reader
when form1_reader is valid (e.g., if (form1_reader) emulated_disk_reader =
std::make_shared<Image_Disk_Reader>(form1_reader); else leave
emulated_disk_reader null) and update the selection logic in the loop that picks
reader for System::Type::DISK to fall back to raw_reader when
emulated_disk_reader is null so you never dereference form1_reader inside
Image_Disk_Reader.
In `@readers/emulated_sector_reader.ixx`:
- Around line 64-72: The sectorsBase() and sectorsCount() implementations
incorrectly divide by _emulated_sectors_per_source; since the wrapper exposes
smaller sectors the visible LBA space must expand, so change both to multiply by
_emulated_sectors_per_source (i.e., return _source->sectorsBase() *
_emulated_sectors_per_source and return _source->sectorsCount() *
_emulated_sectors_per_source), keeping the same return types and using the
existing _source and _emulated_sectors_per_source symbols.
- Around line 51-55: The current copy length (emulated_sectors_to_copy) ignores
the starting offset (data_offset) and can read past source_sectors; compute an
offset-adjusted available count by subtracting the number of emulated sectors
consumed by data_offset (i.e., data_offset / S) from emulated_sectors_available,
clamp that with 0, then use emulated_sectors_to_copy = std::min(count,
adjusted_available); finally only call memcpy when emulated_sectors_to_copy > 0.
Update the logic around emulated_sectors_available, data_offset,
emulated_sectors_to_copy and the memcpy call accordingly.
In `@utils/strings.ixx`:
- Around line 357-360: from_pascal_string reads length from on-disk
pascal_string<M> without validation, so if string.length > M it can read past
data; fix by clamping the length to at most the template capacity M before
constructing the std::string (e.g., compute effective_len =
std::min(string.length, M) and use that when building the std::string),
referencing the from_pascal_string function and pascal_string<M> type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f71c669-8847-4101-bf5f-7aeb3a6646fa
📒 Files selected for processing (13)
CMakeLists.txtfilesystem/apm/apm.ixxfilesystem/apm/apm_browser.ixxfilesystem/apm/apm_defs.ixxfilesystem/hfs/hfs.ixxfilesystem/hfs/hfs_defs.ixxinfo.ixxreaders/emulated_sector_reader.ixxreaders/image_disk_reader.ixxsystems/s_hfs.ixxsystems/system.hhsystems/systems.ixxutils/strings.ixx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
filesystem/apm/apm_browser.ixx (1)
33-33:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPartition entry reads ignore
sectorsBase(), causing wrong offsets.Line 33 reads partition map entries from
PARTITION_MAP_OFFSET + entry_indexwithout includingdata_reader->sectorsBase(), while Line 25 correctly uses the base offset. This inconsistency breaks parsing when the reader base is non-zero, causing the parser to read from incorrect sector addresses.🛠️ Proposed fix
- if(data_reader->read((uint8_t *)&partition_map_entry, PARTITION_MAP_OFFSET + entry_index, 1) != 1) + if(data_reader->read((uint8_t *)&partition_map_entry, data_reader->sectorsBase() + PARTITION_MAP_OFFSET + entry_index, 1) != 1) break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@filesystem/apm/apm_browser.ixx` at line 33, The partition map entry read uses an absolute offset and omits the reader base: update the call to data_reader->read that reads partition_map_entry (using PARTITION_MAP_OFFSET + entry_index) to include data_reader->sectorsBase() just like the earlier read (i.e. add the sectorsBase() value to the offset), so the read uses data_reader->sectorsBase() + PARTITION_MAP_OFFSET + entry_index and remains consistent with the reader's base.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@filesystem/apm/apm_browser.ixx`:
- Line 38: The do/while loop using ++entry_index <
endian_swap(partition_map_entry.entry_count) can spin unbounded if
partition_map_entry.entry_count is corrupted; add a sanity limit and validate
the count before/inside the loop: compute uint32_t count =
endian_swap(partition_map_entry.entry_count), reject or clamp obviously bogus
values (e.g., zero or > MAX_APM_ENTRIES), and change the loop condition to also
break when entry_index >= count or entry_index >= MAX_APM_ENTRIES (define a
named constant like MAX_APM_ENTRIES) so the parser cannot iterate millions of
times on malformed images.
---
Duplicate comments:
In `@filesystem/apm/apm_browser.ixx`:
- Line 33: The partition map entry read uses an absolute offset and omits the
reader base: update the call to data_reader->read that reads partition_map_entry
(using PARTITION_MAP_OFFSET + entry_index) to include data_reader->sectorsBase()
just like the earlier read (i.e. add the sectorsBase() value to the offset), so
the read uses data_reader->sectorsBase() + PARTITION_MAP_OFFSET + entry_index
and remains consistent with the reader's base.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34aa0f08-35c2-4a69-a751-87b7431bd29a
📒 Files selected for processing (5)
filesystem/apm/apm.ixxfilesystem/apm/apm_browser.ixxfilesystem/apm/apm_defs.ixxfilesystem/hfs/hfs.ixxfilesystem/hfs/hfs_defs.ixx
💤 Files with no reviewable changes (1)
- filesystem/apm/apm_defs.ixx
✅ Files skipped from review due to trivial changes (1)
- filesystem/apm/apm.ixx
🚧 Files skipped from review as they are similar to previous changes (2)
- filesystem/hfs/hfs.ixx
- filesystem/hfs/hfs_defs.ixx
superg
reviewed
Jun 10, 2026
superg
left a comment
superg
left a comment
Owner
There was a problem hiding this comment.
It's substantial work, just couple nit comments but overall looking good!
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds basic HFS partition support to log the volume name.
It also includes the following groundwork :
Produced output example:
Summary by CodeRabbit