Thanks for the comments, I am preparing a new version with all
problems/suggestions fixed.
On Tue, Jul 8, 2025 at 12:48 PM Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> On 7/3/25 16:37, Manos Pitsidianakis wrote:
> > Add derive macro for declaring qdev properties directly above the field
> > definitions. To do this, we split DeviceImpl::properties method on a
> > separate trait so we can implement only that part in the derive macro
> > expansion (we cannot partially implement the DeviceImpl trait).
> >
> > Adding a `property` attribute above the field declaration will generate
> > a `qemu_api::bindings::Property` array member in the device's property
> > list.
> >
> > Signed-off-by: Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
>
> Very nice. I have relatively many comments but they are all very
> simple. The main ones are about unused functionality that could be left
> for later, and some code duplication.
>
> 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?
>
> > - Update hpet code to use the derive macro
>
> Don't worry about that for the first inclusion. More below.
>
> > - Change MacroError use to syn::Error use if changed in upstream too
>
> True. In the review below I'll just use syn::Error instead of MacroError.
>
> > +impl DevicePropertiesImpl for PL011Luminary {}
>
> Does it make sense to use #[derive()] anyway, and skip the import?
> Especially because...
Oops, yes.
>
> > +/// Trait to define device properties.
> > +pub trait DevicePropertiesImpl {
> > + /// An array providing the properties that the user can set on the
> > + /// device. Not a `const` because referencing statics in constants
> > + /// is unstable until Rust 1.83.0.
> > + fn properties() -> &'static [Property] {
> > + &[]
> > + }
> > +}
> > +
>
> ... the trait should be declared unsafe(*), and it's nice to hide the
> implementation of unsafe traits behind macros that do guarantee the safety.
Good idea, will do,
>
> (*) One can also declare properties() unsafe, but with 1.83.0
> properties() becomes an associated const, and there are no
> unsafe const declarations... might as well plan ahead.
>
> And then, what about calling the trait "DeviceImplUnsafe"? It is a
> clearer split: device code uses #[derive()] for anything that cannot be
> declared safely (think of zerocopy), and impl for what *is* safe.
Hm, isn't it redundant if the trait is marked as `unsafe`? Or maybe I
misunderstood your point.
>
> > + } else if value == "qdev_prop" {
> > + let _: syn::Token![=] = content.parse()?;
> > + if retval.qdev_prop.is_some() {
> > + return Err(syn::Error::new(
> > + value.span(),
> > + "`qdev_prop` can only be used at most once",
> > + ));
> > + }
> > + retval.qdev_prop = Some(content.parse()?);
>
> qdev_prop is only needed together with bitnr, right? If so:
>
> 1) Thoughts for later: maybe if bitnr is used the macro should use a
> different trait than QDevProp (e.g. QDevBitProp). Would this be enough
> for qdev_prop to go away? (I think/hope so)
>
> 2) Thoughts for now: maybe leave out bitnr and qdev_prop? And revisit
> together with the HPET conversion, which needs bitnr.
Agreed, that makes more sense
>
> > + let prop_name = rename
> > + .as_ref()
>
> I think ".as_ref()" is not needed? I may be wrong though.
>
> > + DevicePropertyName::Str(lit) => {> +
> > let span = lit.span();
> > + let value = lit.value();
> > + let lit = std::ffi::CString::new(value.as_str())
> > + .map_err(|err| {
> > + MacroError::Message(
> > + format!("Property name `{value}` cannot be
> > represented as a C string: {err}"),
> > + span
> > + )
> > + })?;
> > + let lit = syn::LitCStr::new(&lit, span);
> > + Ok(quote_spanned! {span=>
> > + #lit
> > + })
>
> quote_spanned! is not needed here, because all the tokens that you're
> producing are interpolated:
>
> Any interpolated tokens preserve the Span information provided by
> their ToTokens implementation. Tokens that originate within the
> quote_spanned! invocation are spanned with the given span argument
> (https://docs.rs/quote/1.0.40/quote/macro.quote_spanned.html)
>
> Also please extract this into a separate function. That is, make
> everything just
>
> make_c_str(lit.value(), lit.span())
>
> (make_c_str returns a Result<syn::LitCStr, syn::Error>).
>
> > + .unwrap_or_else(|| {
> > + let span = field_name.span();
> > + let field_name_value = field_name.to_string();
> > + let lit =
> > std::ffi::CString::new(field_name_value.as_str()).map_err(|err| {
> > + MacroError::Message(
> > + format!("Field `{field_name_value}` cannot be
> > represented as a C string: {err}\nPlease set an explicit property name
> > using the `rename=...` option in the field's `property` attribute."),
>
> I don't think this error can happen, because the field name cannot
> contain a NUL character and that's the only way CString::new fails. So
> just using the same function above is fine, because the more detailed
> error isn't necessary.
>
> Putting everything together and using .map_or_else() gives you something
> like this:
>
> let prop_name = rename
> .map_or_else(
> || make_c_str(field_name.to_string(), field_name.span())
> |lit| {
> match lit {
> DevicePropertyName::CStr(lit) => {
> Ok(lit)
> }
> DevicePropertyName::Str(lit) => {
> make_c_str(lit.value(), lit.span())
> }
> }
> })?;
>
> You could even go ahead and only accept syn::LitStr, dropping
> DevicePropertyName altogether. But since you've written the code and
> c"" support is only 10-15 lines of code overall, do as you wish.
>
> > + span
> > + )
> > + })?;
> > + let lit = syn::LitCStr::new(&lit, span);
> > + Ok(quote_spanned! {span=>
> > + #lit
> > + })
> > + })?;
> > + let field_ty = field.ty.clone();
> > + let qdev_prop = qdev_prop
> > + .as_ref()
>
> Again, .as_ref() might not be needed here either.
>
> > + .map(|path| {
> > + quote_spanned! {field_span=>
> > + unsafe { &#path }
> > + }
> > + })
> > + .unwrap_or_else(
> > + || quote_spanned! {field_span=> <#field_ty as
> > ::qemu_api::qdev::QDevProp>::VALUE },
> > + );
>
> If you decide to keep qdev_prop, .map_or_else() can be used here too.
>
> > + let set_default = defval.is_some();
> > + let bitnr = bitnr.as_ref().unwrap_or(&zero);
> > + let defval = defval.as_ref().unwrap_or(&zero);
> > + properties_expanded.push(quote_spanned! {field_span=>
> > + ::qemu_api::bindings::Property {
> > + name: ::std::ffi::CStr::as_ptr(#prop_name),
> > + info: #qdev_prop ,
> > + offset: ::core::mem::offset_of!(#name, #field_name) as
> > isize,
> > + bitnr: #bitnr,
> > + set_default: #set_default,
> > + defval: ::qemu_api::bindings::Property__bindgen_ty_1 { u:
> > #defval as u64 },
>
> Maybe add a TODO that not all types should have a default (e.g.
> pointers). No need to fix it now, but having a note in the code would
> be nice.
>
> > diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
> > index
> > 36f02fb57dbffafb21a2e7cc96419ca42e865269..01f199f198c6a5f8a761beb143e567fc267028aa
> > 100644
> > --- a/rust/qemu-api/src/qdev.rs
> > +++ b/rust/qemu-api/src/qdev.rs
> > @@ -101,8 +101,54 @@ pub trait ResettablePhasesImpl {
> > T::EXIT.unwrap()(unsafe { state.as_ref() }, typ);
> > }
> >
> > +/// Helper trait to return pointer to a [`bindings::PropertyInfo`] for a
> > type.
> > +///
> > +/// This trait is used by [`qemu_api_macros::DeviceProperty`] derive macro.
> > +///
> > +/// # Safety
> > +///
> > +/// This trait is marked as `unsafe` because currently having a `const`
> > refer to an `extern static`
> > +/// results in this compiler error:
> > +///
> > +/// ```text
> > +/// constructing invalid value: encountered reference to `extern` static
> > in `const`
> > +/// ```
> > +///
> > +/// 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:
error[E0080]: it is undefined behavior to use this value
--> build/rust/qemu-api/libqemu_api.rlib.p/structured/qdev.rs:135:5
|
135 | const VALUE: &'static bindings::PropertyInfo = unsafe {
&bindings::qdev_prop_chr };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing
invalid value: encountered reference to `extern` static in `const`
|
= note: The rules on what exactly is undefined behavior aren't
clear, so this check might be overzealous. Please open an issue on the
rustc repository if you believe it should not be considered undefined
behavior.
= note: the raw bytes of the constant (size: 8, align: 8) {
╾────alloc93<imm>─────╼ │ ╾──────╼
}
IIUC the error, it's because const refs can get dereferenced in const
context by the compiler in general, but PropertyInfo does not get
dereferenced anywhere so we are good.
>
> > @@ -68,10 +68,13 @@ impl ObjectImpl for DummyState {
> >
> > impl ResettablePhasesImpl for DummyState {}
> >
> > -impl DeviceImpl for DummyState {
> > +impl DevicePropertiesImpl for DummyState {
> > fn properties() -> &'static [Property] {
> > &DUMMY_PROPERTIES
> > }
> > +}
>
> You can easily use #[derive()] here too, since the DummyState code is
> mostly copied from pl011.
>
> > +impl DevicePropertiesImpl for DummyChildState {}
>
> ... and here too.
>
> Thanks,
>
> Paolo
>
--
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
