qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Zhao Liu <zhao1.liu@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-rust@nongnu.org,
	Dapeng Mi <dapeng1.mi@intel.com>
Subject: Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell
Date: Wed, 16 Apr 2025 20:34:37 +0800	[thread overview]
Message-ID: <Z/+j3be+ZT7G1ToL@intel.com> (raw)
In-Reply-To: <Z/97xG5VONqmlK+7@intel.com>

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



  reply	other threads:[~2025-04-16 12:27 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
2025-04-14 14:49 ` [PATCH 1/9] rust/vmstate: Support field_exists check in vmstate_struct macro Zhao Liu
2025-04-16 14:54   ` Paolo Bonzini
2025-04-14 14:49 ` [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell Zhao Liu
2025-04-15 10:54   ` Paolo Bonzini
2025-04-16  9:43     ` Zhao Liu
2025-04-16 12:34       ` Zhao Liu [this message]
2025-04-16 14:24         ` Paolo Bonzini
2025-05-16  8:25     ` Zhao Liu
2025-04-14 14:49 ` [PATCH 3/9] rust/vmstate_test: Test varray with " Zhao Liu
2025-04-14 14:49 ` [PATCH 4/9] rust/vmstate_test: Fix typo in test_vmstate_macro_array_of_pointer_wrapped() Zhao Liu
2025-04-14 14:49 ` [PATCH 5/9] rust/timer: Define NANOSECONDS_PER_SECOND binding as u64 Zhao Liu
2025-04-14 14:49 ` [PATCH 6/9] rust/hpet: convert num_timers to u8 type Zhao Liu
2025-04-14 14:49 ` [PATCH 7/9] rust/hpet: convert HPETTimer index " Zhao Liu
2025-04-14 14:49 ` [PATCH 8/9] rust/hpet: Support migration Zhao Liu
2025-04-15 12:01   ` Zhao Liu
2025-04-15 14:21     ` Paolo Bonzini
2025-04-15 17:43       ` Paolo Bonzini
2025-04-16 10:20         ` Zhao Liu
2025-04-16 14:40           ` Paolo Bonzini
2025-04-16 10:33       ` Zhao Liu
2025-04-14 14:49 ` [PATCH 9/9] rust/hpet: Fix a clippy error Zhao Liu
2025-04-15 10:24 ` [PATCH 0/9] rust/hpet: Initial support for migration Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Z/+j3be+ZT7G1ToL@intel.com \
    --to=zhao1.liu@intel.com \
    --cc=dapeng1.mi@intel.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-rust@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).