Conversation
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 significantly refactors the ISO image creation process by introducing a dedicated 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 pull request introduces a new, more flexible IsoBuilder for creating ISO images, which is a significant improvement. It allows for more complex file structures and boot configurations. I've provided feedback on several points, including a potential panic from an unwrap(), redundant file cleanup logic, and opportunities for code enhancement such as replacing floating-point math with integer arithmetic for sector calculations and refining the IsoBuilder API. Additionally, I've noted a few minor issues like a typo and some leftover debugging print statements.
| // Clean up temporary FAT image if created | ||
| if let Some(path) = uefi_fat_img_path { | ||
| std::fs::remove_file(path)?; | ||
| } |
There was a problem hiding this comment.
The temporary file for the FAT image is managed by temp_fat_file_holder, which is a NamedTempFile. NamedTempFile automatically handles file deletion when it goes out of scope. Manually removing the file using std::fs::remove_file is not only redundant but can also lead to an error if the NamedTempFile's destructor runs after the file has already been deleted. It's best to rely solely on NamedTempFile for cleanup.
| println!("create_fat_image: fat_img_path: {:?}", fat_img_path); | ||
| println!("create_fat_image: loader_path: {:?}", loader_path); | ||
| println!("create_fat_image: kernel_path: {:?}", kernel_path); |
There was a problem hiding this comment.
This file includes several println! statements that appear to be for debugging. For better maintainability, it's recommended to use a logging facade like log or tracing. This provides more flexible control over log verbosity and output at runtime compared to hardcoded print statements. These debug prints should be removed before merging.
| self.boot_info = Some(boot_info); | ||
| } | ||
|
|
||
| pub fn build(&mut self, iso_path: &Path) -> io::Result<()> { |
There was a problem hiding this comment.
The build method takes &mut self, which implies it can be called multiple times. However, the builder's state, such as current_lba, is not reset between calls. This could lead to a corrupted ISO on subsequent builds. To ensure the builder is used correctly, consider changing the method to consume the builder by taking self (i.e., fn build(self, ...)). This enforces that build is a final, one-time operation.
| fn get_lba_for_path(&self, path: &str) -> io::Result<u32> { | ||
| let mut current_dir = &self.root; | ||
| let components: Vec<_> = Path::new(path).components().collect(); | ||
|
|
||
| for (i, component) in components.iter().enumerate() { | ||
| let component_name = component.as_os_str().to_str().ok_or_else(|| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "Invalid path component") | ||
| })?; | ||
| if i == components.len() - 1 { | ||
| // Last component, could be file or directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::File(file)) => return Ok(file.lba), | ||
| Some(IsoFsNode::Directory(dir)) => return Ok(dir.lba), | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } else { | ||
| // Intermediate component, must be a directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::Directory(dir)) => current_dir = dir, | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )) | ||
| } | ||
|
|
||
| fn get_file_size_in_iso(&self, path: &str) -> io::Result<u64> { | ||
| let mut current_dir = &self.root; | ||
| let components: Vec<_> = Path::new(path).components().collect(); | ||
|
|
||
| for (i, component) in components.iter().enumerate() { | ||
| let component_name = component.as_os_str().to_str().ok_or_else(|| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "Invalid path component") | ||
| })?; | ||
| if i == components.len() - 1 { | ||
| // Last component, must be a file | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::File(file)) => return Ok(file.size), | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } else { | ||
| // Intermediate component, must be a directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::Directory(dir)) => current_dir = dir, | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )) | ||
| } |
There was a problem hiding this comment.
The functions get_lba_for_path and get_file_size_in_iso share very similar logic for traversing path components to find a node. You can reduce this code duplication by creating a private helper function that takes a path and returns the corresponding IsoFsNode. The public methods can then call this helper and extract the specific information they need (lba or size).
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new, more flexible IsoBuilder for creating ISO images, which is a great improvement. The new builder allows for a more complex file structure within the ISO. My review focuses on improving robustness, consistency, and maintainability. I've pointed out some issues with temporary file handling, potential integer overflows, and code duplication. I've also noted some leftover debugging statements and an unused struct field.
| let boot_image_size = std::fs::metadata(&bios_boot.boot_image)?.len(); | ||
| let boot_image_sectors = (boot_image_size as f64 / 512.0).ceil() as u16; |
There was a problem hiding this comment.
The size of the boot image in 512-byte sectors is calculated and cast to u16 without checking for overflow. The El Torito boot catalog entry stores this as a u16. If the boot image is larger than u16::MAX * 512 bytes (about 32MB), this will overflow and lead to a corrupt ISO. An overflow check should be added.
let boot_image_size = std::fs::metadata(&bios_boot.boot_image)?.len();
let boot_image_sectors_u64 = boot_image_size.div_ceil(512);
if boot_image_sectors_u64 > u16::MAX as u64 {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"BIOS boot image is too large for the boot catalog",
));
}
let boot_image_sectors = boot_image_sectors_u64 as u16;| let uefi_fat_img_size = self.get_file_size_in_iso(&uefi_boot.destination_in_iso)?; | ||
| let uefi_fat_img_sectors = (uefi_fat_img_size as f64 / 512.0).ceil() as u16; |
There was a problem hiding this comment.
The size of the UEFI boot image in 512-byte sectors is calculated and cast to u16 without checking for overflow. The El Torito boot catalog entry stores this as a u16. If the boot image is larger than u16::MAX * 512 bytes (about 32MB), this will overflow and lead to a corrupt ISO. An overflow check should be added.
let uefi_fat_img_size = self.get_file_size_in_iso(&uefi_boot.destination_in_iso)?;
let uefi_fat_img_sectors_u64 = uefi_fat_img_size.div_ceil(512);
if uefi_fat_img_sectors_u64 > u16::MAX as u64 {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"UEFI boot image is too large for the boot catalog",
));
}
let uefi_fat_img_sectors = uefi_fat_img_sectors_u64 as u16;|
|
||
| #[derive(Clone)] | ||
| pub struct BiosBootInfo { | ||
| pub boot_catalog: PathBuf, |
There was a problem hiding this comment.
The boot_catalog field in BiosBootInfo seems to be unused in the ISO building process. The boot catalog's location is determined by the constant LBA_BOOT_CATALOG. If this field is indeed unnecessary, it should be removed to simplify the API and avoid confusion.
| if let Some(path) = uefi_fat_img_path { | ||
| std::fs::remove_file(path)?; | ||
| } |
There was a problem hiding this comment.
This manual removal of the temporary FAT image is redundant. The temp_fat_file_holder, which holds the NamedTempFile, will automatically delete the file when it goes out of scope at the end of the function. Relying on RAII for this is cleaner and safer.
| println!("create_fat_image: fat_img_path: {:?}", fat_img_path); | ||
| println!("create_fat_image: loader_path: {:?}", loader_path); | ||
| println!("create_fat_image: kernel_path: {:?}", kernel_path); |
There was a problem hiding this comment.
These println! statements appear to be for debugging. They should be removed or replaced with a proper logging framework (e.g., the log crate) to provide configurable and structured logging. The same applies to other new println! calls in this function.
|
|
||
| // Update PVD total sectors | ||
| let final_pos = iso_file.stream_position()?; | ||
| self.total_sectors = (final_pos as f64 / ISO_SECTOR_SIZE as f64).ceil() as u32; |
There was a problem hiding this comment.
For consistency with other parts of the code (e.g., calculate_lbas) and to avoid potential floating-point inaccuracies, it's better to use integer arithmetic for calculating the total number of sectors.
self.total_sectors = final_pos.div_ceil(ISO_SECTOR_SIZE as u64) as u32;| fn get_lba_for_path(&self, path: &str) -> io::Result<u32> { | ||
| let mut current_dir = &self.root; | ||
| let components: Vec<_> = Path::new(path).components().collect(); | ||
|
|
||
| for (i, component) in components.iter().enumerate() { | ||
| let component_name = component.as_os_str().to_str().ok_or_else(|| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "Invalid path component") | ||
| })?; | ||
| if i == components.len() - 1 { | ||
| // Last component, could be file or directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::File(file)) => return Ok(file.lba), | ||
| Some(IsoFsNode::Directory(dir)) => return Ok(dir.lba), | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } else { | ||
| // Intermediate component, must be a directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::Directory(dir)) => current_dir = dir, | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )) | ||
| } | ||
|
|
||
| fn get_file_size_in_iso(&self, path: &str) -> io::Result<u64> { | ||
| let mut current_dir = &self.root; | ||
| let components: Vec<_> = Path::new(path).components().collect(); | ||
|
|
||
| for (i, component) in components.iter().enumerate() { | ||
| let component_name = component.as_os_str().to_str().ok_or_else(|| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "Invalid path component") | ||
| })?; | ||
| if i == components.len() - 1 { | ||
| // Last component, must be a file | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::File(file)) => return Ok(file.size), | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } else { | ||
| // Intermediate component, must be a directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::Directory(dir)) => current_dir = dir, | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )) | ||
| } |
There was a problem hiding this comment.
The functions get_lba_for_path and get_file_size_in_iso contain very similar logic for traversing the directory tree. This duplication could be avoided by creating a private helper function, for example get_node_for_path(&self, path: &str) -> io::Result<&IsoFsNode>, which would find and return the node. Then, get_lba_for_path and get_file_size_in_iso could call this helper and extract the required information from the returned node. This would make the code more maintainable.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the ISO creation logic by implementing a more flexible IsoBuilder. The new design, which uses an in-memory filesystem representation, is a great step towards supporting more complex ISO layouts and boot configurations. The code is generally well-structured, and the inclusion of a new integration test is commendable.
My review focuses on several key areas for improvement:
- Resource Management: There are opportunities to make temporary file handling more robust by consistently using RAII guards to prevent file leaks.
- Correctness and Robustness: I've identified a few places where using floating-point math or silent type-casting could lead to incorrect behavior or corrupt ISOs, especially with large files. Switching to integer arithmetic and adding explicit checks would improve this.
- Code Clarity and Maintainability: Some parts of the new builder can be simplified by reducing code duplication and clarifying the state management within the
IsoBuilderstruct.
Addressing these points will help solidify this refactoring and ensure the new builder is as robust and maintainable as it is flexible.
| dir_content.extend_from_slice( | ||
| &IsoDirEntry { | ||
| lba: file.lba, | ||
| size: file.size as u32, | ||
| flags: 0x00, | ||
| name, | ||
| } | ||
| .to_bytes(), | ||
| ); |
There was a problem hiding this comment.
The file size file.size (u64) is directly cast to u32 for the IsoDirEntry's size field. This can cause silent truncation for files larger than 4GB, leading to a corrupt directory entry and an invalid ISO. You should add a check to ensure the file size fits within a u32 and return an error if it doesn't, as the ISO9660 format has this limitation.
let file_size_u32 = u32::try_from(file.size).map_err(|_| {
io::Error::new(
io::ErrorKind::InvalidInput,
format!("File '{}' is too large for ISO9660", name),
)
})?;
dir_content.extend_from_slice(
&IsoDirEntry {
lba: file.lba,
size: file_size_u32,
flags: 0x00,
name,
}
.to_bytes(),
);|
|
||
| // Update PVD total sectors | ||
| let final_pos = iso_file.stream_position()?; | ||
| self.total_sectors = (final_pos as f64 / ISO_SECTOR_SIZE as f64).ceil() as u32; |
There was a problem hiding this comment.
Using floating-point arithmetic for calculating the number of sectors can be imprecise and is generally discouraged for this type of calculation. It's safer to use integer arithmetic. Furthermore, the result is cast to u32 without checking for overflow, which could lead to silent truncation for images larger than ~8TB. An explicit, checked conversion should be used.
let total_sectors_u64 = final_pos.div_ceil(ISO_SECTOR_SIZE as u64);
self.total_sectors = u32::try_from(total_sectors_u64).map_err(|_| {
io::Error::new(io::ErrorKind::InvalidInput, "ISO image is too large")
})?;| // Create a dummy kernel path in the same directory as the ISO output | ||
| // This ensures it lives long enough and is cleaned up with the temp_dir of the test. | ||
| let dummy_kernel_path = iso_path | ||
| .parent() | ||
| .ok_or_else(|| { | ||
| io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "ISO path has no parent directory", | ||
| ) | ||
| })? | ||
| .join("dummy_kernel_for_fat"); | ||
| std::fs::write(&dummy_kernel_path, b"")?; | ||
| dummy_kernel_file_for_fat_creation = Some(dummy_kernel_path.clone()); |
There was a problem hiding this comment.
The dummy kernel file is created manually in the parent directory of the output ISO and its cleanup is handled manually. This approach has two issues:
- The parent directory may not be writable, causing the operation to fail.
- If the function panics before cleanup, the temporary file will be left on disk.
It's better to use tempfile::NamedTempFile for this, just as you do for the FAT image. This will create the file in a proper temporary location and ensure it is always cleaned up via its RAII guard.
| println!("create_fat_image: fat_img_path: {:?}", fat_img_path); | ||
| println!("create_fat_image: loader_path: {:?}", loader_path); | ||
| println!("create_fat_image: kernel_path: {:?}", kernel_path); |
There was a problem hiding this comment.
These println! statements appear to be for debugging purposes. For production-quality code, it's better to use a logging facade like the log crate, which allows for configurable log levels and output destinations. These direct prints should be removed to avoid cluttering the standard output of the tool.
| for entry_data in entries { | ||
| let mut entry = [0u8; 32]; | ||
| entry[0] = if entry_data.bootable { | ||
| BOOT_CATALOG_BOOT_ENTRY_HEADER_ID | ||
| } else { | ||
| 0x00 | ||
| }; // Bootable or not | ||
| entry[1] = 0x00; // No Emulation | ||
| entry[2..4].copy_from_slice(&0u16.to_le_bytes()); // Load Segment | ||
| entry[4] = entry_data.platform_id; // System Type | ||
| entry[5] = 0x00; // Unused | ||
| entry[6..8].copy_from_slice(&entry_data.boot_image_sectors.to_le_bytes()); // Sector count (u16, in 512-byte sectors) | ||
| entry[8..12].copy_from_slice(&entry_data.boot_image_lba.to_le_bytes()); | ||
| catalog[offset..offset + 32].copy_from_slice(&entry); | ||
| offset += 32; | ||
| } |
There was a problem hiding this comment.
The logic to create boot entries is well-structured. However, if the number of entries becomes large, this loop could potentially write beyond the bounds of the catalog array, which is fixed at ISO_SECTOR_SIZE. The El Torito specification allows for a boot catalog to span multiple sectors, but the current implementation does not support this. For now, it would be safer to add a check to ensure offset does not exceed catalog.len().
|
|
||
| pub struct IsoBuilder { | ||
| root: IsoDirectory, | ||
| iso_file: Option<File>, |
There was a problem hiding this comment.
The iso_file: Option<File> field is initialized at the beginning of the build method and then immediately taken. This makes the field on the struct itself seem redundant, as it only holds a value for a brief moment and isn't used by other public methods. Consider creating the File handle directly inside build and passing it as an argument to the internal helper methods. This would simplify the IsoBuilder struct's state and make the data flow clearer.
| fn get_lba_for_path(&self, path: &str) -> io::Result<u32> { | ||
| let mut current_dir = &self.root; | ||
| let components: Vec<_> = Path::new(path).components().collect(); | ||
|
|
||
| for (i, component) in components.iter().enumerate() { | ||
| let component_name = component.as_os_str().to_str().ok_or_else(|| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "Invalid path component") | ||
| })?; | ||
| if i == components.len() - 1 { | ||
| // Last component, could be file or directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::File(file)) => return Ok(file.lba), | ||
| Some(IsoFsNode::Directory(dir)) => return Ok(dir.lba), | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } else { | ||
| // Intermediate component, must be a directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::Directory(dir)) => current_dir = dir, | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )) | ||
| } |
There was a problem hiding this comment.
The path traversal logic in get_lba_for_path and get_file_size_in_iso is nearly identical. This code duplication can be avoided by extracting the common logic into a single private helper function, such as get_node_by_path(&self, path: &str) -> io::Result<&IsoFsNode>. The two public functions could then call this helper and simply extract the lba or size from the returned node, improving maintainability.
| if let Some(path) = uefi_fat_img_path { | ||
| std::fs::remove_file(path)?; | ||
| } |
There was a problem hiding this comment.
The temporary FAT image is managed by temp_fat_file_holder, an instance of NamedTempFile. This type's RAII implementation ensures the file is automatically deleted when it goes out of scope. This manual call to std::fs::remove_file is therefore redundant and should be removed. It could also potentially mask other issues, as NamedTempFile's drop implementation ignores errors.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring by implementing a flexible IsoBuilder for creating ISO images, replacing the previous, more rigid implementation. This is a great improvement that adds support for multiple boot entries and provides a much cleaner API. I've identified one critical bug that would prevent compilation, a high-severity issue related to temporary file handling, and several medium-severity suggestions for improving robustness, maintainability, and cleaning up debugging code. Addressing these points will make this contribution even stronger.
| let total_sectors_u64 = final_pos.div_ceil(ISO_SECTOR_SIZE as u64); | ||
| self.total_sectors = u32::try_from(total_sectors_u64).map_err(|_| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "ISO image is too large") | ||
| })?; | ||
| update_total_sectors_in_pvd(iso_file, self.total_sectors)?; |
There was a problem hiding this comment.
The variable final_pos is used here but it's not defined in this scope, which will cause a compilation error. You need to get the file's current position after the padding operation to correctly calculate the total sectors. The indentation of the original line was also incorrect.
| let total_sectors_u64 = final_pos.div_ceil(ISO_SECTOR_SIZE as u64); | |
| self.total_sectors = u32::try_from(total_sectors_u64).map_err(|_| { | |
| io::Error::new(io::ErrorKind::InvalidInput, "ISO image is too large") | |
| })?; | |
| update_total_sectors_in_pvd(iso_file, self.total_sectors)?; | |
| let final_pos = iso_file.stream_position()?; | |
| let total_sectors_u64 = final_pos.div_ceil(ISO_SECTOR_SIZE as u64); | |
| self.total_sectors = u32::try_from(total_sectors_u64).map_err(|_| | |
| io::Error::new(io::ErrorKind::InvalidInput, "ISO image is too large") | |
| )?; | |
| update_total_sectors_in_pvd(iso_file, self.total_sectors)?; |
| if let Some(path) = uefi_fat_img_path { | ||
| std::fs::remove_file(path)?; | ||
| } | ||
| // Clean up dummy kernel file if created |
There was a problem hiding this comment.
The manual call to std::fs::remove_file on uefi_fat_img_path is redundant and potentially problematic. The temp_fat_file_holder (which is a NamedTempFile) will automatically delete the temporary file when it is dropped at the end of the function scope. Manually deleting it beforehand can lead to errors if the Drop implementation of NamedTempFile tries to delete an already-deleted file. It's safer to remove this manual deletion and rely on the tempfile crate's RAII capabilities.
// The temporary FAT image is held by temp_fat_file_holder and will be cleaned up automatically when it's dropped.| println!("create_fat_image: fat_img_path: {:?}", fat_img_path); | ||
| println!("create_fat_image: loader_path: {:?}", loader_path); | ||
| println!("create_fat_image: kernel_path: {:?}", kernel_path); |
There was a problem hiding this comment.
These println! statements appear to be for debugging. It's recommended to remove them or replace them with a proper logging framework (like log or tracing) before merging. This will keep the library's output clean and avoid polluting stdout for consumers of this crate. The same applies to other new println! statements in this function.
| fn get_lba_for_path(&self, path: &str) -> io::Result<u32> { | ||
| let mut current_dir = &self.root; | ||
| let components: Vec<_> = Path::new(path).components().collect(); | ||
|
|
||
| for (i, component) in components.iter().enumerate() { | ||
| let component_name = component.as_os_str().to_str().ok_or_else(|| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "Invalid path component") | ||
| })?; | ||
| if i == components.len() - 1 { | ||
| // Last component, could be file or directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::File(file)) => return Ok(file.lba), | ||
| Some(IsoFsNode::Directory(dir)) => return Ok(dir.lba), | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } else { | ||
| // Intermediate component, must be a directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::Directory(dir)) => current_dir = dir, | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("Path not found: {}", path), | ||
| )) | ||
| } | ||
|
|
||
| fn get_file_size_in_iso(&self, path: &str) -> io::Result<u64> { | ||
| let mut current_dir = &self.root; | ||
| let components: Vec<_> = Path::new(path).components().collect(); | ||
|
|
||
| for (i, component) in components.iter().enumerate() { | ||
| let component_name = component.as_os_str().to_str().ok_or_else(|| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "Invalid path component") | ||
| })?; | ||
| if i == components.len() - 1 { | ||
| // Last component, must be a file | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::File(file)) => return Ok(file.size), | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } else { | ||
| // Intermediate component, must be a directory | ||
| match current_dir.children.get(component_name) { | ||
| Some(IsoFsNode::Directory(dir)) => current_dir = dir, | ||
| _ => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("File not found: {}", path), | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
The functions get_lba_for_path and get_file_size_in_iso share a lot of similar logic for traversing the filesystem tree. This code duplication can be reduced by creating a helper function, for example get_node_for_path(&self, path: &str) -> io::Result<&IsoFsNode>, which would handle the path traversal and return the IsoFsNode. Then, get_lba_for_path and get_file_size_in_iso could use this helper and extract the specific information they need. This would improve maintainability.
| // Create a dummy kernel path in the same directory as the ISO output | ||
| // This ensures it lives long enough and is cleaned up with the temp_dir of the test. | ||
| let dummy_kernel_path = iso_path | ||
| .parent() | ||
| .ok_or_else(|| { | ||
| io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "ISO path has no parent directory", | ||
| ) | ||
| })? | ||
| .join("dummy_kernel_for_fat"); | ||
| std::fs::write(&dummy_kernel_path, b"")?; | ||
| dummy_kernel_file_for_fat_creation = Some(dummy_kernel_path.clone()); | ||
|
|
||
| fat::create_fat_image( | ||
| &fat_img_path, | ||
| &uefi_boot_info.boot_image, | ||
| &dummy_kernel_path, | ||
| )?; |
There was a problem hiding this comment.
The creation of a dummy_kernel_for_fat file seems like a workaround because fat::create_fat_image requires a kernel path. It would be cleaner to refactor fat::create_fat_image to make the kernel_path parameter an Option<&Path>. This would make the API more explicit and remove the need for creating and cleaning up dummy files when they are not needed (e.g., for a UEFI-only boot image).
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini summary |
Summary of ChangesThis pull request introduces a significant refactoring of the ISO image creation process by implementing a new, more robust Highlights
Changelog
Activity
|
1 participant
Pull Request
Change details
Build / Test Results