store: Add support for composefs-rs #1471
Conversation
Signed-off-by: Colin Walters <[email protected]>
This is prep for wider usage of it in this project. Like the containers-image: storage, it is only initialized on demand right now. (An obvious next step is to redo things so the ostree storage is also on-demand) - This is hardcoded to SHA512 right now...but we clearly want a way to configure that or maybe we just really default to 512? - We explicitly bridge between the ostree fsverity enablement to the composefs verity enablement - Right now the usage is just a stub but I plan to expose more here Signed-off-by: Colin Walters <[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 initial support for composefs by adding it as a dependency and implementing logic to initialize a composefs repository within the bootc storage. It also includes a new internal test command to verify the integration.
My review focuses on improving thread safety in the lazy initialization of the composefs repository and promoting better error handling practices in the new test command. These changes will enhance the robustness and maintainability of the new functionality.
cgwalters
changed the title
|
|
||
| let mut db = DirBuilder::new(); | ||
| db.mode(COMPOSEFS_MODE.as_raw_mode()); | ||
| self.physical_root.ensure_dir_with(COMPOSEFS, &db)?; |
There was a problem hiding this comment.
Do we not care about the returned bool for anything here? We just rely on the fact that if there's an error, we're unwraping it and we will return the error. The returned bool isn't really useful for us in this context I guess?
There was a problem hiding this comment.
Yes, it just says whether or not the directory existed already which is information we don't need.
There was a problem hiding this comment.
Yeah, that makes sense. It would never make it to the next line if there was an error returned from ensure_dir_with(). There's no logical need to store and reference the returned value.
| let r = Arc::clone(self.composefs.get_or_init(|| composefs)); | ||
| Ok(r) |
There was a problem hiding this comment.
I noted your response to Gemini below, you mentioned you only expect this to be called once and don't necessarily require thread safety here. I'm just curious about your use of Arc over rc in that case? Since Arc is slightly more computationally expensive, I'm curious to understand your thought process.
There's every possibility that my lack of Rust experience is causing me to overlook something. In which case, consider this a question for my own educational purposes rather than a functional critique of your PR per se. :)
There was a problem hiding this comment.
The underlying API wants Arc https://github.com/containers/composefs-rs/blob/126751dee8a71aa54c9666e69645d2fd1eccb176/crates/composefs/src/repository.rs#L112
Ultimately we definitely don't care about the refcounting overhead here, and in general Arc is needed for things like tokio with work stealing.
There was a problem hiding this comment.
In which case, consider this a question for my own educational purposes rather than a functional critique of your PR per se. :)
Yes that's fine! Thanks for looking at the PR!
There was a problem hiding this comment.
Ahh, that makes sense then. Thanks for the clarification.
3 participants
No description provided.