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(-)
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)
--
2.34.1On 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
> > 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.
Hi Paolo,
I would like to further discuss whether there's any safe issues.
num_timers is a property:
qemu_api::define_property!(
c"timers",
HPETState,
num_timers,
unsafe { &qdev_prop_uint8 },
u8,
default = HPET_MIN_TIMERS
),
Then this means someone could set this property in C side or Rust side
by:
DeviceState *hpet = qdev_new(TYPE_HPET);
qdev_prop_set_uint8(hpet, "timers", 8);
(Though we haven't provide safe interface at Rust side to set property.)
Whatever this happens at C side or Rust side, this depends on QOM core
code (in C) to overwrite the HPETState::num_timers directly.
Then after the call to qdev_prop_set_uint8() starts, all subsequent
processes happen on the C side, so even though the rewriting of num_timers
is runtime, there are no additional safety considerations because it
doesn't cross FFI boundaries. Am I understanding this correctly?
Thanks,
Zhao
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 > >
© 2016 - 2025 Red Hat, Inc.