Cleanup ASIF calculations#86
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
==========================================
+ Coverage 80.89% 80.92% +0.03%
==========================================
Files 26 26
Lines 2381 2411 +30
==========================================
+ Hits 1926 1951 +25
- Misses 455 460 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| read_length = min(length, self.asif.chunk_size) | ||
| if chunk == 0: | ||
| if (table := self.directory.table(offset // self.asif._size_per_table)) is None: | ||
| read_length = min(length, self.asif._size_per_table) |
There was a problem hiding this comment.
Read offsets are aligned to chunk_size, not to table_size.
If a read starts halfway into a sparse table, is it possible that we silently return zeros for the data of the next table?
Perhaps it is structurally cleaner to nest the iteration and have a outer loop over tables, and inner loop over chunks?
| chunk = table.entries[entry_index] & 0x7FFFFFFFFFFFFF | ||
|
|
||
| read_length = min(length, self.asif.chunk_size) | ||
| if chunk == 0: |
There was a problem hiding this comment.
(found with AI):
https://github.com/huven/asif-format
clams there is a data chunk status = 11, and suggests the following read behavior:
Data chunk status = 11: consult the corresponding sector >status in the bitmap chunk; return all zeros for uninitialized >sectors (00) and unmapped sectors (10), read physical chunk >for initialized sectors (01), and reject bitmap state 11.
| result.append(self.asif.fh.read(self.asif.block_size)) | ||
| elif block_status == 0b10: | ||
| # unmapped | ||
| result.append(b"\x00" * self.asif.block_size) |
There was a problem hiding this comment.
Is it possible to trigger this code-path by the test fixture?
Perhaps patch the fixture in-memory and turn a logical chunk into a partial chunk, and hand craft the associated bitmap?
| ) | ||
|
|
||
| # Read the bitmap | ||
| bitmap_chunk = table.entries[bitmap_entry_index] & 0x7FFFFFFFFFFFFF |
There was a problem hiding this comment.
The bitmap is possibly re-read in the case of consecutive partial chunks.
Perhaps the bitmap can be cached.
(can be deferred)
|
|
||
| read_length = min(length, self.asif.chunk_size) | ||
| if chunk == 0: | ||
| table_index, offset_in_table = divmod(offset, self.asif._size_per_table) |
There was a problem hiding this comment.
_read is mixing three levels of abstraction:
- Stream bookkeeping.
- Address translation
- Producing bytes
Perhaps push the layout math to a Table class.
Perhaps introduce a read_partial_chunk method.
(can be deferred)
2 participants
With the information from https://github.com/huven/asif-format and a fresher mind, I took another look at some of the calculations made in the ASIF implementation. I believe these new changes to be much more accurate.