composefs/install: Copy /etc contents to state#1560
composefs/install: Copy /etc contents to state#1560cgwalters merged 1 commit intobootc-dev:composefs-backendfrom
Conversation
|
I've started out by separating new composefs-backend related code to |
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.
99c2823 to
c5c3751
Compare
67c4e8d to
b45ef30
Compare
cgwalters
left a comment
There was a problem hiding this comment.
Looks OK, can you just fix to use tempdir, the other bits can be followups
| 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
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 <pragyanpoudyal41999@gmail.com>
2c6d88c to
1d8606c
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.