composefs/install: Copy /etc contents to state #1560
Conversation
|
I've started out by separating new composefs-backend related code to |
![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 refactors the initramfs crate into a library and introduces functionality to copy /etc contents for composefs installations. The changes are logical and follow the PR's description. I've identified a couple of areas for improvement in the new copy_etc_to_state function concerning resource management and error handling, and a minor code style issue.
| use camino::Utf8PathBuf; | ||
| use fn_error_context::context; | ||
|
|
||
| use bootc_initramfs_setup; |
There was a problem hiding this comment.
This use statement is redundant because crate names are already in scope in Rust 2018 and later editions. You can safely remove this line.
Johan-Liebert1
force-pushed
the
bind-mnt-etc
branch
2 times, most recently
from
99c2823 to
c5c3751
Compare
Johan-Liebert1
force-pushed
the
composefs-backend
branch
from
67c4e8d to
b45ef30
Compare
| if !output.status.success() { | ||
| anyhow::bail!( | ||
| "Copying /etc failed with status {}: {}", | ||
| output.status, | ||
| String::from_utf8_lossy(&output.stderr) | ||
| ); | ||
| } |
There was a problem hiding this comment.
We have a handy
bootc/crates/utils/src/command.rs
Line 38 in 7434b0f
There was a problem hiding this comment.
I see. I'll use that
| let uuid = uuid::Uuid::new_v4(); | ||
| let dir = format!("/var/tmp/{uuid}"); | ||
|
|
||
| create_dir_all(&dir).context("Creating temp directory")?; |
There was a problem hiding this comment.
Confused why we wouldn't just use tempdir::tempdir
|
|
||
| create_dir_all(&dir).context("Creating temp directory")?; | ||
|
|
||
| bootc_initramfs_setup::mount_at_wrapper(composefs_fd, CWD, &dir)?; |
There was a problem hiding this comment.
Ah yeah, we have mount helper code in lib/...and in composefs-boot, probably again need to factor out a helper crate
|
|
||
| bootc_initramfs_setup::mount_at_wrapper(composefs_fd, CWD, &dir)?; | ||
|
|
||
| let output = Command::new("cp") |
There was a problem hiding this comment.
I'm totally fine with this, but at some point I'd like to add a high level copy_recursive to e.g. cap-std-ext
There was a problem hiding this comment.
yes, that would be ideal
| ]) | ||
| .output()?; | ||
|
|
||
| // Unmount regardless of copy succeeding |
There was a problem hiding this comment.
Yeah I think what we want is a wrapper for tempdir that handles unmount-on-drop
There was a problem hiding this comment.
Composefs has a struct for exactly this, but again is only visible at crate level. We could make that public. https://github.com/containers/composefs-rs/blob/main/crates/composefs/src/mountcompat.rs#L129
The composefs-rs crate has a lot of helper functions which are private
Johan-Liebert1
force-pushed
the
bind-mnt-etc
branch
from
c5c3751 to
2c6d88c
Compare
For bind mounting /etc we copy the contents of the EROFS' /etc to the deployment's state directory Mounting the EORFS requires help from the initramfs crate, so we also turn it into a library crate. Signed-off-by: Johan-Liebert1 <[email protected]>
Johan-Liebert1
force-pushed
the
bind-mnt-etc
branch
from
2c6d88c to
1d8606c
Compare
2 participants
For bind mounting /etc we copy the contents of the EROFS' /etc to the deployment's state directory
Mounting the EORFS requires help from the initramfs crate, so we also turn it into a library crate.