Draft: Implement runtime guessing of sector size #137
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to make sector_size configurable per partition instead of using a global constant, enabling support for disks with non-standard sector sizes.
- Removed the global
sector_sizeconstant fromfs/constants.py - Added
sector_sizeas an instance attribute to thePartitionclass with a default value of 512 - Updated all functions to accept
sector_sizeas a parameter or read it from partition instances
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| recuperabit/fs/constants.py | Removed the global sector_size constant |
| recuperabit/fs/core_types.py | Added sector_size attribute to Partition class and removed duplicate get_image method |
| recuperabit/fs/ntfs.py | Updated all sector size references to use partition.sector_size and added sector_size parameter to parsing functions |
| recuperabit/utils.py | Added sector_size parameter to tree rendering functions and updated import statements |
| main.py | Updated tree_folder calls to pass sector_size from partition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| def tree_folder(directory: 'File', padding: int = 0) -> str: | ||
| def tree_folder(directory: 'File', padding: int = 0, sector_size: int | None = None) -> str: |
There was a problem hiding this comment.
The type annotation int | None uses Python 3.10+ union syntax, but the codebase uses typing.Optional elsewhere for consistency. Consider changing to Optional[int] for backward compatibility and consistency with the rest of the codebase.
| def tree_folder(directory: 'File', padding: int = 0, sector_size: int | None = None) -> str: | |
| def tree_folder(directory: 'File', padding: int = 0, sector_size: Optional[int] = None) -> str: |
| def tree_folder(directory: 'File', padding: int = 0) -> str: | ||
| def tree_folder(directory: 'File', padding: int = 0, sector_size: int | None = None) -> str: | ||
| """Return a tree-like textual representation of a directory.""" | ||
| assert sector_size is not None, "sector_size must be provided" |
There was a problem hiding this comment.
Using an assertion for input validation in production code is not recommended. If sector_size is required, either remove the None default or raise a ValueError instead of using assert. Assertions can be disabled with Python's -O flag, making this check unreliable.
| assert sector_size is not None, "sector_size must be provided" | |
| if sector_size is None: | |
| raise ValueError("sector_size must be provided") |
| for position in self.found_file: | ||
| dump = sectors(img, position, FILE_size) | ||
| parsed = parse_file_record(dump) | ||
| parsed = parse_file_record(dump, 512) # Default sector size during discovery |
There was a problem hiding this comment.
The comment states 'Default sector size during discovery', but this hardcoded value could be problematic if the actual sector size differs. Consider documenting why it's safe to use 512 here during the discovery phase, or if the sector size should be determined and used instead.
| # Parse INDX records | ||
| logging.info('Parsing INDX records') | ||
| for position in self.found_indx: | ||
| dump = sectors(img, position, INDX_size) |
There was a problem hiding this comment.
Similar to the previous issue, this hardcoded 512 sector size during discovery should be better documented to explain why it's safe to assume this value before the actual sector size is determined from the boot sector.
| dump = sectors(img, position, INDX_size) | |
| dump = sectors(img, position, INDX_size) | |
| # NTFS typically uses a sector size of 512 bytes. During the initial discovery phase, | |
| # we use 512 as the default sector size for parsing INDX records. This is safe because | |
| # the NTFS boot sector, which contains the actual sector size, may not be available yet. | |
| # After discovery, the correct sector size will be determined from the boot sector and | |
| # used for further parsing. See NTFS documentation for details: | |
| # https://learn.microsoft.com/en-us/windows/win32/fileio/ntfs-technical-reference |
Fixes #118, #99