composefs/usr: Fix /usr permissions on overlay mount #1836
Conversation
![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 correctly addresses the issue of incorrect directory permissions for /usr transient mounts by introducing an optional mode argument to ensure_dir and propagating it up to overlay_transient. The fix in composefs_usr_overlay to use 0o755 permissions is appropriate.
The changes are well-implemented. However, the signature change to the public function bootc_initramfs_setup::overlay_transient is a breaking API change. While all internal call sites have been updated, this could impact external consumers of the crate. It would be good to acknowledge this in the PR description or consider if a non-breaking alternative is feasible (e.g., adding a new function with the new signature).
I've also added a few suggestions to replace magic numbers with named constants to improve code readability and maintainability.
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Looks good as is, but two followups possible
| } | ||
|
|
||
| overlay_transient(usr)?; | ||
| overlay_transient(usr, Some(0o755))?; |
There was a problem hiding this comment.
Let's not hardcode the mode here - while it's unlikely to change, if someone wants to make it (or more likely all of /usr including subdirs) group writable (775), or alternatively not user writable (555) that'd be up to the distro/OS.
There was a problem hiding this comment.
if someone wants to make it (or more likely all of /usr including subdirs) group writable (775), or alternatively not user writable (555)
How would we want to do this? Would we take mode as an optional cli opt?
There was a problem hiding this comment.
Ah yeah I was unclear. I'm just saying we should inherit the mode of the underlying directory - seems that simple to me
The upper,work directories being created for `/usr` transient mount always had the mode `0o700` hence only being accessible to root Update `bootc_initramfs_setup::ensure_dir` to accept an optional `mode` argument Fixes: bootc-dev#1833 Signed-off-by: Pragyan Poudyal <[email protected]>
Johan-Liebert1
force-pushed
the
usr-overlay-fix
branch
from
b0623e4 to
ca06673
Compare
2 participants
The upper,work directories being created for
/usrtransient mount always had the mode0o700hence only being accessible to rootUpdate
bootc_initramfs_setup::ensure_dirto accept an optionalmodeargumentFixes: #1833