Thank you for the review comments Paolo. I will address any bits I did
wrong and not much the rest, it's obvious you have a disagreement over
how things are done and that's fine. This series does not attempt to
solve everything at once and arguing again and again over "this Trait
should have been OtherTrait and this thing should have been thing!()"
is not productive. Your review style of relentless disagreement after
disagreement is tiresome and impossible to deal with; it's like a
denial of service for other human beings. I suggest you take a step
back and take a deep breath before reviewing Rust patches again. I
assure you I will make sure to address all your comments either in
code, TODO comments, or patch messages.
In the meantime, take it easy.
On Sun, Oct 27, 2024 at 10:58 PM Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> Hello,
>
> here is my second attempt to review this, this time placing the remarks
> as close as possible to the code that is affected. However, the meat is
> the same as in my previous replies to the 03/11 thread.
>
> I hope this shows that I have practical concerns about the patch and
> it's not just FUD that it's not acceptable.
>
> On 10/24/24 16:03, Manos Pitsidianakis wrote:
> > Add a new derive procedural macro to declare device models. Add
> > corresponding DeviceImpl trait after already existing ObjectImpl trait.
> > At the same time, add instance_init, instance_post_init,
> > instance_finalize methods to the ObjectImpl trait and call them from the
> > ObjectImplUnsafe trait, which is generated by the procedural macro.
> >
> > This allows all the boilerplate device model registration to be handled
> > by macros, and all pertinent details to be declared through proc macro
> > attributes or trait associated constants and methods.
> >
> > The device class can now be generated automatically and the name can be
> > optionally overridden:
> >
> > ------------------------ >8 ------------------------
> > #[repr(C)]
> > #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)]
> > #[device(class_name_override = PL011Class)]
> > /// PL011 Device Model in QEMU
> > pub struct PL011State {
>
> The first design issue is already visible here in this example. I could
> place the same comment when the code appears in rust/hw/char/pl011, but
> it's easier to do it here.
>
> You have two derive macros, Object and Device. Object is derived by all
> objects (even if right now we have only devices), Device is derived by
> devices only.
>
> The class name is a property of any object, not just devices. It should
> not be part of the #[device()] attribute. #[derive(Device)] and
> #[device()] instead should take care of properties and categories (and
> possibly vmstate, but I'm not sure about that and there's already enough
> to say about this patch).
>
>
> You also have no documentation, which means that users will have no idea
> of what are the other sub-attributes of #[device()], including the
> difference between class_name and class_name_override, or how categories
> are defined.
>
> Even if we don't have support for rustdoc yet in tree, we should have
> all the needed documentation as soon as the API moves from "ad hoc usage
> of C symbols" to idiomatic.
>
> > Object methods (instance_init, etc) methods are now trait methods:
> >
> > ------------------------ >8 ------------------------
> > /// Trait a type must implement to be registered with QEMU.
> > pub trait ObjectImpl {
> > type Class: ClassImpl;
> > const TYPE_NAME: &'static CStr;
> > const PARENT_TYPE_NAME: Option<&'static CStr>;
> > const ABSTRACT: bool;
>
>
> Class, TYPE_NAME, PARENT_TYPE_NAME, ABSTRACT should be defined via
> #[object()].
>
> But actually, there is already room for defining a separate trait:
>
> /// # Safety
> ///
> /// - the first field of the struct must be of `Object` type,
> /// or derived from it
> ///
> /// - `TYPE` must match the type name used in the `TypeInfo` (no matter
> /// if it is defined in C or Rust).
> ///
> /// - the struct must be `#[repr(C)]`
> pub unsafe trait ObjectType {
> type Class: ClassImpl;
> const TYPE_NAME: &'static CStr;
> }
>
> ... because you can implement it even for classes that are defined in C
> code. Then #[derive(Object)] can find the TYPE_NAME directly from the
> first field of the struct, i.e.
>
> parent_obj: SysBusDevice;
>
> becomes
>
> const PARENT_TYPE_NAME: Option<&'static CStr> =
> Some(<SysBusDevice as TypeImpl>::TYPE_NAME);
>
> while #[object()] would be just
>
> #[object(class_type = PL011Class, type_name = "pl011")]
>
> Accessing the type of the first field is easy using the get_fields()
> function that Junjie added at
> https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonz...@redhat.com/
>
> This shows another reason why I prefer to get CI to work first. Having
> to do simple, but still non-trivial work, often provides code that can
> be reused in more complex setups.
>
> > unsafe fn instance_init(&mut self) {}
> > fn instance_post_init(&mut self) {}
> > fn instance_finalize(&mut self) {}
> > }
>
> In the trait, having a default implementation that is empty works
> (unlike for realize/reset, as we'll see later). So this is a bit
> simpler. However, instance_finalize should have a non-empty default
> implementation:
>
> std::ptr::drop_in_place(self);
>
> which should be okay for most devices.
>
> Alternatively, leave out instance_post_init() and instance_finalize()
> until we need them, and put the drop_in_place() call directly in the
> unsafe function that goes in the TypeInfo.
>
> > ------------------------ >8 ------------------------
> >
> > Device methods (realize/reset etc) are now safe idiomatic trait methods:
> >
> > ------------------------ >8 ------------------------
> > /// Implementation methods for device types.
> > pub trait DeviceImpl: ObjectImpl {
> > fn realize(&mut self) {}
> > fn reset(&mut self) {}
> > }
> > ------------------------ >8 ------------------------
>
> This is an incorrect definition of the trait. The default definition of
> device methods is not "empty", it's "just reuse the superclass
> implementation". In particular, this means that PL011LuminaryState
> right now cannot use #[derive(Device)].
>
> > The derive Device macro is responsible for creating all the extern "C" FFI
> > functions that QEMU needs to call these methods.
>
> This is unnecessary. It is perfectly possible to write the extern "C"
> functions (class_init, realize, reset) just once as either type-generic
> functions, or functions in a trait. More on this later.
>
> > diff --git a/rust/qemu-api/src/objects.rs b/rust/qemu-api/src/objects.rs
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..5c6762023ed6914f9c6b7dd16a5e07f778c2d4fa
> > --- /dev/null
> > +++ b/rust/qemu-api/src/objects.rs
> > @@ -0,0 +1,90 @@
> > +// Copyright 2024, Linaro Limited
> > +// Author(s): Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +//! Implementation traits for QEMU objects, devices.
> > +
> > +use ::core::ffi::{c_int, c_void, CStr};
> > +
> > +use crate::bindings::{DeviceState, Error, MigrationPriority, Object,
> > ObjectClass, TypeInfo};
> > +
> > +/// Trait a type must implement to be registered with QEMU.
> > +pub trait ObjectImpl {
> > + type Class: ClassImpl;
> > + const TYPE_NAME: &'static CStr;
> > + const PARENT_TYPE_NAME: Option<&'static CStr>;
> > + const ABSTRACT: bool;
>
> These consts should entirely be derived from the #[object()] attribute.
> You can facilitate the split by having two traits, one for things
> derived from the attribute (the above four), and one for the vtable.
>
> > + unsafe fn instance_init(&mut self) {}
> > + fn instance_post_init(&mut self) {}
> > + fn instance_finalize(&mut self) {}
> > +}
>
> See above remark on the default implementation of instance_finalize.
>
> > +/// The `extern`/`unsafe` analogue of [`ObjectImpl`]; it is used
> > internally by `#[derive(Object)]`
> > +/// and should not be implemented manually.
> > +pub unsafe trait ObjectImplUnsafe {
> > + const TYPE_INFO: TypeInfo;
> > +
> > + const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>;
> > + const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut
> > Object)>;
> > + const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut
> > Object)>;
> > +}
> > +
>
> This trait is not needed at all, because it really has juts one
> implementation. The fact that there is just one implementation is
> hidden by the fact that you are generating the code instead of relying
> on the type system.
>
> All you need is a single function, which will be called via the
> module_init mechanism:
>
> fn rust_type_register<T: ObjectImpl>() {
> let TypeInfo ti = ...;
> unsafe { type_register(&ti); }
> }
>
> >
> > +/// Methods for QOM class types.
> > +pub trait ClassImpl {
> > + type Object: ObjectImpl;
> > +
> > + unsafe fn class_init(&mut self, _data: *mut core::ffi::c_void) {}
> > + unsafe fn class_base_init(&mut self, _data: *mut core::ffi::c_void) {}
> > +}
> > +
>
> This trait (or more precisely class_init and class_base_init) is not
> needed. class_base_init is only needed in very special cases, we can
> just decide they won't be available in Rust for now and possible for ever.
>
> As to class_init device XYZ would only need a non-empty class_init
> method if we added support for the _data argument. But then we would
> need a way to provide the type of _data, and to cast _data to the
> appropriate type; we would also need a way to provide a mapping from
> multiple data objects to multiple type names, which is hard to do
> because right now each Rust struct has a single type name associated.
>
> So, let's just keep only the auto-generated class_init for simplicity.
> If we can just decide that, if device XYZ has superclass FooDevice, it
> implements FooDeviceImpl and class_init is provided by the FooDevice
> bindings.
>
> I can't really say if the "type Object" part is needed. I couldn't
> offhand find anything that uses it, but I may have missed it. If so, it
> can be in ClassImplUnsafe.
>
> > +/// The `extern`/`unsafe` analogue of [`ClassImpl`]; it is used internally
> > by `#[derive(Object)]`
> > +/// and should not be implemented manually.
> > +pub unsafe trait ClassImplUnsafe {
> > + const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass,
> > data: *mut c_void)>;
> > + const CLASS_BASE_INIT: Option<
> > + unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void),
> > + >;
> > +}
>
> Again, no need to have CLASS_BASE_INIT here.
>
> > +/// Implementation methods for device types.
> > +pub trait DeviceImpl: ObjectImpl {
> > + fn realize(&mut self) {}
> > + fn reset(&mut self) {}
> > +}
>
> These unfortunately cannot be functions. Doing so forces the class_init
> method to assign both dc->reset and dc->realize for _all_ classes,
> whereas for example PL011LuminaryClass would not override either.
>
> Therefore, the definition must be
>
> pub trait DeviceImpl: ObjectImpl {
> const REALIZE: Option<fn realize(&mut self)> = None;
> const RESET: Option<fn realize(&mut self)> = None;
> }
>
> Yes, it's uglier, but we cannot escape the fact that we're implementing
> something that Rust doesn't have natively (inheritance). :( So we
> cannot use language features meant for a completely different kind of
> polymorphism.
>
> > +/// The `extern`/`unsafe` analogue of [`DeviceImpl`]; it is used
> > internally by `#[derive(Device)]`
> > +/// and should not be implemented manually.
> > +pub unsafe trait DeviceImplUnsafe {
> > + const REALIZE: Option<unsafe extern "C" fn(dev: *mut DeviceState,
> > _errp: *mut *mut Error)>;
> > + const RESET: Option<unsafe extern "C" fn(dev: *mut DeviceState)>;
> > +}
>
> This trait is also unnecessary, because all that you need is a single
> function:
>
> fn rust_device_class_init<T: DeviceImpl>(
> klass: *mut ObjectClass, _data: *mut c_void)
>
> defined outside the procedural macro. #[derive(Device)] can define
> ClassImplUnsafe to point CLASS_INIT to rust_device_class_init.
>
> (Later, rust_device_class_init() can be moved into a trait so that it's
> possible to define other classes of devices, for example PCI devices.
> Note that such an extension would be much easier, than if it was
> _required_ to touch the procedural macro).
>
> >
> > +/// Constant metadata and implementation methods for types with device
> > migration state.
> > +pub trait Migrateable: DeviceImplUnsafe {
> > + const NAME: Option<&'static CStr> = None;
> > + const UNMIGRATABLE: bool = true;
> > + const EARLY_SETUP: bool = false;
> > + const VERSION_ID: c_int = 1;
> > + const MINIMUM_VERSION_ID: c_int = 1;
> > + const PRIORITY: MigrationPriority = MigrationPriority::MIG_PRI_DEFAULT;
> > +
> > + unsafe fn pre_load(&mut self) -> c_int {
> > + 0
> > + }
> > + unsafe fn post_load(&mut self, _version_id: c_int) -> c_int {
> > + 0
> > + }
> > + unsafe fn pre_save(&mut self) -> c_int {
> > + 0
> > + }
> > + unsafe fn post_save(&mut self) -> c_int {
> > + 0
> > + }
> > + unsafe fn needed(&mut self) -> bool {
> > + false
> > + }
> > + unsafe fn dev_unplug_pending(&mut self) -> bool {
> > + false
> > + }
> > +}
>
> Premature. No need to add this trait until you add support for migration.
>
> > diff --git a/rust/qemu-api/src/tests.rs b/rust/qemu-api/src/tests.rs
> > deleted file mode 100644
> > index
> > df54edbd4e27e7d2aafc243355d1826d52497c21..0000000000000000000000000000000000000000
> > --- a/rust/qemu-api/src/tests.rs
> > +++ /dev/null
> > @@ -1,49 +0,0 @@
>
> Nope. Fix the test, don't remove it.
>
>
> > -#[derive(Debug, qemu_api_macros::Object)]
> > +#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)]
> > +#[device(class_name_override = PL011Class)]
> > /// PL011 Device Model in QEMU
> > pub struct PL011State {
> > pub parent_obj: SysBusDevice,
> > @@ -51,6 +52,7 @@ pub struct PL011State {
> > pub read_count: usize,
> > pub read_trigger: usize,
> > #[doc(alias = "chr")]
> > + #[property(name = c"chardev", qdev_prop = qdev_prop_chr)]
>
> (See earlier comments on accepting only a LitStr and deriving qdev_prop
> from the type).
>
> > +impl DeviceImpl for PL011State {
> > + fn realize(&mut self) {
> > + ...
> > + }
> > +
> > + fn reset(&mut self) {
> > + ...
> > + }
>
> This extractions of code into DeviceImpl is good. However, as I said
> above, I'm not sure about the trait itself. I'll remark later when I
> encounter the definition.
>
> > +impl qemu_api::objects::Migrateable for PL011State {}
>
> Premature.
>
> Before moving on to the procedural macro code, my proposal to split the
> patches is:
>
> 1) introduce the trait ObjectType, define it for Object, DeviceState and
> SysBusDevice.
>
> 2) introduce the traits ObjectImpl, DeviceImpl and ClassImplUnsafe.
> Define the first two for PL011State.
>
> 3) add to common code the wrappers that call into DeviceImpl, removing
> them from PL011State
>
> 4) introduce the functions rust_type_register and rust_device_class_init
> that use the traits.
>
> 5) remove most arguments of device_class_init!(), using the
> infrastructure introduced in the previous two steps
>
> 6) split ObjectImpl into a part that will be covered by #[object()],
> let's call it ObjectInfo
>
> 7) add implementation of #[object()], replace PL011State's
> implementation of ObjectInfo with #[object()]
>
> 8) split DeviceImpl into a part that will be covered by #[device()]
> (properties and categories), let's call it DeviceInfo
>
> 9) add #[derive(Device) and implementation of #[device()], replace
> PL011State's implementation of DeviceInfo with #[device()]
>
> Where 1-5 should be submitted as a separate series, one that does not
> touch procedural macros *at all* and just generalizes the pl011 code
> that defines QOM types.
>
>
> Anyhow, I'll continue reviewing the procedural macro code.
>
> > +#[derive(Debug, Default)]
> > +struct DeriveContainer {
> > + category: Option<syn::Path>,
> > + class_name: Option<syn::Ident>,
> > + class_name_override: Option<syn::Ident>,
> > +}
>
> Rename to DeviceAttribute.
>
> > +impl Parse for DeriveContainer {
> > + fn parse(input: ParseStream) -> Result<Self> {
>
> syn::Result represents a parse error, not an error in the allowed syntax
> of the attribute. Below, you're using panic! and unwrap(), but probably
> instead of syn::Result we need to have something like
>
> pub enum Error {
> CompileError(syn::Span, String),
> ParseError(syn::Error)
> }
>
> which extends the CompileError enum of
> https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonz...@redhat.com/
> and is amenable to use with "?". In particular, note the idiom used by
> the root derive_offsets() functions:
>
> let input = parse_macro_input!(input as DeriveInput);
> let expanded = derive_offsets_or_error(input).
> unwrap_or_else(Into::into);
>
> TokenStream::from(expanded)
>
> which works via an "impl From<CompileError> for proc_macro2::TokenStream".
>
> I believe that most of the benefit of this series (basically, all except
> the #[property] attribute) can be obtained without the procedural macro.
> Therefore, once we do add the procedural macro, we should not have it
> panic on errors.
>
> > + let _: syn::Token![#] = input.parse()?;
> > + let bracketed;
> > + _ = syn::bracketed!(bracketed in input);
> > + assert_eq!(DEVICE, bracketed.parse::<syn::Ident>()?);
> > + let mut retval = Self {
> > + category: None,
> > + class_name: None,
> > + class_name_override: None,
> > + };
> > + let content;
> > + _ = syn::parenthesized!(content in bracketed);
> > + while !content.is_empty() {
> > + let value: syn::Ident = content.parse()?;
> > + if value == CLASS_NAME {
> > + let _: syn::Token![=] = content.parse()?;
> > + if retval.class_name.is_some() {
> > + panic!("{} can only be used at most once", CLASS_NAME);
> > + }
>
> No panic!, instead we need to return a compile_error!() TokenStream, or
> as above a Result<> that can be converted to compile_error!() up in the
> chain.
>
> > + retval.class_name = Some(content.parse()?);
>
> Please make this function a separate trait in utilities:
>
> trait AttributeParsing {
> const NAME: Symbol;
> fn set(&mut self, key: &syn::Ident, input: &ParseStream) -> Result<()>;
> fn parse(input: ParseStream) -> Result<Self> { ... }
> }
>
> Then the "if" can move to the struct-specific implementation of
> AttributeParsing::set, while the rest can move to the default
> implementation of AttributeParsing::parse.
>
> #[property()] and #[device()] (and also the proposed #[object()]) can
> then share the implementation of AttributeParsing::parse.
>
> > + } else if value == CLASS_NAME_OVERRIDE {
> > + let _: syn::Token![=] = content.parse()?;
> > + if retval.class_name_override.is_some() {
> > + panic!("{} can only be used at most once",
> > CLASS_NAME_OVERRIDE);
> > + }> + retval.class_name_override =
> > Some(content.parse()?);
> > + } else if value == CATEGORY {
> > + let _: syn::Token![=] = content.parse()?;
> > + if retval.category.is_some() {
> > + panic!("{} can only be used at most once", CATEGORY);
> > + }
> > + let lit: syn::LitStr = content.parse()?;
> > + let path: syn::Path = lit.parse()?;
>
> Do I understand that this would be
>
> category = "foo::bar::Baz"
>
> ? If so, why the extra quotes? There can actually be more than one
> category, so at least add a TODO here.
>
> > +#[derive(Debug)]
> > +struct QdevProperty {
> > + name: Option<syn::LitCStr>,
>
> Just LitStr. Convert it to CString in the macro. You can reuse the
> c_str!() macro that I'm adding in the series to fix CI and support old
> rustc, i.e. quote! { ::qemu_api::c_str!(#name) } or something like that.
>
> > + qdev_prop: Option<syn::Path>,
> > +}
> > +
> > +impl Parse for QdevProperty {
> > + fn parse(input: ParseStream) -> Result<Self> {
> > + let _: syn::Token![#] = input.parse()?;
> > + let bracketed;
> > + _ = syn::bracketed!(bracketed in input);
> > + assert_eq!(PROPERTY, bracketed.parse::<syn::Ident>()?);
> > + let mut retval = Self {
> > + name: None,
> > + qdev_prop: None,
> > + };
> > + let content;
> > + _ = syn::parenthesized!(content in bracketed);
> > + while !content.is_empty() {
> > + let value: syn::Ident = content.parse()?;
> > + if value == NAME {
> > + let _: syn::Token![=] = content.parse()?;
> > + if retval.name.is_some() {
> > + panic!("{} can only be used at most once", NAME);
> > + }
> > + retval.name = Some(content.parse()?);
> > + } else if value == QDEV_PROP {
> > + let _: syn::Token![=] = content.parse()?;
> > + if retval.qdev_prop.is_some() {
> > + panic!("{} can only be used at most once", QDEV_PROP);
> > + }
> > + retval.qdev_prop = Some(content.parse()?);
> > + } else {
> > + panic!("unrecognized token `{}`", value);
> > + }
> > +
> > + if !content.is_empty() {
> > + let _: syn::Token![,] = content.parse()?;
> > + }
> > + }
> > + Ok(retval)
>
> See above with respect to the duplicated code with #[device()].
>
> > + let derive_container: DeriveContainer = input
> > + .attrs
> > + .iter()
> > + .find(|a| a.path() == DEVICE)
> > + .map(|a| syn::parse(a.to_token_stream().into()).expect("could not
> > parse device attr"))
> > + .unwrap_or_default();
> > + let (qdev_properties_static, qdev_properties_expanded) =
> > make_qdev_properties(&input);
>
> Please put functions before their callers.
>
> > + let realize_fn = format_ident!("__{}_realize_generated", name);
> > + let reset_fn = format_ident!("__{}_reset_generated", name);
> > +
> > + let expanded = quote! {
> > + unsafe impl ::qemu_api::objects::DeviceImplUnsafe for #name {
> > + const REALIZE: ::core::option::Option<
> > + unsafe extern "C" fn(
> > + dev: *mut ::qemu_api::bindings::DeviceState,
> > + errp: *mut *mut ::qemu_api::bindings::Error,
> > + ),
> > + > = Some(#realize_fn);
> > + const RESET: ::core::option::Option<
> > + unsafe extern "C" fn(dev: *mut
> > ::qemu_api::bindings::DeviceState),
> > + > = Some(#reset_fn);
> > + }
> > +
> > + #[no_mangle]
>
> Not needed.
>
> > + pub unsafe extern "C" fn #realize_fn(
> > + dev: *mut ::qemu_api::bindings::DeviceState,
> > + errp: *mut *mut ::qemu_api::bindings::Error,
> > + ) {
> > + let mut instance =
> > NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a
> > non-null pointer of type ", stringify!(#name)));
> > + unsafe {
> > +
> > ::qemu_api::objects::DeviceImpl::realize(instance.as_mut());
> > + }
> > + }
> > +
> > + #[no_mangle]
>
> Not needed.
>
> > + pub unsafe extern "C" fn #reset_fn(
> > + dev: *mut ::qemu_api::bindings::DeviceState,
> > + ) {
> > + let mut instance =
> > NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a
> > non-null pointer of type ", stringify!(#name)));
> > + unsafe {
> > + ::qemu_api::objects::DeviceImpl::reset(instance.as_mut());
> > + }
> > + }
>
> All this code depends on nothing but #name. This is not the C
> preprocessor; the way to do it in Rust is monomorphization as described
> above.
>
> > +fn gen_device_class(
> > + derive_container: DeriveContainer,
> > + qdev_properties_static: syn::Ident,
> > + name: &syn::Ident,
> > +) -> proc_macro2::TokenStream {
> > + let (class_name, class_def) = match (
> > + derive_container.class_name_override,
> > + derive_container.class_name,
> > + ) {
> > + (Some(class_name), _) => {
> > + let class_expanded = quote! {
> > + #[repr(C)]
> > + pub struct #class_name {
> > + _inner: [u8; 0],
> > + }
> > + };
> > + (class_name, class_expanded)
> > + }
> > + (None, Some(class_name)) => (class_name, quote! {}),
> > + (None, None) => {
> > + let class_name = format_ident!("{}Class", name);
> > + let class_expanded = quote! {
> > + #[repr(C)]
> > + pub struct #class_name {
> > + _inner: [u8; 0],
> > + }
>
> This should have a DeviceClass member, it should not be a dummy 0-byte type.
>
> Also, this should be generated by #[derive(Object)].
>
> > + };
> > + (class_name, class_expanded)
> > + }
> > + };
> > + let class_init_fn = format_ident!("__{}_class_init_generated",
> > class_name);
> > + let class_base_init_fn =
> > format_ident!("__{}_class_base_init_generated", class_name);
> > +
> > + let (vmsd, vmsd_impl) = {
> > + let (i, vmsd) = make_vmstate(name);
> > + (quote! { &#i }, vmsd)
> > + };
> > + let category = if let Some(category) = derive_container.category {
> > + quote! {
> > + const BITS_PER_LONG: u32 = ::core::ffi::c_ulong::BITS;
> > + let _: ::qemu_api::bindings::DeviceCategory = #category;
> > + let nr: ::core::ffi::c_ulong = #category as _;
> > + let mask = 1 << (nr as u32 % BITS_PER_LONG);
> > + let p =
> > ::core::ptr::addr_of_mut!(dc.as_mut().categories).offset((nr as u32 /
> > BITS_PER_LONG) as isize);
> > + let p: *mut ::core::ffi::c_ulong = p.cast();
> > + let categories = p.read_unaligned();
> > + p.write_unaligned(categories | mask);
>
> What's wrong with
>
> const BITS_PER_ELEMENT: u32 =
> ::core::mem::sizeof(dc.categories) /
> dc.categories.len() * 8;
>
> dc.categories[((nr as u32) / BITS_PER_ELEMENT) as usize]
> |= 1 << ((nr as u32) % BITS_PER_ELEMENT);
>
> ?
>
> > + #[no_mangle]
> > + pub unsafe extern "C" fn #class_init_fn(klass: *mut ObjectClass,
> > data: *mut core::ffi::c_void) {
> > + {
> > + {
> > + let mut dc =
> > +
> > ::core::ptr::NonNull::new(klass.cast::<::qemu_api::bindings::DeviceClass>()).unwrap();
>
> And then "let mut dc = dc.as_mut()". Just write the conversion once.
>
> > + unsafe {
> > + dc.as_mut().realize =
> > + <#name as
> > ::qemu_api::objects::DeviceImplUnsafe>::REALIZE;
> > +
> > ::qemu_api::bindings::device_class_set_legacy_reset(
> > + dc.as_mut(),
> > + <#name as
> > ::qemu_api::objects::DeviceImplUnsafe>::RESET
> > + );
>
> As written elsewhere, these should be conditional.
>
> > + dc.as_mut().vmsd = #vmsd;
> > + #props
> > + #category
> > + }> + }
>
> All this code should be outside the macro, and should use trait consts
> instead of quoting.
>
> > + let mut klass =
> > NonNull::new(klass.cast::<#class_name>()).expect(concat!("Expected klass to
> > be a non-null pointer of type ", stringify!(#class_name)));
> > + unsafe {
> > +
> > ::qemu_api::objects::ClassImpl::class_init(klass.as_mut(), data);
> > + }
> > + }
> > + }
> > + #[no_mangle]
> > + pub unsafe extern "C" fn #class_base_init_fn(klass: *mut
> > ObjectClass, data: *mut core::ffi::c_void) {
> > + {
> > + let mut klass =
> > NonNull::new(klass.cast::<#class_name>()).expect(concat!("Expected klass to
> > be a non-null pointer of type ", stringify!(#class_name)));
> > + unsafe {
> > +
> > ::qemu_api::objects::ClassImpl::class_base_init(klass.as_mut(), data);
> > + }
> > + }
> > + }
> > +
> > + #vmsd_impl
> > + }
> > +}
> > +
> > +fn make_vmstate(name: &syn::Ident) -> (syn::Ident,
> > proc_macro2::TokenStream) {
>
> Not needed. Just let the user provide a VMStateDescription in
> DeviceImpl. I'm not sure if it's possible to make it a const; if not,
> it can be a function returning a &'static VMStateDescription.
>
> > + let vmstate_description_ident = format_ident!("__VMSTATE_{}", name);
> > +
> > + let pre_load = format_ident!("__{}_pre_load_generated", name);
> > + let post_load = format_ident!("__{}_post_load_generated", name);
> > + let pre_save = format_ident!("__{}_pre_save_generated", name);
> > + let post_save = format_ident!("__{}_post_save_generated", name);
> > + let needed = format_ident!("__{}_needed_generated", name);
> > + let dev_unplug_pending =
> > format_ident!("__{}_dev_unplug_pending_generated", name);
> > +
> > + let migrateable_fish = quote! {<#name as
> > ::qemu_api::objects::Migrateable>};
> > + let vmstate_description = quote! {
> > + #[used]
>
> Attribute not needed.
>
> > + #[allow(non_upper_case_globals)]
> > + pub static #vmstate_description_ident:
> > ::qemu_api::bindings::VMStateDescription =
> > ::qemu_api::bindings::VMStateDescription {
> > + name: if let Some(name) = #migrateable_fish::NAME {
> > + name.as_ptr()
> > + } else {
> > + <#name as
> > ::qemu_api::objects::ObjectImplUnsafe>::TYPE_INFO.name
> > + },
> > + unmigratable: #migrateable_fish::UNMIGRATABLE,
> > + early_setup: #migrateable_fish::EARLY_SETUP,
> > + version_id: #migrateable_fish::VERSION_ID,
> > + minimum_version_id: #migrateable_fish::MINIMUM_VERSION_ID,
> > + priority: #migrateable_fish::PRIORITY,
> > + pre_load: Some(#pre_load),
> > + post_load: Some(#post_load),
> > + pre_save: Some(#pre_save),
> > + post_save: Some(#post_save),
> > + needed: Some(#needed),
> > + dev_unplug_pending: Some(#dev_unplug_pending),
> > + fields: ::core::ptr::null(),
> > + subsections: ::core::ptr::null(),
> > + };
> > +
> > + #[no_mangle]
>
> Not needed (other occurrences below).
>
> > + pub unsafe extern "C" fn #pre_load(opaque: *mut
> > ::core::ffi::c_void) -> ::core::ffi::c_int {
> > + let mut instance =
> > NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be
> > a non-null pointer of type ", stringify!(#name), "::Object"));
> > + unsafe {
> > +
> > ::qemu_api::objects::Migrateable::pre_load(instance.as_mut())
> > + }
> > + }
> > + #[no_mangle]
> > + pub unsafe extern "C" fn #post_load(opaque: *mut
> > ::core::ffi::c_void, version_id: core::ffi::c_int) -> ::core::ffi::c_int {
> > + let mut instance =
> > NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be
> > a non-null pointer of type ", stringify!(#name), "::Object"));
> > + unsafe {
> > +
> > ::qemu_api::objects::Migrateable::post_load(instance.as_mut(), version_id)
>
> Again, introducing the Migrateable code and all these thunks is
> premature; but in any case, this can only return 0 or -1 so make
> Migrateable::post_load() return Result<(), ()>.
>
> > + }
> > + }
> > + #[no_mangle]
> > + pub unsafe extern "C" fn #pre_save(opaque: *mut
> > ::core::ffi::c_void) -> ::core::ffi::c_int {
> > + let mut instance =
> > NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be
> > a non-null pointer of type ", stringify!(#name), "::Object"));
> > + unsafe {
> > +
> > ::qemu_api::objects::Migrateable::pre_save(instance.as_mut())
> > + }
> > + }
>
> Likewise.
>
> > + #[no_mangle]
> > + pub unsafe extern "C" fn #post_save(opaque: *mut
> > ::core::ffi::c_void) -> ::core::ffi::c_int {
> > + let mut instance =
> > NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be
> > a non-null pointer of type ", stringify!(#name), "::Object"));
> > + unsafe {
> > +
> > ::qemu_api::objects::Migrateable::post_save(instance.as_mut())
> > + }
> > + }
>
> Likewise.
>
> > + #[no_mangle]
> > + pub unsafe extern "C" fn #needed(opaque: *mut ::core::ffi::c_void)
> > -> bool {
> > + let mut instance =
> > NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be
> > a non-null pointer of type ", stringify!(#name), "::Object"));
> > + unsafe {
> > + ::qemu_api::objects::Migrateable::needed(instance.as_mut())
> > + }
> > + }
> > + #[no_mangle]
> > + pub unsafe extern "C" fn #dev_unplug_pending(opaque: *mut
> > ::core::ffi::c_void) -> bool {
> > + let mut instance =
> > NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be
> > a non-null pointer of type ", stringify!(#name), "::Object"));
> > + unsafe {
> > +
> > ::qemu_api::objects::Migrateable::dev_unplug_pending(instance.as_mut())
> > + }
> > + }
> > + };
> > +
> > + let expanded = quote! {
> > + #vmstate_description
> > + };
> > + (vmstate_description_ident, expanded)
> > +}
> > diff --git a/rust/qemu-api-macros/src/lib.rs
> > b/rust/qemu-api-macros/src/lib.rs
> > index
> > 59aba592d9ae4c5a4cdfdc6f9b9b08363b8a57e5..7753a853fae72fc87e6dc642cf076c6d0c736345
> > 100644
> > --- a/rust/qemu-api-macros/src/lib.rs
> > +++ b/rust/qemu-api-macros/src/lib.rs
> > @@ -2,42 +2,21 @@
> > // Author(s): Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
> > // SPDX-License-Identifier: GPL-2.0-or-later
> >
> > +#![allow(dead_code)]
>
> Why?
>
> > use proc_macro::TokenStream;
> > -use quote::{format_ident, quote};
> > -use syn::{parse_macro_input, DeriveInput};
> > +
> > +mod device;
> > +mod object;
> > +mod symbols;
> > +mod utilities;
> >
> > #[proc_macro_derive(Object)]
> > pub fn derive_object(input: TokenStream) -> TokenStream {
> > - let input = parse_macro_input!(input as DeriveInput);
> > -
> > - let name = input.ident;
> > - let module_static = format_ident!("__{}_LOAD_MODULE", name);
> > -
> > - let expanded = quote! {
> > - #[allow(non_upper_case_globals)]
> > - #[used]
> > - #[cfg_attr(target_os = "linux", link_section = ".ctors")]
> > - #[cfg_attr(target_os = "macos", link_section =
> > "__DATA,__mod_init_func")]
> > - #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
> > - pub static #module_static: extern "C" fn() = {
> > - extern "C" fn __register() {
> > - unsafe {
> > - ::qemu_api::bindings::type_register_static(&<#name as
> > ::qemu_api::definitions::ObjectImpl>::TYPE_INFO);
> > - }
> > - }
> > -
> > - extern "C" fn __load() {
> > - unsafe {
> > - ::qemu_api::bindings::register_module_init(
> > - Some(__register),
> > -
> > ::qemu_api::bindings::module_init_type::MODULE_INIT_QOM
> > - );
> > - }
> > - }
> > -
> > - __load
> > - };
> > - };
> > + object::derive_object(input)
>
> Moving code to a separate file should be a separate patch from modifying
> the expansion of the macro.
>
> > diff --git a/rust/qemu-api-macros/src/symbols.rs
> > b/rust/qemu-api-macros/src/symbols.rs
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..f73768d228ed2b4d478c18336db56cb11e70f012
> > --- /dev/null
> > +++ b/rust/qemu-api-macros/src/symbols.rs
> > @@ -0,0 +1,55 @@
> > +// Copyright 2024, Linaro Limited
> > +// Author(s): Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +use core::fmt;
> > +use syn::{Ident, Path};
> > +
> > +#[derive(Copy, Clone, Debug)]
> > +pub struct Symbol(&'static str);
> > +
> > +pub const DEVICE: Symbol = Symbol("device");
> > +pub const NAME: Symbol = Symbol("name");
> > +pub const CATEGORY: Symbol = Symbol("category");
> > +pub const CLASS_NAME: Symbol = Symbol("class_name");
> > +pub const CLASS_NAME_OVERRIDE: Symbol = Symbol("class_name_override");
> > +pub const QDEV_PROP: Symbol = Symbol("qdev_prop");
> > +pub const MIGRATEABLE: Symbol = Symbol("migrateable");
> > +pub const PROPERTIES: Symbol = Symbol("properties");
> > +pub const PROPERTY: Symbol = Symbol("property");
>
> Declare these in device.rs as needed, not here. This avoids "use
> symbols::*". It also allows making them not "pub", so that dead ones
> are detected by the compiler (e.g. MIGRATEABLE, PROPERTIES).
>
> > +pub fn assert_is_repr_c_struct(input: &DeriveInput, derive_macro: &'static
> > str) {
>
> Nice but a bit overengineered. Unless you think/know that you'll have a
> use for Repr elsewhere, try sharing code with Junjie's macro
> https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonz...@redhat.com/.
>
> Paolo
>
