Composefs finalize #1604
Composefs finalize #1604
Conversation
Using cap_std's symlink results in an error if the symlink target is absolute Signed-off-by: Pragyan Poudyal <[email protected]>
Add a TempMount struct which mounts a device/partition on a tempdir and automatically unmount on drop Signed-off-by: Pragyan Poudyal <[email protected]>
![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 finalize-staged service for composefs, which handles the three-way /etc merge and unstages boot entries at shutdown. The changes include refactoring boot logic to reduce duplication, adding a new TempMount utility for safer mount handling, and implementing the core finalization logic. My review has identified a couple of critical issues in the new finalization logic related to redundant mount operations and incorrect path handling for the /etc merge. Additionally, there are high-severity concerns about potential resource leaks in the new TempMount utility. Addressing these points will improve the correctness and robustness of the new functionality.
Johan-Liebert1
force-pushed
the
composefs-finalize
branch
from
54a8f81 to
9002d2d
Compare
The service is intended to perform the following task at shutdown if a staged deployment is present - Perform three way /etc merge for the staged deployment - Un-stage boot entries depending upon bootloader and boot entry type Some assumptions made - ESP will always be present - We won't have two bootloaders at the same time Signed-off-by: Pragyan Poudyal <[email protected]>
Allow mounting of any fds acquired using open_tree like syscalls Signed-off-by: Pragyan Poudyal <[email protected]>
Johan-Liebert1
force-pushed
the
composefs-finalize
branch
from
9002d2d to
e46daf7
Compare
|
I had as a comment, which is plain wrong as we can't anyway |
| let dir_path = PathBuf::from(format!( | ||
| "/sysroot/boot/loader/{ROLLBACK_BOOT_LOADER_ENTRIES}", | ||
| )); | ||
| let dir_path = PathBuf::from(format!("/sysroot/boot/loader/{STAGED_BOOT_LOADER_ENTRIES}",)); |
There was a problem hiding this comment.
Not new but we should centralize boot/loader path handling at some point
| } | ||
|
|
||
| pub fn get_sysroot_parent_dev() -> Result<String> { | ||
| let sysroot = Utf8PathBuf::from("/sysroot"); |
There was a problem hiding this comment.
This relates to #1190 (comment) in that what I think would help us overall is to pass around a Storage instance that has a canonical opened fd for the sysroot instead of having a lot of places that load it ambiently.
The biggest reason to do this is that it will work the same way at install time, where we're actually operating on a different mount point and not /sysroot.
2 participants
Introduce finalize-staged service
The service is intended to perform the following task at shutdown if a
staged deployment is present
Some assumptions made