Re: [PATCH 5/7] rust: qemu-macros: add ToMigrationState derive macro

[Paolo Bonzini <pbonzini@redhat.com>]

On 9/25/25 14:35, Zhao Liu wrote:
>> +/// #[derive(ToMigrationState)]
>>   /// pub struct DeviceRegs {
>>   ///     status: u32,
>>   /// }
>> +/// # unsafe impl VMState for DeviceRegsMigration {
>> +/// #     const BASE: VMStateField = ::common::Zeroable::ZERO;
>> +/// # }
> 
> Outdated comment? Looks like the DeviceRegsMigration definition is
> missing.

It's defined by the #[derive(ToMigrationState)].

> the below conversion is ugly:>
> #[property(rename = "msi", bit = HPET_FLAG_MSI_SUPPORT_SHIFT as u8, default = false)]
> 
> conversion should happen within the macro parsing process. But unfortunately,
> try_into() is not const, maybe I could do this for bit property:
> 
> diff --git a/rust/qemu-macros/src/lib.rs b/rust/qemu-macros/src/lib.rs
> index c459f9bcb42f..e67df57c3712 100644
> --- a/rust/qemu-macros/src/lib.rs
> +++ b/rust/qemu-macros/src/lib.rs
> @@ -275,7 +275,10 @@ macro_rules! str_to_c_str {
>                   name: ::std::ffi::CStr::as_ptr(#prop_name),
>                   info: #qdev_prop,
>                   offset: ::core::mem::offset_of!(#name, #field_name) as isize,
> -                bitnr: #bitnr,
> +                bitnr: {
> +                    const _: () = assert!(#bitnr <= u8::MAX as _, "bit exceeds u8 range");
> +                    #bitnr as u8
> +                },
>                   set_default: #set_default,
>                   defval: ::hwcore::bindings::Property__bindgen_ty_1 { u: #defval as u64 },
>                   ..::common::Zeroable::ZERO

Good idea (also testing >= 0 is needed).  "const { assert!(...); }" is 
even simpler.

>> +/// - `#[migration_state(clone)]` - Clones the field value.
> 
> How about emphasizing the use case?
> 
> "Clones the field value, especially for the types don't implement `Copy`."

I don't have a use case yet to be honest, but at the same time I want to 
help potential device authors and remove the need to muck with the macro.

>> +/// Fields without any attributes use `ToMigrationState` recursively; note that
>> +/// this is a simple copy for types that implement `Copy`.
>> +///
>> +/// # Attribute compatibility
>> +///
>> +/// - `omit` cannot be used with any other attributes
>> +/// - only one of `into(Type)`, `try_into(Type)` can be used, but they can be
>> +///   coupled with `clone`.
>> +///
> 
> ...
> 
> The implementation of the entire macro is great.

Thanks. :)  It's indeed pretty easy to follow, and I like procedural 
macro code that is simple but powerful.

The attrs crate also helps a lot!

>> +#[test]
>> +fn test_derive_to_migration_state() {
> 
> ...
> 
>> +        quote! {
>> +            #[derive(Default)]
>> +            pub struct CustomMigration {
>> +                pub shared_data: String,
>> +                pub converted_field: Cow<'static, str>,
>> +                pub fallible_field: i8,
>> +                pub nested_field: <NestedStruct as ToMigrationState>::Migrated,
>> +                pub simple_field: <u32 as ToMigrationState>::Migrated,
>> +            }
> 
> In the production code, CustomMigration still needs to implement VMState
> trait, so that String & Cow<'static, str> also need to implement VMState
> trait. This seems like the thing that we are currently missing.

Or more simply they're not chosen well. :)  For the documentation I will 
think of better types.

> For test, it's enough to show how the macro works.

Yes, for testing it's a lesser deal.

Paolo