jailer: increase code coverage #1940
Conversation
lauralt
force-pushed
the
jailer_tests
branch
2 times, most recently
from
69bf9bf to
5bd84d8
Compare
lauralt
added
the
Status: Awaiting review
src/jailer/src/env.rs
Outdated
| @@ -210,14 +210,11 @@ impl Env { | |||
| .map_err(|e| Error::ChangeFileOwner(path_buf, e)) | |||
| } | |||
|
|
|||
| pub fn run(mut self) -> Result<()> { | |||
| fn prepare_for_chroot(&mut self) -> Result<()> { | |||
There was a problem hiding this comment.
The name is a bit misleading, this method doesn't do all the necessary preparing for chroot (cgroups, open /dev/null). Could the 2 things that this method deals with be split in different methods and tested separately?
There was a problem hiding this comment.
I agree it's not the best name, didn't have a better idea at that moment.
I split the function in two, now the testing part should be clearer too.
src/jailer/src/env.rs
Outdated
| self.prepare_for_chroot()?; | ||
|
|
||
| // Safe to unwrap because we check in the call above that `file_name()` is successful. | ||
| // The borrow checker doesn't let us to nicely use an `&OsStr` as function parameter |
There was a problem hiding this comment.
OsStr is a borrowed reference to an OsStringfrom inside self. That is why the self is seen as mutably borrowed by the checker. What you can do is make prepare_for_chroot return an OsString. Something around:
fn prepare_for_chroot(&mut self) -> Result<(OsString)> {
...
Ok(exec_file_name.to_os_string())
}
pub fn run {
let exec_file_name = self.prepare_for_chroot()?;
let chroot_exec_file = PathBuf::from("/").join(&exec_file_name);
let cgroup = Cgroup::new(&self.id, self.numa_node, &exec_file_name)?;
}
WDYT?
There was a problem hiding this comment.
Nice! Indeed I tried something similar, but with OsStr instead of OsString🤦♀️.
I'll update, thanks!
src/jailer/src/env.rs
Outdated
| let some_arg_vals = ArgVals { | ||
| node: "1", | ||
| id: "bd65600d-8669-4903-8a14-af88203add38", | ||
| exec_file: "file1", |
There was a problem hiding this comment.
Cant we use TempFilefor zzns and file1 and TempDir forchroot_base`?
Split `run()` in three in order to be able to unit-test some part of it. Add unit tests for some uncovered sections from jailer's `main.rs`. Fix a bit the cleanup logic from `test_mknod_and_own_dev()`. Signed-off-by: Laura Loghin <lauralg@amazon.com>
Reason for This PR
A step forward for #363.
Description of Changes
With the recently merged #1862, realized the coverage for jailer can be quite easily increased.
Split
run()in two in order to be able to unit-test some part of it.Added a few unit tests and made a small cleanup in jailer env tests.
rust-vmm.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).unsafecode is properly documented.firecracker/swagger.yaml.CHANGELOG.md.