| Message ID | 20250414144943.1112885-3-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust/hpet: Initial support for migration | expand |
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. But I agree that getting BqlCell<> varrays to work is a good thing anyway; then you can separately decide whether to drop BqlCell<> from num_timers. > 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 { /// ($($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]))?; ); With this change, assert_field_type! is nicer and at least the trait you're introducing in assertions.rs goes away... > +// 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 not, I *think* you can do a blanket implementation of Into<T> for BqlCell<T>. Maybe that's nicer, you can decide. Paolo
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
> > #[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]); I realize that my thinking was wrong here! The `v` (with specific type) isn't a valid instance, and the variable `num` being passed isn't correctly initialized. Therefore, checking `num`'s value here is meaningless; it's enough to just check if the type matches! > > ); > > > > 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])])?; So, the correct code should just check array[0] as you said: let access = v.$i$($crate::if_present!([$num]: [0])])?; Based on this, there's no need for anything else such as `Into`. > types_must_be_equal::<_, $ti>(access); > } > }; > }; > } Thanks, Zhao
Il mer 16 apr 2025, 14:14 Zhao Liu <zhao1.liu@intel.com> ha scritto: > > let access = v.$i$($crate::if_present!([$num]: [v.$num - > 1])])?; > > So, the correct code should just check array[0] as you said: > > let access = v.$i$($crate::if_present!([$num]: [0])])?; > Except I think the if_present! call would not be in assert_field_type; it would be in the caller, i.e. vmstate.rs. Paolo Based on this, there's no need for anything else such as `Into`. > > > types_must_be_equal::<_, $ti>(access); > > } > > }; > > }; > > } > > Thanks, > Zhao > >
diff --git a/rust/qemu-api/src/assertions.rs b/rust/qemu-api/src/assertions.rs index eb12e9499a72..232cac5b8dba 100644 --- a/rust/qemu-api/src/assertions.rs +++ b/rust/qemu-api/src/assertions.rs @@ -22,6 +22,34 @@ impl<T> EqType for T { type Itself = T; } +pub trait IntoUsize { + fn into_usize(v: Self) -> usize; +} + +macro_rules! impl_into_usize { + ($type:ty) => { + impl IntoUsize for $type { + fn into_usize(v: Self) -> usize { + v.try_into().unwrap() + } + } + + impl IntoUsize for crate::cell::BqlCell<$type> { + fn into_usize(v: Self) -> usize { + let tmp: $type = v.try_into().unwrap(); + tmp.try_into().unwrap() + } + } + }; +} + +// vmstate_n_elems() in C side supports such types. +impl_into_usize!(u8); +impl_into_usize!(u16); +impl_into_usize!(i32); +impl_into_usize!(u32); +impl_into_usize!(u64); + /// Assert that two types are the same. /// /// # Examples @@ -101,7 +129,7 @@ fn types_must_be_equal<T, U>(_: T) T: $crate::assertions::EqType<Itself = U>, { } - let index: usize = v.$num.try_into().unwrap(); + let index: usize = $crate::assertions::IntoUsize::into_usize(v.$num); types_must_be_equal::<_, &$ti>(&v.$i[index]); } }; diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs index ab0785a26928..d31bff093707 100644 --- a/rust/qemu-api/src/cell.rs +++ b/rust/qemu-api/src/cell.rs @@ -309,6 +309,29 @@ fn from(t: T) -> BqlCell<T> { } } +// 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() + } + } + }; +} + +impl_into_inner!(bool); +impl_into_inner!(i8); +impl_into_inner!(i16); +impl_into_inner!(i32); +impl_into_inner!(i64); +impl_into_inner!(u8); +impl_into_inner!(u16); +impl_into_inner!(u32); +impl_into_inner!(u64); +impl_into_inner!(usize); + impl<T: fmt::Debug + Copy> fmt::Debug for BqlCell<T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { self.get().fmt(f)
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. 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(-)