| Message ID | 20250317151236.536673-14-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust/vmstate: Clean up, fix, enhance & test | expand |
On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > In C version, VMSTATE_VALIDATE accepts the function pointer, which is > used to check if some conditions of structure could meet, although the > C version macro doesn't accept any structure as the opaque type. > > But it's hard to integrate VMSTATE_VALIDAE into vmstate_struct, a new > macro has to be introduced to specifically handle the case corresponding > to VMSTATE_VALIDATE. > > One of the difficulties is inferring the type of a callback by its name > `test_fn`. We can't directly use `test_fn` as a parameter of > test_cb_builder__() to get its type "F", because in this way, Rust > compiler will be too conservative on drop check and complain "the > destructor for this type cannot be evaluated in constant functions". > > Fortunately, PhantomData<T> could help in this case, because it is > considered to never have a destructor, no matter its field type [*]. > > The `phantom__()` in the `call_func_with_field` macro provides a good > example of using PhantomData to infer type. So copy this idea and apply > it to the `vmstate_validate` macro. > > [*]: https://doc.rust-lang.org/std/ops/trait.Drop.html#drop-check > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > rust/qemu-api/src/vmstate.rs | 57 +++++++++++++++++++++++++++++++++++- > 1 file changed, 56 insertions(+), 1 deletion(-) > > diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs > index bb41bfd291c0..4eefd54ca120 100644 > --- a/rust/qemu-api/src/vmstate.rs > +++ b/rust/qemu-api/src/vmstate.rs > @@ -25,9 +25,12 @@ > //! functionality that is missing from `vmstate_of!`. > > use core::{marker::PhantomData, mem, ptr::NonNull}; > +use std::os::raw::{c_int, c_void}; > > pub use crate::bindings::{VMStateDescription, VMStateField}; > -use crate::{bindings::VMStateFlags, prelude::*, qom::Owned, zeroable::Zeroable}; > +use crate::{ > + bindings::VMStateFlags, callbacks::FnCall, prelude::*, qom::Owned, zeroable::Zeroable, > +}; > > /// This macro is used to call a function with a generic argument bound > /// to the type of a field. The function must take a > @@ -292,6 +295,14 @@ pub const fn with_varray_multiply(mut self, num: u32) -> VMStateField { > self.num = num as i32; > self > } > + > + #[must_use] > + pub const fn with_exist_check(mut self) -> Self { > + assert!(self.flags.0 == 0); > + self.flags = VMStateFlags(VMStateFlags::VMS_MUST_EXIST.0 | VMStateFlags::VMS_ARRAY.0); > + self.num = 0; // 0 elements: no data, only run test_fn callback > + self > + } > } > > /// This macro can be used (by just passing it a type) to forward the `VMState` > @@ -510,6 +521,50 @@ macro_rules! vmstate_fields { > }} > } > > +pub extern "C" fn rust_vms_test_field_exists<T, F: for<'a> FnCall<(&'a T, u8), bool>>( > + opaque: *mut c_void, > + version_id: c_int, > +) -> bool { > + let owner: &T = unsafe { &*(opaque.cast::<T>()) }; > + let version: u8 = version_id.try_into().unwrap(); > + // SAFETY: the opaque was passed as a reference to `T`. > + F::call((owner, version)) > +} > + > +pub type VMSFieldExistCb = unsafe extern "C" fn( > + opaque: *mut std::os::raw::c_void, > + version_id: std::os::raw::c_int, > +) -> bool; > + > +#[doc(alias = "VMSTATE_VALIDATE")] > +#[macro_export] > +macro_rules! vmstate_validate { > + ($struct_name:ty, $test_name:expr, $test_fn:expr $(,)?) => { > + $crate::bindings::VMStateField { > + name: ::std::ffi::CStr::as_ptr($test_name), > + // TODO: Use safe callback. Why is the TODO still there? > + field_exists: { > + const fn test_cb_builder__< > + T, > + F: for<'a> $crate::callbacks::FnCall<(&'a T, u8), bool>, > + >( > + _phantom: ::core::marker::PhantomData<F>, > + ) -> $crate::vmstate::VMSFieldExistCb { > + let _: () = F::ASSERT_IS_SOME; > + $crate::vmstate::rust_vms_test_field_exists::<T, F> > + } > + > + const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> { > + ::core::marker::PhantomData > + } > + Some(test_cb_builder__::<$struct_name, _>(phantom__(&$test_fn))) > + }, > + ..$crate::zeroable::Zeroable::ZERO > + } > + .with_exist_check() > + }; Would it be possible, or make sense, to move most of the code for field_exists inside .with_exist_check()? Paolo
Il mar 18 mar 2025, 07:16 Zhao Liu <zhao1.liu@intel.com> ha scritto: > > Would it be possible, or make sense, to move most of the code for > > field_exists inside .with_exist_check()? > > > > If so, the method would be like: > > pub fn with_exist_check<T, F>( > mut self, > _cb: F > ) -> Self > where > F: for<'a> FnCall<(&'a T, u8), bool>, > > Then the use case could be like: > > vmstate_struct!(HPETState, timers[0 .. num_timers], > &VMSTATE_HPET_TIMER, > BqlRefCell<HPETTimer>).with_exist_check<HPETState, _>(foo_field_check), > > Here we need to specify the structure type in with_exist_check, though it's > already specified in vmstate_struct as the first field. > > In this way, I understand with_exist_check() doesn't need phantom__() > trick. > > Instead, (after I dropped the few patches you mentioned,) now vmstate_of > & vmstate_struct could accept the optional "test_fn" field (luckily, at > least test_fn can still be parsed!), then the example would be: > > vmstate_struct!(HPETState, timers[0 .. num_timers], > &VMSTATE_HPET_TIMER, > BqlRefCell<HPETTimer>, foo_field_check) > > And in this way, phantom__() is necessary. > > So I think the main issue is the format, which do you prefer? > For now I would leave out a generic field-exists check, and keep the implementation of vmstate_validate as you did it in v1. Once we look more in the builder concept we can think of adding also a VMStateField<T> (with a PhantomData<fn(&T) -> bool> inside) and add with_field_exists(). Paolo > Thanks, > Zhao > >
> > +#[doc(alias = "VMSTATE_VALIDATE")] > > +#[macro_export] > > +macro_rules! vmstate_validate { > > + ($struct_name:ty, $test_name:expr, $test_fn:expr $(,)?) => { > > + $crate::bindings::VMStateField { > > + name: ::std::ffi::CStr::as_ptr($test_name), > > + // TODO: Use safe callback. > > Why is the TODO still there? I forgot to delete this comment... > > + field_exists: { > > + const fn test_cb_builder__< > > + T, > > + F: for<'a> $crate::callbacks::FnCall<(&'a T, u8), bool>, > > + >( > > + _phantom: ::core::marker::PhantomData<F>, > > + ) -> $crate::vmstate::VMSFieldExistCb { > > + let _: () = F::ASSERT_IS_SOME; > > + $crate::vmstate::rust_vms_test_field_exists::<T, F> > > + } > > + > > + const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> { > > + ::core::marker::PhantomData > > + } > > + Some(test_cb_builder__::<$struct_name, _>(phantom__(&$test_fn))) > > + }, > > + ..$crate::zeroable::Zeroable::ZERO > > + } > > + .with_exist_check() > > + }; > > Would it be possible, or make sense, to move most of the code for > field_exists inside .with_exist_check()? > If so, the method would be like: pub fn with_exist_check<T, F>( mut self, _cb: F ) -> Self where F: for<'a> FnCall<(&'a T, u8), bool>, Then the use case could be like: vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>).with_exist_check<HPETState, _>(foo_field_check), Here we need to specify the structure type in with_exist_check, though it's already specified in vmstate_struct as the first field. In this way, I understand with_exist_check() doesn't need phantom__() trick. Instead, (after I dropped the few patches you mentioned,) now vmstate_of & vmstate_struct could accept the optional "test_fn" field (luckily, at least test_fn can still be parsed!), then the example would be: vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, foo_field_check) And in this way, phantom__() is necessary. So I think the main issue is the format, which do you prefer? Thanks, Zhao
> For now I would leave out a generic field-exists check, and keep the > implementation of vmstate_validate as you did it in v1. > > Once we look more in the builder concept we can think of adding also a > VMStateField<T> (with a PhantomData<fn(&T) -> bool> inside) and add > with_field_exists(). This makes sense! Thanks, Zhao
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index bb41bfd291c0..4eefd54ca120 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -25,9 +25,12 @@ //! functionality that is missing from `vmstate_of!`. use core::{marker::PhantomData, mem, ptr::NonNull}; +use std::os::raw::{c_int, c_void}; pub use crate::bindings::{VMStateDescription, VMStateField}; -use crate::{bindings::VMStateFlags, prelude::*, qom::Owned, zeroable::Zeroable}; +use crate::{ + bindings::VMStateFlags, callbacks::FnCall, prelude::*, qom::Owned, zeroable::Zeroable, +}; /// This macro is used to call a function with a generic argument bound /// to the type of a field. The function must take a @@ -292,6 +295,14 @@ pub const fn with_varray_multiply(mut self, num: u32) -> VMStateField { self.num = num as i32; self } + + #[must_use] + pub const fn with_exist_check(mut self) -> Self { + assert!(self.flags.0 == 0); + self.flags = VMStateFlags(VMStateFlags::VMS_MUST_EXIST.0 | VMStateFlags::VMS_ARRAY.0); + self.num = 0; // 0 elements: no data, only run test_fn callback + self + } } /// This macro can be used (by just passing it a type) to forward the `VMState` @@ -510,6 +521,50 @@ macro_rules! vmstate_fields { }} } +pub extern "C" fn rust_vms_test_field_exists<T, F: for<'a> FnCall<(&'a T, u8), bool>>( + opaque: *mut c_void, + version_id: c_int, +) -> bool { + let owner: &T = unsafe { &*(opaque.cast::<T>()) }; + let version: u8 = version_id.try_into().unwrap(); + // SAFETY: the opaque was passed as a reference to `T`. + F::call((owner, version)) +} + +pub type VMSFieldExistCb = unsafe extern "C" fn( + opaque: *mut std::os::raw::c_void, + version_id: std::os::raw::c_int, +) -> bool; + +#[doc(alias = "VMSTATE_VALIDATE")] +#[macro_export] +macro_rules! vmstate_validate { + ($struct_name:ty, $test_name:expr, $test_fn:expr $(,)?) => { + $crate::bindings::VMStateField { + name: ::std::ffi::CStr::as_ptr($test_name), + // TODO: Use safe callback. + field_exists: { + const fn test_cb_builder__< + T, + F: for<'a> $crate::callbacks::FnCall<(&'a T, u8), bool>, + >( + _phantom: ::core::marker::PhantomData<F>, + ) -> $crate::vmstate::VMSFieldExistCb { + let _: () = F::ASSERT_IS_SOME; + $crate::vmstate::rust_vms_test_field_exists::<T, F> + } + + const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> { + ::core::marker::PhantomData + } + Some(test_cb_builder__::<$struct_name, _>(phantom__(&$test_fn))) + }, + ..$crate::zeroable::Zeroable::ZERO + } + .with_exist_check() + }; +} + /// A transparent wrapper type for the `subsections` field of /// [`VMStateDescription`]. ///
In C version, VMSTATE_VALIDATE accepts the function pointer, which is used to check if some conditions of structure could meet, although the C version macro doesn't accept any structure as the opaque type. But it's hard to integrate VMSTATE_VALIDAE into vmstate_struct, a new macro has to be introduced to specifically handle the case corresponding to VMSTATE_VALIDATE. One of the difficulties is inferring the type of a callback by its name `test_fn`. We can't directly use `test_fn` as a parameter of test_cb_builder__() to get its type "F", because in this way, Rust compiler will be too conservative on drop check and complain "the destructor for this type cannot be evaluated in constant functions". Fortunately, PhantomData<T> could help in this case, because it is considered to never have a destructor, no matter its field type [*]. The `phantom__()` in the `call_func_with_field` macro provides a good example of using PhantomData to infer type. So copy this idea and apply it to the `vmstate_validate` macro. [*]: https://doc.rust-lang.org/std/ops/trait.Drop.html#drop-check Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- rust/qemu-api/src/vmstate.rs | 57 +++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-)