Internals bwrap#5438
Conversation
Dead code since 8dfed46
This is just a workaround.
This is a thin helper to aid in debugging coreos#5436
|
Skipping CI for Draft Pull Request. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| #!/bin/bash | ||
| set -euo pipefail | ||
|
|
||
| dn=$(cd $(dirname $0) && pwd) |
Check warning
Code scanning / shellcheck
Quote this to prevent word splitting. Warning test
| echo someerr 1>&2 | ||
| echo world | ||
| EOF | ||
| rpm-ostree internals bwrap-script / /bin/bash $(pwd)/script >out.txt |
Check warning
Code scanning / shellcheck
Quote this to prevent word splitting. Warning test
![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 internals command from C to Rust and adds a new bwrap-script subcommand. The changes are well-structured and include a new test case for the added functionality. The supporting changes in the build system and module wiring are correct. I've identified a couple of areas for improvement in the new Rust code related to maintainability and robustness, specifically regarding the use of magic numbers and a hardcoded path.
| let root = &Dir::open_ambient_dir(&self.root, authority)?; | ||
| let mut bwrap = | ||
| bwrap::Bubblewrap::new_with_mutability(root, BubblewrapMutability::MutateFreely)?; | ||
| let td = Dir::open_ambient_dir("/var/tmp", authority)?; |
There was a problem hiding this comment.
Hardcoding /var/tmp might reduce portability. This will fail if the tool is run in an environment where /var/tmp does not exist or is not writable. Using std::env::temp_dir() is more robust as it returns a platform-specific temporary directory.
| let td = Dir::open_ambient_dir("/var/tmp", authority)?; | |
| let td = Dir::open_ambient_dir(std::env::temp_dir(), authority)?; |
| bwrap.take_fd(mfd.into_raw_fd(), 5); | ||
| bwrap.append_child_arg("/proc/self/fd/5"); |
There was a problem hiding this comment.
The file descriptor 5 is used as a magic number here and on the next line. It's better to define it as a constant to improve readability and maintainability.
| bwrap.take_fd(mfd.into_raw_fd(), 5); | |
| bwrap.append_child_arg("/proc/self/fd/5"); | |
| const SCRIPT_FD: i32 = 5; | |
| bwrap.take_fd(mfd.into_raw_fd(), SCRIPT_FD); | |
| bwrap.append_child_arg(&format!("/proc/self/fd/{}", SCRIPT_FD)); |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Nice. In https://github.com/coreos/fedora-coreos-config/blob/5603f9a31091b4ddef4eb3368ada64f88f2b05c4/build-rootfs#L138, one idea I had was actually exposing bwrap as |
|
4 participants
No description provided.