Re: [PATCH 8/9] rust/hpet: Support migration

[Zhao Liu <zhao1.liu@intel.com>]

> > Although it can handle callbacks well, I found that the difficulty still
> > lies in the fact that the vmstate_fields and vmstate_subsections macros
> > cannot be eliminated, because any dynamic creation of arrays is not
> > allowed in a static context!
> 
> Yes, this makes sense.  Array size must be known inside a const function and
> the extra terminator at the end of fields and subsections cannot be added by
> the builder itself.  c_str! has the same issue for the name, if I understand
> correctly.

Yes, I have to use c_str! in name().

> > In any case, it's definitely still rough, but hope it helps and
> > takes a small step forward.
> 
> Yes, of course---this:
> 
> +static VMSTATE_HPET_RTC_IRQ_LEVEL: VMStateDescription<HPETState> =
> +    VMStateDescriptionBuilder::<HPETState>::new()
> +        .name(c_str!("hpet/rtc_irq_level"))
> +        .version_id(1)
> +        .minimum_version_id(1)
> +        .needed(&HPETState::is_rtc_irq_level_needed)
> +        .fields(vmstate_fields! {
> +            vmstate_of!(HPETState, rtc_irq_level),
> +        })
> +        .build();
> +
> 
> is readable, not foreign (it's similar to the MemoryRegionOps) and provides
> an easy way to insert FFI wrappers.
> 
> Right now it's now fully typesafe, because the VMStateField returned by
> vmstate_of! (as well as the arrays returned by vmstate_fields! and
> vmstate_subsections!) does not track that it's for an HPETState; but that's
> a small thing overall and getting the basic builder right is more important.

I agree, additional consideration is needed here. Currently it is
vmstate_fields! that limits changes to vmstate_of!.

> I also made a note to check which callbacks could have a Result<> as the
> return type, possibly reusing the Errno module (Result<(), ()> looks a bit
> silly); but that is also not needed for this early stage.
> 
> Just a couple notes:
> 
> > +    bindings::{VMStateDescription as RawVMStateDescription, VMStateFlags},
> 
> I would use bindings::VMStateDescription throughout, similar to how
> it's done in memory.rs.

Sure, will fix.

> > +    pub const fn name(mut self, name_str: &CStr) -> Self {
> > +        self.0.name = ::std::ffi::CStr::as_ptr(name_str);
> 
> 
> This can use "name_str.as_ptr()" because the type of name_str is known
> (unlike in macros, such as define_property! or vmstate_validate!).

I see and will fix.

> (By the way, talking about macros, I have just stumbled on the attrs crate,
> which is something to keep an eye on for when QOM/qdev bindings are extended
> along the lines of https://lore.kernel.org/qemu-devel/e8e55772-906b-42cb-a744-031e6ae65f16@redhat.com/T/.
> But I don't think procedural macros are a good match for VMState).

I didn't have a deep understanding of this previously :-(. I'll take a
closer look at this.

Thanks,
Zhao