Deuxfleurs/garage
41
56
Fork 73
Code Issues 99 Pull requests 15 Releases 30 Wiki Activity

Tests: Add garage integration tests #215

Closed
KokaKiwi wants to merge 8 commits from KokaKiwi/garage:integration-tests into main
pull from: KokaKiwi/garage:integration-tests
merge into: Deuxfleurs:main
Conversation 1 Commits 8 Files changed 15 +8 -5
1 changed files with 8 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 540c5479a7 - Show all commits

13
src/garage/tests/common/garage.rs
View file

@ -1,4 +1,3 @@
use std::env::var_os;
use std::mem::MaybeUninit; use std::mem::MaybeUninit;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
use std::process; use std::process;
@ -186,6 +185,8 @@ static INSTANCE_INIT: Once = Once::new();
#[static_init::destructor] #[static_init::destructor]
extern "C" fn terminate_instance() { extern "C" fn terminate_instance() {
if INSTANCE_INIT.is_completed() { if INSTANCE_INIT.is_completed() {
KokaKiwi marked this conversation as resolved Outdated
quentin commented 2022-02-04 14:03:14 +00:00
Outdated
Review
Copy link

Why do you need extern "C" here?

Why do you need `extern "C"` here?
KokaKiwi commented 2022-02-04 14:30:08 +00:00
Outdated
Review
Copy link

It's required by the static_init::destructor attribute, as it register the function to be called by libc global ctor/dtor stuff

It's required by the `static_init::destructor` attribute, as it register the function to be called by libc global ctor/dtor stuff
// This block is sound as it depends on `INSTANCE_INIT` being completed, meaning `INSTANCE`
// is actually initialized.
unsafe { unsafe {
INSTANCE.assume_init_mut().terminate(); INSTANCE.assume_init_mut().terminate();
} }
@ -200,15 +201,17 @@ pub fn instance() -> &'static Instance {
INSTANCE.write(instance); INSTANCE.write(instance);
}); });
KokaKiwi marked this conversation as resolved Outdated
quentin commented 2022-02-04 14:04:03 +00:00
Outdated
Review
Copy link

Do you have any documentation explaining why unsafe is required in this function and the previous one?

Do you have any documentation explaining why `unsafe` is required in this function and the previous one?
KokaKiwi commented 2022-02-04 15:15:45 +00:00
Outdated
Review
Copy link

The unsafe blocks here are required because:

  1. INSTANCE is a mutable static, accessing/modifying is unsafe by definition: https://doc.rust-lang.org/nomicon/what-unsafe-does.html
  2. INSTANCE is of MaybeUninit<T> type, accessing its value is unsafe it could be uninitialized at the time it's accessed: std::mem::MaybeUninit::assume_init_ref (see Safety section)

I'll add some changes here to document why these unsafe blocks are actually sound ("safe")

The unsafe blocks here are required because: 1. `INSTANCE` is a mutable static, accessing/modifying is unsafe by definition: https://doc.rust-lang.org/nomicon/what-unsafe-does.html 2. `INSTANCE` is of `MaybeUninit<T>` type, accessing its value is unsafe it could be uninitialized at the time it's accessed: [`std::mem::MaybeUninit::assume_init_ref`](https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.assume_init_ref) (see `Safety` section) I'll add some changes here to document why these unsafe blocks are actually sound ("safe")
// This block is sound as it depends on `INSTANCE_INIT` being completed by calling `call_once` (blocking),
// meaning `INSTANCE` is actually initialized.
unsafe { INSTANCE.assume_init_ref() } unsafe { INSTANCE.assume_init_ref() }
} }
pub fn command(config_path: &Path) -> process::Command { pub fn command(config_path: &Path) -> process::Command {
use std::env;
let mut command = process::Command::new( let mut command = process::Command::new(
var_os("GARAGE_TEST_INTEGRATION_EXE") env::var("GARAGE_TEST_INTEGRATION_EXE")
.as_ref() .unwrap_or_else(|_| env!("CARGO_BIN_EXE_garage").to_owned()),
.and_then(|e| e.to_str())
.unwrap_or(env!("CARGO_BIN_EXE_garage")),
); );
command.arg("-c").arg(config_path); command.arg("-c").arg(config_path);