Re: [PATCH 1/2] rust: Add antoher variant for impl_vmstate_struct! macro

[Martin Kletzander <mkletzan@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4566 bytes --]

On Fri, Aug 01, 2025 at 11:44:03PM +0200, Paolo Bonzini wrote:
>Il ven 1 ago 2025, 17:00 Martin Kletzander <mkletzan@redhat.com> ha scritto:
>
>> From: Martin Kletzander <mkletzan@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 <mkletzan@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
>>
>>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]