Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to the integration tests: #7

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

asomers
Copy link
Collaborator

@asomers asomers commented Nov 7, 2022

  • Use the tempfile crate instead of hard-coding temporary file names
  • Must use _exit(0) at the end of child processes, so we don't run the Rust destructors in both children and parents.
  • Minimize use of unsafe
  • Always run cap_enter in a child process.

* Use the tempfile crate instead of hard-coding temporary file names
* Must use _exit(0) at the end of child processes, so we don't run the
  Rust destructors in both children and parents.
* Minimize use of `unsafe`
* Always run `cap_enter` in a child process.
Copy link
Owner

@dlrobertson dlrobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small nit. Happy to merge and split it out into a separate issue.

capsicum::enter().unwrap();
let path = CString::new("lib.rs").unwrap();
let _ = dir.open_file(path, 0, None).unwrap();
unsafe { libc::_exit(0) };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Would you be up for adding a failure case? We could use tempdir to create a directory which we wouldn't have access to, or do something like a lookup in "./tests". We probably should avoid using "./src" and "./tests" which I think wouldn't work in some cases if the tests binary was run manually outside of the root crate directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was noticing the same thing about "./src".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are other problems too. We aren't monitoring the child process's status correctly. I suggest we merge this PR as-is, and then I'll do another that uses Nix as a dependency in order to use waitpid correctly, which will help fix these problems.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 works for me.

@dlrobertson dlrobertson merged commit 5ae555a into dlrobertson:master Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants