Re: [PATCH 13/17] rust/vmstate: Support vmstate_validate

[Zhao Liu <zhao1.liu@intel.com>]

> > +#[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