From 35803f4a30cdbaad30d7e9ffbd2ebf322d51aff6 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 15 Jun 2024 14:51:26 +0200 Subject: [PATCH] refactor: Remove check_one_call (cherry picked from commit 065f880e52c6d6cb44e4b857272176ebe2464eea) --- rust/nix-expr/src/eval_state.rs | 51 +++++++++++++++------------------ rust/nix-expr/src/value.rs | 12 ++++---- rust/nix-store/src/store.rs | 26 +++++++---------- rust/nix-util/src/context.rs | 9 ------ rust/nix-util/src/settings.rs | 20 ++++++------- 5 files changed, 48 insertions(+), 70 deletions(-) diff --git a/rust/nix-expr/src/eval_state.rs b/rust/nix-expr/src/eval_state.rs index b9656c2..a45c75f 100644 --- a/rust/nix-expr/src/eval_state.rs +++ b/rust/nix-expr/src/eval_state.rs @@ -16,10 +16,9 @@ lazy_static! { static ref INIT: Result<()> = { unsafe { raw::GC_allow_register_threads(); + check_call!(raw::libexpr_init[&mut Context::new()])?; + Ok(()) } - Context::new().check_one_call(|ctx_ptr| unsafe { - raw::libexpr_init(ctx_ptr); - }) }; } pub fn init() -> Result<()> { @@ -67,9 +66,9 @@ impl EvalState { init()?; - let eval_state = context.check_one_call(|ctx_ptr| unsafe { - raw::state_create(ctx_ptr, lookup_path.as_mut_ptr(), store.raw_ptr()) - })?; + let eval_state = unsafe { + check_call!(raw::state_create[&mut context, lookup_path.as_mut_ptr(), store.raw_ptr()]) + }?; Ok(EvalState { eval_state: NonNull::new(eval_state).unwrap_or_else(|| { panic!("nix_state_create returned a null pointer without an error") @@ -128,9 +127,12 @@ impl EvalState { Ok(()) } pub fn value_type_unforced(&mut self, value: &Value) -> Option { - let r = self - .context - .check_one_call(|ctx_ptr| unsafe { raw::get_type(ctx_ptr, value.raw_ptr()) }); + let r = unsafe { + check_call!(raw::get_type[ + &mut self.context, + value.raw_ptr() + ]) + }; // .unwrap(): no reason for this to fail, as it does not evaluate ValueType::from_raw(r.unwrap()) } @@ -153,8 +155,7 @@ impl EvalState { if t != ValueType::Int { bail!("expected an int, but got a {:?}", t); } - self.context - .check_one_call(|ctx_ptr| unsafe { raw::get_int(ctx_ptr, v.raw_ptr()) }) + unsafe { check_call!(raw::get_int[&mut self.context, v.raw_ptr()]) } } /// Evaluate, and require that the value is an attrset. /// Returns a list of the keys in the attrset. @@ -163,10 +164,8 @@ impl EvalState { if t != ValueType::AttrSet { bail!("expected an attrset, but got a {:?}", t); } - let n = self.context.check_one_call(|ctx_ptr| unsafe { - raw::get_attrs_size(ctx_ptr, v.raw_ptr()) as usize - })?; - let mut attrs = Vec::with_capacity(n); + let n = unsafe { check_call!(raw::get_attrs_size[&mut self.context, v.raw_ptr()]) }?; + let mut attrs = Vec::with_capacity(n as usize); for i in 0..n { let cstr_ptr: *const i8 = unsafe { check_call!(raw::get_attr_name_byidx[ @@ -180,7 +179,7 @@ impl EvalState { let s = cstr .to_str() .map_err(|e| anyhow::format_err!("Nix attrset key is not valid UTF-8: {}", e))?; - attrs.insert(i, s.to_owned()); + attrs.insert(i as usize, s.to_owned()); } Ok(attrs) } @@ -239,8 +238,7 @@ impl EvalState { let s = CString::new(s).with_context(|| "new_value_str: contains null byte")?; let v = unsafe { let value = self.new_value_uninitialized()?; - self.context - .check_one_call(|ctx_ptr| raw::init_string(ctx_ptr, value.raw_ptr(), s.as_ptr()))?; + check_call!(raw::init_string[&mut self.context, value.raw_ptr(), s.as_ptr()])?; value }; Ok(v) @@ -249,8 +247,7 @@ impl EvalState { pub fn new_value_int(&mut self, i: Int) -> Result { let v = unsafe { let value = self.new_value_uninitialized()?; - self.context - .check_one_call(|ctx_ptr| raw::init_int(ctx_ptr, value.raw_ptr(), i))?; + check_call!(raw::init_int[&mut self.context, value.raw_ptr(), i])?; value }; Ok(v) @@ -260,14 +257,12 @@ impl EvalState { fn get_string(&mut self, value: &Value) -> Result { let mut r = result_string_init!(); unsafe { - self.context.check_one_call(|ctx_ptr| { - raw::get_string( - ctx_ptr, - value.raw_ptr(), - Some(callback_get_result_string), - callback_get_result_string_data(&mut r), - ) - })?; + check_call!(raw::get_string[ + &mut self.context, + value.raw_ptr(), + Some(callback_get_result_string), + callback_get_result_string_data(&mut r) + ])?; }; r } diff --git a/rust/nix-expr/src/value.rs b/rust/nix-expr/src/value.rs index b25e449..e7ff7c7 100644 --- a/rust/nix-expr/src/value.rs +++ b/rust/nix-expr/src/value.rs @@ -1,5 +1,5 @@ use nix_c_raw as raw; -use nix_util::context::Context; +use nix_util::{check_call, context::Context}; use std::ptr::{null_mut, NonNull}; // TODO: test: cloning a thunk does not duplicate the evaluation. @@ -73,10 +73,12 @@ impl Drop for Value { } impl Clone for Value { fn clone(&self) -> Self { - let mut context = Context::new(); - context - .check_one_call(|ctx_ptr| unsafe { raw::gc_incref(ctx_ptr, self.inner.as_ptr()) }) - .unwrap(); + // TODO: Is it worth allocating a new Context here? Ideally cloning is cheap. + // this is very unlikely to error, and it is not recoverable + // Maybe try without, and try again with context to report details? + unsafe { + check_call!(raw::gc_incref[&mut Context::new(), self.inner.as_ptr()]).unwrap(); + } // can't return an error here, but we don't want to ignore the error either as it means we could use-after-free Value { inner: self.inner } } diff --git a/rust/nix-store/src/store.rs b/rust/nix-store/src/store.rs index 1e692c0..d827153 100644 --- a/rust/nix-store/src/store.rs +++ b/rust/nix-store/src/store.rs @@ -2,18 +2,17 @@ use anyhow::{bail, Result}; use lazy_static::lazy_static; use nix_c_raw as raw; use nix_util::context::Context; -use nix_util::result_string_init; use nix_util::string_return::{callback_get_result_string, callback_get_result_string_data}; +use nix_util::{check_call, result_string_init}; use std::ffi::{c_char, CString}; use std::ptr::null_mut; use std::ptr::NonNull; /* TODO make Nix itself thread safe */ lazy_static! { - static ref INIT: Result<()> = { - Context::new().check_one_call(|ctx_ptr| unsafe { - raw::libstore_init(ctx_ptr); - }) + static ref INIT: Result<()> = unsafe { + check_call!(raw::libstore_init[&mut Context::new()])?; + Ok(()) }; } @@ -74,9 +73,9 @@ impl Store { .chain(std::iter::once(null_mut())) // signal the end of the array .collect(); - let store = context.check_one_call(|ctx_ptr| unsafe { - raw::store_open(ctx_ptr, uri_ptr.as_ptr(), params.as_mut_ptr()) - })?; + let store = unsafe { + check_call!(raw::store_open[&mut context, uri_ptr.as_ptr(), params.as_mut_ptr()]) + }?; if store.is_null() { panic!("nix_c_store_open returned a null pointer without an error"); } @@ -95,14 +94,9 @@ impl Store { pub fn get_uri(&mut self) -> Result { let mut r = result_string_init!(); - self.context.check_one_call(|ctx_ptr| unsafe { - raw::store_get_uri( - ctx_ptr, - self.inner.ptr(), - Some(callback_get_result_string), - callback_get_result_string_data(&mut r), - ) - })?; + unsafe { + check_call!(raw::store_get_uri[&mut self.context, self.inner.ptr(), Some(callback_get_result_string), callback_get_result_string_data(&mut r)]) + }?; r } } diff --git a/rust/nix-util/src/context.rs b/rust/nix-util/src/context.rs index 5a47ac6..cec4063 100644 --- a/rust/nix-util/src/context.rs +++ b/rust/nix-util/src/context.rs @@ -61,15 +61,6 @@ impl Context { r } - /// Run the function, and check the error, then reset the error. - /// Make at most one call to a Nix function in `f`. - /// Do not use if the context isn't fresh or cleared (e.g. with `check_err_and_clear`). - pub fn check_one_call T>(&mut self, f: F) -> Result { - let t = f(self.ptr()); - self.check_err_and_clear()?; - Ok(t) - } - pub fn check_one_call_or_key_none T>( &mut self, f: F, diff --git a/rust/nix-util/src/settings.rs b/rust/nix-util/src/settings.rs index 138ffe6..218e3b5 100644 --- a/rust/nix-util/src/settings.rs +++ b/rust/nix-util/src/settings.rs @@ -2,7 +2,7 @@ use anyhow::Result; use nix_c_raw as raw; use crate::{ - context, result_string_init, + check_call, context, result_string_init, string_return::{callback_get_result_string, callback_get_result_string_data}, }; @@ -10,23 +10,19 @@ pub fn set(key: &str, value: &str) -> Result<()> { let mut ctx = context::Context::new(); let key = std::ffi::CString::new(key)?; let value = std::ffi::CString::new(value)?; - ctx.check_one_call(|ctx_ptr| unsafe { - raw::setting_set(ctx_ptr, key.as_ptr(), value.as_ptr()); - }) + unsafe { + check_call!(raw::setting_set[&mut ctx, key.as_ptr(), value.as_ptr()])?; + } + Ok(()) } pub fn get(key: &str) -> Result { let mut ctx = context::Context::new(); let key = std::ffi::CString::new(key)?; let mut r: Result = result_string_init!(); - ctx.check_one_call(|ctx_ptr| unsafe { - raw::setting_get( - ctx_ptr, - key.as_ptr(), - Some(callback_get_result_string), - callback_get_result_string_data(&mut r), - ) - })?; + unsafe { + check_call!(raw::setting_get[&mut ctx, key.as_ptr(), Some(callback_get_result_string), callback_get_result_string_data(&mut r)])?; + } r }