Repository navigation
Install multiple programs from a bundle - #17
Conversation
…rland/*/ Signed-off-by: Glen <glen.meehan@ptoronmail.com>
Signed-off-by: Glen <glen.meehan@ptoronmail.com>
Signed-off-by: Glen <glen.meehan@ptoronmail.com>
|
Thanks. The camelCase function names follow the existing code and Zig's standard style, so I've corrected the naming rule in CONTRIBUTING.md instead of renaming. |
Signed-off-by: Glen <glen.meehan@protonmail.com>
Signed-off-by: Glen <glen.meehan@protonmail.com>
… the entry is removed Signed-off-by: Glen <glen.meehan@protonmail.com>
There was a problem hiding this comment.
Devin Review found 3 new potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| fs.superblock.sm_start_block, | ||
| fs.superblock.sm_block_count, | ||
| ); | ||
| try fs.insertEntry(allocator, root, entry); |
There was a problem hiding this comment.
🔴 Duplicate program entries retain freed blocks
For each new program, installOne inserts the same directory entry twice. deleteFile frees its blocks but leaves the second entry pointing at reusable storage.
Learn more
Each call to insertEntry writes an entry into the next empty root-directory slot; it does not check for a duplicate name. Here the same entry is written twice for each new program. deleteFile removes only the first matching entry before freeing its metadata and payload extents. The remaining entry still resolves to blocks that the space manager can assign to a different file.
Example: A fresh bundle installs hello in two root slots. After del hello, ls still shows hello; creating another file can reuse hello's freed blocks, so the remaining name refers to that file's storage.
Recommended fix: Persist the allocations once, insert the directory entry once, and ensure rollback cannot free extents after that entry becomes visible.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @memset(std.mem.asBytes(e), 0); | ||
| try fs.device.writeBlocks(fs.device.ctx, lba, dir_buf); |
There was a problem hiding this comment.
🔴 Failed extent release loses file access
If SpaceManager.free runs out of allocator memory, deleteFile has already erased the directory entry. The file becomes inaccessible and its blocks remain allocated.
Learn more
Deletion now writes an empty directory slot before returning the file's extents to the free list. SpaceManager.free appends to an allocator-backed list and can fail. When that happens, deletion returns an error but the file is already unlinked; the caller cannot look it up or retry deletion.
Example: Deleting notes needs the free list to grow when the supplied allocator has no space. The directory slot for notes is erased, then free returns OutOfMemory; notes is gone but its blocks remain in use.
Recommended fix: Reserve enough capacity in the space manager's free list for every extent and the metadata block before changing the directory. Then perform the non-allocating extent releases after the entry is erased.
Was this helpful? React with 👍 or 👎 to provide feedback.
| errdefer if (!linked) { | ||
| fs.space_manager.free(allocator, meta_extent) catch {}; | ||
| }; |
There was a problem hiding this comment.
🟡 Rollback hides failed extent releases
When rollback's SpaceManager.free fails, installOne silently discards the error in both handlers. The extent remains unavailable without a diagnostic.
Learn more
The repository's error-handling rule forbids bare catch {} in new code without a specific documented reason. Both rollback handlers use it around SpaceManager.free, which can fail when its free list cannot grow. In that case the original installation error is returned, but the additional failed cleanup is invisible and its extent is not reclaimed.
Example: The installer cannot write the payload, then freeing its metadata extent needs a larger free list and fails with OutOfMemory. The caller sees only the write error and receives no indication that an extent was lost.
Recommended fix: Record or log cleanup failures in both errdefer handlers, or pre-reserve free-list capacity before allocating extents so cleanup cannot fail.
Was this helpful? React with 👍 or 👎 to provide feedback.
Signed-off-by: Glen <glen.meehan@protonmail.com>
Signed-off-by: Glen <glen.meehan@protonmail.com>
Packs every
userland/<name>/program into one bundle; the installer creates a CodaFS file per program. Adds a second demo program (hello) and a smoke-test check.