On Thu, Jul 10, 2025 at 5:26 PM Paolo Bonzini <[email protected]> wrote:
>
> On Thu, Jul 10, 2025 at 11:41 AM Manos Pitsidianakis
> <[email protected]> wrote:
> > > Aside from that, I actually liked using Device for the macro name in
> > > your earlier versions. Yes, it's just for properties in practice, but
> > > it's nice and small to just say Device; and it mimics Object. It's your
> > > choice anyway.
> >
> > I was thinking of making a `Device` derive macro that lets you also
> > define `DeviceImpl::REALIZE` and `DeviceImpl::vmsd` as macro
> > attributes on the struct definition, then merge DeviceProperties into
> > that. WDYT?
>
> Like #[derive(Device(realize = PL011State::realize))]? I kind of like
> having traits for classes (the "const" does look a bit ugly/foreign,
> but Linux has some other ideas using a #[vtable] procedural macro).
I was thinking:
#[repr(C)]
#[derive(Device)]
#[device(realize = PL011State::realize, vmsd = VMSTATE_PL011)]
pub struct PL011State {
..
}
I agree about traits for class methods, it's definitely cleaner. The
lines blur here because we have REALIZE as a constant in order to make
it nullable from the C side 🤔
> For vmsd yes, you could pass it as a VMStateDescription const's name
> in the same style, like #[derive(Device(vmsd =
> device_class::VMSTATE_PL011))].
>
> WRT naming, I was thinking the other way round: call it Device
> already, and then add functionality without having to change the name.
> Your choice; naming suggestions are risky :) but I wasn't afraid of
> making this suggestion because you had that name before.
>
> > Hm, isn't it redundant if the trait is marked as `unsafe`? Or maybe I
> > misunderstood your point.
>
> Yes, you're right - just wanted something not too tied to properties
> in case the derive macro is extended later. Maybe DeviceDerive, or
> DeviceImplDerive? But any name is fine, just keep it consistent
> between macro and trait.
>
> > > > +/// It is the implementer's responsibility to provide a valid
> > > > [`bindings::PropertyInfo`] pointer
> > > > +/// for the trait implementation to be safe.
> > > > +pub unsafe trait QDevProp {
> > > > + const VALUE: *const bindings::PropertyInfo;
> > >
> > > "*const" or "&"?
> >
> > This is the thing I mentioned to you over IRC: Unfortunately even with
> > const refs for statics we get this because the static is extern:
>
> Ah okay - I just wasn't sure, which is why I wrote it as a question.
> It worked in vmstate because there the const is a VMStateField which
> has a *const inside. So yes, definitely a *const.
>
> Paolo
>
Thanks!
--
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
