Conversation
The `IsoBuilder::build` method has been updated to accept `esp_lba` and `esp_size_sectors` as `Option<u32>`. This provides greater flexibility in ISO image creation, allowing for scenarios where an EFI System Partition (ESP) is not required or its parameters are determined dynamically. The logic for writing GPT structures and calculating the ISO9660 data start LBA has been adjusted to conditionally handle the presence of ESP parameters. Additionally, the `build_iso` function now returns the `NamedTempFile` associated with the temporary FAT image. This ensures that the temporary file, which holds the UEFI boot image, is managed by the caller, preventing its premature deletion and ensuring its availability throughout the ISO building process.
This commit refactors parts of the FAT image creation and ISO builder logic to improve code organization and clarity. In `src/fat.rs`: - The `copy_to_fat` function has been moved from the `utils` module and made a private helper function within `fat.rs`. This improves encapsulation as the function is only used in this module. In `src/iso/builder.rs`: - The `get_lba_for_path` function is simplified by leveraging a new internal helper, making the path resolution logic cleaner and more robust. - Calls to `seek_and_pad_to_lba` have been replaced with `seek_to_lba`, streamlining the file writing process.
Moved several ISO-related data structures and constants from `src/iso/builder.rs` to new, more specific modules. The following types and constants were relocated: - `IsoFile`, `IsoDirectory`, `IsoFsNode` to `src/iso/fs_node.rs` - `BootInfo`, `BiosBootInfo`, `UefiBootInfo` to `src/iso/boot_info.rs` - `IsoImage`, `IsoImageFile` to `src/iso/iso_image.rs` - `ESP_START_LBA` to `src/iso/constants.rs` This refactoring improves modularity and code organization by separating concerns. `builder.rs` now focuses solely on the ISO image construction logic, while the new modules define their respective data structures, making them more discoverable and reusable. The public API (`src/lib.rs`) has been updated to re-export these types for external use.
This commit modularizes the ISO builder implementation by extracting utility functions into separate modules. The changes include: - Moving calculation and helper functions to builder_utils module - Creating iso_writer module for ISO writing operations - Reorganizing imports to reflect the new module structure - Improving code organization and maintainability This refactoring makes the codebase more modular and easier to understand while maintaining the same functionality.
- Replace direct io::Error::new() calls with io_error! macro for better context - Extract boot catalog entry creation into separate helper functions - Use get_file_metadata() instead of direct std::fs::metadata() calls - Simplify BIOS and UEFI boot entry creation
Summary of ChangesHello @p14c31355, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a substantial refactoring of the ISO image building logic, aiming to enhance modularity, maintainability, and testability. Key components previously co-located in a single builder file have been reorganized into a more granular module structure, separating concerns such as boot information, filesystem node definitions, GPT handling, and core writing operations. Additionally, error handling has been standardized through new utility macros, and the process for creating UEFI bootable ISOs has been refined and thoroughly tested with a new integration test. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a significant and well-executed refactoring. The iso module has been broken down into smaller, more focused sub-modules, which greatly improves code organization, readability, and maintainability. The introduction of helper modules like builder_utils.rs and iso_writer.rs, and the separation of data structures into their own files (boot_info.rs, fs_node.rs, etc.) are excellent changes. The GPT logic is also now much cleaner in its own module. I've found a couple of issues, one of which is critical, but overall this is a very positive change for the codebase.
| /// Returns the name of the node. | ||
| pub fn name(&self) -> String { | ||
| match self { | ||
| IsoFsNode::File(file) => file.path.file_name().unwrap().to_str().unwrap().to_string(), | ||
| IsoFsNode::Directory(dir) => dir.children.get(".").unwrap().name(), // This is a bit of a hack, assuming "." is always present and is the directory name | ||
| } | ||
| } |
There was a problem hiding this comment.
The name() method for IsoFsNode::Directory is implemented in a way that will cause a panic. It tries to access dir.children.get("."), but the in-memory IsoDirectory representation does not contain . or .. entries; these are synthesized during ISO creation. This will result in an unwrap() on a None value. Even if it didn't panic, it would lead to infinite recursion as it would call name() on itself.
Since a node's name is determined by its key in the parent's children map, a node doesn't inherently know its own name. Given this method is not used elsewhere, the safest and cleanest solution is to remove it.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini summary |
Summary of ChangesThis pull request undertakes a significant refactoring of the ISO image creation process, aiming to improve the overall architecture and robustness of the Highlights
Changelog
Activity
|
1 participant
Pull Request
Change details
Build / Test Results