Re: [PATCH v2 10/14] rust/vmstate: Support vmstate_validate

[Paolo Bonzini <pbonzini@redhat.com>]

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