diff mbox series

[v2,10/14] rust/vmstate: Support vmstate_validate

Message ID 20250318083248.1402990-11-zhao1.liu@intel.com (mailing list archive)
State New
Headers show
Series rust/vmstate: Clean up, fix, enhance & test | expand

Commit Message

Zhao Liu March 18, 2025, 8:32 a.m. UTC
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.

Additionally, add a `with_exist_check()` method to help add callback
in future.

[*]: https://doc.rust-lang.org/std/ops/trait.Drop.html#drop-check

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since v1:
 * Rename `with_exist_check()` to `with_validate_flag()`.
 * Add a `with_exist_check()` method to help add field_exists callback,
   though it can't be used in static VMStateDescription definition but
   will be useful for vmstate builder.
---
 rust/qemu-api/src/vmstate.rs | 72 +++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini March 18, 2025, 10:11 a.m. UTC | #1
On 3/18/25 09:32, Zhao Liu 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.
> 
> Additionally, add a `with_exist_check()` method to help add callback
> in future.

I think for now, until it can be const, we can remove it and clean up
things a bit:

diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 62a0308014e..2786e8ae709 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -294,30 +294,6 @@ pub const fn with_varray_multiply(mut self, num: u32) -> VMStateField {
          self.num = num as i32;
          self
      }
-
-    #[must_use]
-    pub const fn with_validate_flag(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
-    }
-
-    // FIXME: Unfortunately, this non-const fn cannot currently be called in a
-    // static context. Also, it can't be const because the compiler complains
-    // about "constant functions cannot evaluate destructors" for `F`. Add it
-    // for future vmstate builder.
-    #[must_use]
-    pub fn with_exist_check<T, F>(mut self, _cb: F) -> Self
-    where
-        F: for<'a> FnCall<(&'a T, u8), bool>,
-    {
-        assert!(self.field_exists.is_none());
-        let _: () = F::ASSERT_IS_SOME;
-
-        self.field_exists = Some(rust_vms_test_field_exists::<T, F>);
-        self
-    }
  }
  
  /// This macro can be used (by just passing it a type) to forward the `VMState`
@@ -572,9 +548,9 @@ const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> {
                  }
                  Some(test_cb_builder__::<$struct_name, _>(phantom__(&$test_fn)))
              },
+            flags: VMStateFlags(VMStateFlags::VMS_MUST_EXIST.0 | VMStateFlags::VMS_ARRAY.0),
              ..$crate::zeroable::Zeroable::ZERO
          }
-        .with_validate_flag()
      };
  }
  

Otherwise the series looks great, thanks!

Paolo
Zhao Liu March 18, 2025, 12:23 p.m. UTC | #2
>  /// This macro can be used (by just passing it a type) to forward the `VMState`
> @@ -572,9 +548,9 @@ const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> {
>                  }
>                  Some(test_cb_builder__::<$struct_name, _>(phantom__(&$test_fn)))
>              },

I want to keep the comment here as C version did, because there's an array flag :

// Though VMS_ARRAY is set, the num is 0: no data, only run test_fn callback

> +            flags: VMStateFlags(VMStateFlags::VMS_MUST_EXIST.0 | VMStateFlags::VMS_ARRAY.0),
>              ..$crate::zeroable::Zeroable::ZERO
>          }
> -        .with_validate_flag()
>      };
>  }
> 
> Otherwise the series looks great, thanks!

Let me refresh the v3. :-)

Thanks,
Zhao
Paolo Bonzini March 18, 2025, 12:32 p.m. UTC | #3
Il mar 18 mar 2025, 13:03 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> >  /// This macro can be used (by just passing it a type) to forward the
> `VMState`
> > @@ -572,9 +548,9 @@ const fn phantom__<T>(_: &T) ->
> ::core::marker::PhantomData<T> {
> >                  }
> >                  Some(test_cb_builder__::<$struct_name,
> _>(phantom__(&$test_fn)))
> >              },
>
> I want to keep the comment here as C version did, because there's an array
> flag
>

Ok, let's add it. No need to send v3 since it's just a single patch and
with no changes further down.

Paolo

// Though VMS_ARRAY is set, the num is 0: no data, only run test_fn callback
>
> > +            flags: VMStateFlags(VMStateFlags::VMS_MUST_EXIST.0 |
> VMStateFlags::VMS_ARRAY.0),
> >              ..$crate::zeroable::Zeroable::ZERO
> >          }
> > -        .with_validate_flag()
> >      };
> >  }
> >
> > Otherwise the series looks great, thanks!
>
> Let me refresh the v3. :-)
>
> Thanks,
> Zhao
>
>
Zhao Liu March 18, 2025, 1:07 p.m. UTC | #4
On Tue, Mar 18, 2025 at 01:32:03PM +0100, Paolo Bonzini wrote:
> Date: Tue, 18 Mar 2025 13:32:03 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH v2 10/14] rust/vmstate: Support vmstate_validate
> 
> Il mar 18 mar 2025, 13:03 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> 
> > >  /// This macro can be used (by just passing it a type) to forward the
> > `VMState`
> > > @@ -572,9 +548,9 @@ const fn phantom__<T>(_: &T) ->
> > ::core::marker::PhantomData<T> {
> > >                  }
> > >                  Some(test_cb_builder__::<$struct_name,
> > _>(phantom__(&$test_fn)))
> > >              },
> >
> > I want to keep the comment here as C version did, because there's an array
> > flag
> >
> 
> Ok, let's add it. No need to send v3 since it's just a single patch and
> with no changes further down.
> 

Thanks and sorry, I missed your notification because I didn't actively
pull my mutt just now... but, v3 might also be worthwhile.

While setting flags for vmstate_validate, I also took the opportunity to
do a little cleanup on vmstate_clock. I hope it didn't cause any extra
burden. :-)

Regards,
Zhao
diff mbox series

Patch

diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 01f06ed7379e..62a0308014e3 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
@@ -291,6 +294,30 @@  pub const fn with_varray_multiply(mut self, num: u32) -> VMStateField {
         self.num = num as i32;
         self
     }
+
+    #[must_use]
+    pub const fn with_validate_flag(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
+    }
+
+    // FIXME: Unfortunately, this non-const fn cannot currently be called in a
+    // static context. Also, it can't be const because the compiler complains
+    // about "constant functions cannot evaluate destructors" for `F`. Add it
+    // for future vmstate builder.
+    #[must_use]
+    pub fn with_exist_check<T, F>(mut self, _cb: F) -> Self
+    where
+        F: for<'a> FnCall<(&'a T, u8), bool>,
+    {
+        assert!(self.field_exists.is_none());
+        let _: () = F::ASSERT_IS_SOME;
+
+        self.field_exists = Some(rust_vms_test_field_exists::<T, F>);
+        self
+    }
 }
 
 /// This macro can be used (by just passing it a type) to forward the `VMState`
@@ -508,6 +535,49 @@  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),
+            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_validate_flag()
+    };
+}
+
 /// A transparent wrapper type for the `subsections` field of
 /// [`VMStateDescription`].
 ///