Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell

[Zhao Liu]

On Tue, Apr 15, 2025 at 12:54:51PM +0200, Paolo Bonzini wrote:
> Date: Tue, 15 Apr 2025 12:54:51 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped
>  in BqlCell
> 
> On 4/14/25 16:49, Zhao Liu wrote:
> > Currently, if the `num` field of a varray is not a numeric type, such as
> > being placed in a wrapper, the array variant of assert_field_type will
> > fail the check.
> > 
> > HPET currently wraps num_timers in BqlCell<>. Although BqlCell<> is not
> > necessary from strictly speaking, it makes sense for vmstate to respect
> > BqlCell.
> 
> Dropping BqlCell<> from num_timers is indeed possible.

I can move the num_timers adjustment from realize() into init().

> But I agree that getting BqlCell<> varrays to work is a good thing anyway;

While num_timers can get out of BqlCell<>, num_timers_save is still
needed to stay in BqlCell<>, since I understand pre_save - as a callback
of the vmstate - still needs the bql protection.

So this patch is still necessary to support migration for HPET.

> then you can separately decide whether to drop BqlCell<> from num_timers.

Yes. In the next version, I can drop BqlCell<> and adjust the C version
as well. But then I can't update the document at the same time, because
the document needs to list the synchronized commit ID. I can update the
document after the HPET migration is able to be merged.

> > The failure of assert_field_type is because it cannot convert BqlCell<T>
> > into usize for use as the index.
> > 
> > Therefore, first, implement `From` trait for common numeric types on
> > BqlCell<>. Then, abstract the wrapper and non-wrapper cases uniformly
> > into a `IntoUsize` trait and make assert_field_type to get usize type
> > index via `IntoUsize` trait.
> > 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   rust/qemu-api/src/assertions.rs | 30 +++++++++++++++++++++++++++++-
> >   rust/qemu-api/src/cell.rs       | 23 +++++++++++++++++++++++
> >   2 files changed, 52 insertions(+), 1 deletion(-)
> 
> I think you can drop the "num=" case of assert_field_type!, and use
> something like this macro:
> 
> /// Drop everything up to the colon, with the intention that
> /// `if_present!` is called inside an optional macro substitution
> /// (such as `$(... $arg ...)?` or `$(... $arg ...)*`).  This allows
> /// expanding `$result` depending on the presence of an argument,
> /// even if the argument itself is not included in `$result`.
> ///
> /// # Examples
> ///
> /// ```
> /// # use qemu_api::if_present;
> /// macro_rules! is_present {

I understand this is_present could have another name to avoid confusion
with our real is_present macro.

> ///     ($($cond:expr)?) => {
> ///         loop {
> ///             $(if_present!([$cond]: break true;);)?
> ///             #[allow(unreachable_code)]
> ///             break false;
> ///         }
> ///     }
> /// }
> ///
> /// assert!(!is_present!());
> /// assert!(is_present!("abc"));
> /// ```
> #[macro_export]
> macro_rules! if_present {
>      ([$($cond:tt)*]: $($result:tt)*) => { $($result)* };
> }
> 
> to expand the array part of the access:
> 
> assert_field_type!(...
>     $($crate::if_present!([$num]: [0]))?;

This example remind me that I introduced a bug into array part:

    let index: usize = v.$num.try_into().unwrap();
    types_must_be_equal::<_, &$ti>(&v.$i[index]);

In the current code, actually it accesses v[num], but when num
stores the length of the whole array, it will cause index out of bounds.

So for current code, at least it should access `v.i[num - 1]`:

    let index: usize = v.$num.try_into().unwrap() - 1; // access the last 
element.
    types_must_be_equal::<_, &$ti>(&v.$i[index]);

> );
> 
> With this change, assert_field_type! is nicer and at least the trait you're
> introducing in assertions.rs goes away...

Yes! Great idea.

Then with your help, we could integrate the array part like:

#[macro_export]
macro_rules! if_present {
    ([$($cond:tt)*]: $($result:tt)*) => { $($result)* };
}

...

#[macro_export]
macro_rules! assert_field_type {
    ($t:ty, $i:tt, $ti:ty $(, $num:ident)?) => {
        const _: () = {
            #[allow(unused)]
            fn assert_field_type(v: $t) {
                fn types_must_be_equal<T, U>(_: T)
                where
                    T: $crate::assertions::EqType<Itself = U>,
                {
                }

                let access = v.$i$($crate::if_present!([$num]: [v.$num - 1])])?;
                types_must_be_equal::<_, $ti>(access);
            }
        };
    };
}

> > +// Orphan rules don't like something like `impl<T> From<BqlCell<T>> for T`.
> > +// It's enough to just implement Into for common types.
> > +macro_rules! impl_into_inner {
> > +    ($type:ty) => {
> > +        impl From<BqlCell<$type>> for $type {
> > +            fn from(c: BqlCell<$type>) -> $type {
> > +                c.get()
> > +            }
> > +        }
> > +    };
> > +}
> 
> ... and it's not clear to me whether this is needed with the change above?
> Would impl_vmstate_transparent!'s definition of VARRAY_FLAG be enough?

If I change the array part like (the change is needed because: cannot
subtract `{integer}` from `BqlCell<u8>`):

- let access = v.$i$([$crate::if_present!([$num]: v.$num) - 1])?;
+ let access = v.$i$([$crate::if_present!([$num]: v.$num).into() - 1])?;

Then there'll be an error:

85   | macro_rules! assert_field_type {
     | ------------------------------ in this expansion of 
`$crate::assert_field_type!` (#2)
...
96   |                 let access = v.$i$([$crate::if_present!([$num]: 
v.$num).into() - 1])?;
     |                                                                         
^^^^ cannot infer type


This is because I want to also check whether "num" would cause index out
of bounds. If we just check the [0] element, then everything is OK...

> If not, I *think* you can do a blanket implementation of Into<T> for
> BqlCell<T>.  Maybe that's nicer, you can decide.

I tired this way, but there's 2 difficulities:
 * Into<T> for BqlCell<T> will violate coherence rules:

error[E0119]: conflicting implementations of trait `Into<_>` for type 
`cell::BqlCell<_>`
   --> qemu-api/src/cell.rs:312:1
    |
312 | impl<T> Into<T> for BqlCell<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `core`:
            - impl<T, U> Into<U> for T
              where U: From<T>;

 * As index, we need to convert BqlCell<T> into T and then convert T
   into usize. :-(

Do you think there is a better way to check array[num -1]? (array's
len() method also returns usize).

Or do you think whether it's necessary to check array[num -1]?
(referring to C version, for example, VMSTATE_STRUCT_VARRAY_UINT8, it
doesn't check the array's out of bounds case.)

Thanks,
Zhao