Message ID | 20250317151236.536673-5-zhao1.liu@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | rust/vmstate: Clean up, fix, enhance & test | expand |
On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > When specify an array field in vmstate_struct macro, there will be an > error: > > > local ambiguity when calling macro `vmstate_struct`: multiple parsing > > options: built-in NTs expr ('vmsd') or 1 other option. > > This is because "expr" can't recognize the "vmsd" field correctly, so > that it gets confused with the previous array field. > > To fix the above issue, use "ident" for "vmsd" field, and explicitly > refer to it in the macro. I think this is not needed if the varray case is left as is, and other cases use .with_...() instead of arguments? Paolo > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > rust/hw/char/pl011/src/device_class.rs | 2 +- > rust/qemu-api/src/vmstate.rs | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs > index 0b2076ddaa0f..e43a5d6cd063 100644 > --- a/rust/hw/char/pl011/src/device_class.rs > +++ b/rust/hw/char/pl011/src/device_class.rs > @@ -72,7 +72,7 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int { > post_load: Some(pl011_post_load), > fields: vmstate_fields! { > vmstate_unused!(core::mem::size_of::<u32>()), > - vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>), > + vmstate_struct!(PL011State, regs, VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>), > }, > subsections: vmstate_subsections! { > VMSTATE_PL011_CLOCK > diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs > index 94efbd8bb735..3f95d4825149 100644 > --- a/rust/qemu-api/src/vmstate.rs > +++ b/rust/qemu-api/src/vmstate.rs > @@ -435,7 +435,7 @@ macro_rules! vmstate_unused { > #[doc(alias = "VMSTATE_STRUCT")] > #[macro_export] > macro_rules! vmstate_struct { > - ($struct_name:ty, $field_name:ident $(, [0 .. $num:ident $(* $factor:expr)?])?, $vmsd:expr, $type:ty $(,)?) => { > + ($struct_name:ty, $field_name:ident $(, [0 .. $num:ident $(* $factor:expr)?])?, $vmsd:ident, $type:ty $(,)?) => { > $crate::bindings::VMStateField { > name: ::core::concat!(::core::stringify!($field_name), "\0") > .as_bytes() > @@ -447,7 +447,7 @@ macro_rules! vmstate_struct { > }, > size: ::core::mem::size_of::<$type>(), > flags: $crate::bindings::VMStateFlags::VMS_STRUCT, > - vmsd: $vmsd, > + vmsd: &$vmsd, > ..$crate::zeroable::Zeroable::ZERO $( > .with_varray_flag($crate::call_func_with_field!( > $crate::vmstate::vmstate_varray_flag, > -- > 2.34.1 >
On Mon, Mar 17, 2025 at 06:17:07PM +0100, Paolo Bonzini wrote: > Date: Mon, 17 Mar 2025 18:17:07 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH 04/17] rust/vmstate: Use ident instead of expr to parse > vmsd in vmstate_struct macro > > On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > > > When specify an array field in vmstate_struct macro, there will be an > > error: > > > > > local ambiguity when calling macro `vmstate_struct`: multiple parsing > > > options: built-in NTs expr ('vmsd') or 1 other option. > > > > This is because "expr" can't recognize the "vmsd" field correctly, so > > that it gets confused with the previous array field. > > > > To fix the above issue, use "ident" for "vmsd" field, and explicitly > > refer to it in the macro. > > I think this is not needed if the varray case is left as is, and other > cases use .with_...() instead of arguments? > Yes! With a[0..num], the `vmsd` could be parsed correctly. I'll drop this patch as well and refresh unit tests. Thanks, Zhao
On Tue, Mar 18, 2025 at 10:46:10AM +0800, Zhao Liu wrote: > Date: Tue, 18 Mar 2025 10:46:10 +0800 > From: Zhao Liu <zhao1.liu@intel.com> > Subject: Re: [PATCH 04/17] rust/vmstate: Use ident instead of expr to parse > vmsd in vmstate_struct macro > > On Mon, Mar 17, 2025 at 06:17:07PM +0100, Paolo Bonzini wrote: > > Date: Mon, 17 Mar 2025 18:17:07 +0100 > > From: Paolo Bonzini <pbonzini@redhat.com> > > Subject: Re: [PATCH 04/17] rust/vmstate: Use ident instead of expr to parse > > vmsd in vmstate_struct macro > > > > On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > > > > > When specify an array field in vmstate_struct macro, there will be an > > > error: > > > > > > > local ambiguity when calling macro `vmstate_struct`: multiple parsing > > > > options: built-in NTs expr ('vmsd') or 1 other option. > > > > > > This is because "expr" can't recognize the "vmsd" field correctly, so > > > that it gets confused with the previous array field. > > > > > > To fix the above issue, use "ident" for "vmsd" field, and explicitly > > > refer to it in the macro. > > > > I think this is not needed if the varray case is left as is, and other > > cases use .with_...() instead of arguments? > > > > Yes! With a[0..num], the `vmsd` could be parsed correctly. I'll drop this > patch as well and refresh unit tests. > Additionally, at present, IMO it is not suitable to replace the vmsd argument with .with_vmsd() method because VMS_STRUCT requires a vmsd field, and .with_vmsd() is optional. There is no way to ensure that vmsd is not omitted... unless VMS_STRUCT is also set within .with_vmsd(). This would be akin to merging vmstate_struct and vmstate_of, but I suppose that would be a long way as for now. So I prefer vmsd argument at the moment :-) Thanks, Zhao
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs index 0b2076ddaa0f..e43a5d6cd063 100644 --- a/rust/hw/char/pl011/src/device_class.rs +++ b/rust/hw/char/pl011/src/device_class.rs @@ -72,7 +72,7 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int { post_load: Some(pl011_post_load), fields: vmstate_fields! { vmstate_unused!(core::mem::size_of::<u32>()), - vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>), + vmstate_struct!(PL011State, regs, VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>), }, subsections: vmstate_subsections! { VMSTATE_PL011_CLOCK diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 94efbd8bb735..3f95d4825149 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -435,7 +435,7 @@ macro_rules! vmstate_unused { #[doc(alias = "VMSTATE_STRUCT")] #[macro_export] macro_rules! vmstate_struct { - ($struct_name:ty, $field_name:ident $(, [0 .. $num:ident $(* $factor:expr)?])?, $vmsd:expr, $type:ty $(,)?) => { + ($struct_name:ty, $field_name:ident $(, [0 .. $num:ident $(* $factor:expr)?])?, $vmsd:ident, $type:ty $(,)?) => { $crate::bindings::VMStateField { name: ::core::concat!(::core::stringify!($field_name), "\0") .as_bytes() @@ -447,7 +447,7 @@ macro_rules! vmstate_struct { }, size: ::core::mem::size_of::<$type>(), flags: $crate::bindings::VMStateFlags::VMS_STRUCT, - vmsd: $vmsd, + vmsd: &$vmsd, ..$crate::zeroable::Zeroable::ZERO $( .with_varray_flag($crate::call_func_with_field!( $crate::vmstate::vmstate_varray_flag,