On Fri, Aug 01, 2025 at 11:44:03PM +0200, Paolo Bonzini wrote:
Il ven 1 ago 2025, 17:00 Martin Kletzander <mklet...@redhat.com> ha scritto:From: Martin Kletzander <mklet...@redhat.com> In some cases (e.g. in vmstate_tests.rs) the second argument to impl_vmstate_struct! is actually an existing struct which is then copied (since VMStateDescription implements Copy) when saved into the static VMSD using .get(). That is not a problem because it is part of the data segment and the pointers are not being free'd since they point to static data. But it is a problem when tests rely on comparing the VMState descriptions as pointers rather than contents. And it also wastes space, more or less. Introduce second variant of the macro which can, instead of the expression, take an identifier or what looks like a reference. This second variant is added before the current variant so that it has preference, and only references the existing static data from it. This way tests are fixed and space is saved. And now that the VMStateDescription checking is fixed we can also check for the right value in test_vmstate_struct_varray_uint8_wrapper(). Signed-off-by: Martin Kletzander <mklet...@redhat.com> --- I'm not sure whether this is caused by different utility on my system or bash version or whatever, but without this patch these three tests fail for me and this patch fixes it.I found something similar, though I wasn't sure if it was broken in master as well or only in the rust-next branch.
It is not broken on master, but it *was* broken on rust-next.
If that works in master as well, I would remove completely the possibility of using &FOO, and always use .as_ref(). It's more efficient as you said, and there's no reason that I know to use the less efficient one.
That would mean that you cannot do what's done in e.g. pl011
impl_vmstate_struct!(
PL011Registers,
VMStateDescriptionBuilder::<PL011Registers>::new()
...
.build()
);
requiring you to always do:
static/const VMSTATE_PL011_REGISTERS: VMStateDescription<PL011Registers> =
VMStateDescriptionBuilder::<PL011Registers>::new()
...
.build();
impl_vmstate_struct!(PL011Registers, VMSTATE_PL011_REGISTERS);
*BUT* of course I had to rebase the patches on top of current rust-next
on Friday and there were some of your commits from Thursday which I now
see actually fix all what I tried fixing before as well. I tried
finding the previous commit on which I saw all the issues and after some
rebuilding I could not. So it is now not even broken on rust-next.
This way I completely wasted your time, but at least learned something
that's happening in the code. Sorry for that.
Martin
Paolo rust/qemu-api/src/vmstate.rs | 11 +++++++++++rust/qemu-api/tests/vmstate_tests.rs | 1 + 2 files changed, 12 insertions(+) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index b5c6b764fbba..716e52afe740 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -449,6 +449,17 @@ macro_rules! vmstate_validate { /// description of the struct. #[macro_export] macro_rules! impl_vmstate_struct { + ($type:ty, $(&)?$vmsd:ident) => { + unsafe impl $crate::vmstate::VMState for $type { + const BASE: $crate::bindings::VMStateField = + $crate::bindings::VMStateField { + vmsd: $vmsd.as_ref(), + size: ::core::mem::size_of::<$type>(), + flags: $crate::bindings::VMStateFlags::VMS_STRUCT, + ..$crate::zeroable::Zeroable::ZERO + }; + } + }; ($type:ty, $vmsd:expr) => { unsafe impl $crate::vmstate::VMState for $type { const BASE: $crate::bindings::VMStateField = { diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/ vmstate_tests.rs index 2c0670ba0eed..7d3180e6c2ea 100644 --- a/rust/qemu-api/tests/vmstate_tests.rs +++ b/rust/qemu-api/tests/vmstate_tests.rs @@ -320,6 +320,7 @@ fn test_vmstate_struct_varray_uint8_wrapper() { b"arr_a_wrap\0" ); assert_eq!(foo_fields[5].num_offset, 228); + assert_eq!(foo_fields[5].vmsd, VMSTATE_FOOA.as_ref()); assert!(unsafe { foo_fields[5].field_exists.unwrap()(foo_b_p, 0) }); // The last VMStateField in VMSTATE_FOOB. -- 2.50.1
signature.asc
Description: PGP signature
