[WIP] Add test for rootfs propagation with mock mount

[tiqwab]

This is a follow-up pull request to add unit tests for functions implemented in #309.

This pull request calls mount system call through Syscall trait to mock it in unit tests. Although we can use fork and then execute real mount system call in child process, I feel checking mount arguments is simple and enough in this case.

@utam0k @yihuaf
What do you think about the idea of using mock for these tests?

[yihuaf]

Haven't looked the entire PR yet. I think to test the readonly_path and mask_path individually, we should fork and use namespaces. mock only shows the command is being called, but testing the actual assumption we make.

Edit: My knee jerk reaction is to test these functions with fork, new mount namespace, and the actual mount calls. How hard is it to write the actual test with fork? You may have to use serial test in these cases.

[tiqwab]

I actually wrote a draft for the test of rootfs::make_parent_mount_private function with the actual mount system call in tiqwab@4e49290. Is it similar to what you think?

I just focused on writing unit tests for rootfs::make_parent_mount_private and rootfs::adjust_root_mount_propagation and in that case I thought checking mount args is simpler and enough. However I understand the benefit of using the actual mount call and we might had better to use it considering other test cases in rootfs.

[tiqwab]

and I realized that there is a similar work in #340, so we might had better to close this and leave it for @tommady.

[tommady]

and I realized that there is a similar work in #340, so we might had better to close this and leave it for @tommady.

ya thank you and please allow me to copy some of your masterwork there 🙇🏻

[yihuaf]

I actually wrote a draft for the test of rootfs::make_parent_mount_private function with the actual mount system call in tiqwab@4e49290. Is it similar to what you think?

I just focused on writing unit tests for rootfs::make_parent_mount_private and rootfs::adjust_root_mount_propagation and in that case I thought checking mount args is simpler and enough. However I understand the benefit of using the actual mount call and we might had better to use it considering other test cases in rootfs.

Yes, this is what I had in mind as well. A few minor points:

1. Let's use fork instead of clone, or we should implement clone properly. Rust memory management has some weird issue with regarding to stack vs. heap allocation for stack. We used to use clone but discovered issue in rust. So fork is much safer since you trust the kernel code to copy all the memory correctly. If clone prove to be better suited than fork, I would bring back the old implementation for a clone wrapper. See Refactor clone(2) child stack creation. #167 for reference.

2. We can now use ipc-channel to pass richer results from child process back to parent process. See Implemented util function to test in child process #345 for reference.

[yihuaf]

and I realized that there is a similar work in #340, so we might had better to close this and leave it for @tommady.

@tommady Are you able to incorporate this change? Since the title of #340 is vague, can I ask what is the scope of the PR? Let us know what you plan to keep from this PR for #340. In general, please keep the PR as small as reasonably possible. This will be much easier for review and coordinate.

[tommady]

@tommady Are you able to incorporate this change? Since the title of #340 is vague, can I ask what is the scope of the PR? Let us know what you plan to keep from this PR for #340. In general, please keep the PR as small as reasonably possible. This will be much easier for review and coordinate.

hi @yihuaf the goal of PR #340 is trying to increase the testing coverage of rootfs.rs file only.

so the plain is that

1. won't modify the existing working flow or logic
2. only try to mock the system calls
3. try to increase the coverage as much as possible

is that acceptable to you? thanks

[yihuaf]

Looks like @tommady already incorporated this PR into other PRs. Closing this for now. Please reopen if I made a mistake.