[RFC 0/2] rust/qemu-api: Rethink property definition macro

[RFC 0/2] rust/qemu-api: Rethink property definition macro

[Zhao Liu]

Hi all,

This RFC is try to implement the property definition macro in Rust
style, to take advantage of Rust's language features and to eliminate
some of the extra checking burden of the C version.

This RFC would change the way property definitions are defined, making
the Rust version of the interface less like C.

Junjie and I felt that this was a small enough, as well as special
enough case, to get us thinking together about what changes Rust would
bring to the QEMU interface.

Welcome your comments!


Background
==========

Let's start by reviewing the original property definition macro in C
version.

This is the most basic one, "DEFINE_PROP":

#define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) {  \
        .name      = (_name),                                    \
        .info      = &(_prop),                                   \
        .offset    = offsetof(_state, _field)                    \
            + type_check(_type, typeof_field(_state, _field)),   \
        __VA_ARGS__                                              \
        }

This macro, to ensure correctness, needs to consider two places:

 1. "_field" must be the member name of "_state".
   * checked by typeof_field().

 2. "_field", "_type" and "_prop" must be aligned.
   * Type of "field" is checked by type_check().
   * Various variants of the macro bind "_prop" to "_type".

For Rust version macro, "define_property!", performs the first check by
"offset_of!", but the second requirement is not ensured because it
doesn't check if $type matches $field. This makes the current Rust
version more unsafe than the C version. :)


Solution
========

Of course, we can add checks, such as some tricks similar to those in
patch 2 for type inference.

However, manually binding $type and $prop in macros and checking $field
feels like C style, especially since Rust has type inference and trait.

Therefore, we define a trait for types in QEMU that require properties,
and return the PropertyInfo by trait, like:

pub trait PropertyType {
    fn get_prop_info() -> *const PropertyInfo;
}

impl PropertyType for bool {
    fn get_prop_info() -> *const PropertyInfo {
        unsafe { std::ptr::addr_of!(qdev_prop_bool) }
    }
}

(This is a simplified version, and we spend the extra effort to implement
PropertyTypeImpl, to get $field type in a safer way, pls see patch 2 for
details.)

At the same time, using Rust's type inference, the PropertyInfo is
directly obtained in the macro based on the type of $field.

Therefore, from $field, we can derive $type, and thus $prop, so that
these three things can be naturally aligned, and, in turn, we can
eliminate the $type and $prop parameters, from the Rust macro.

Fewer parameter relationships mean fewer errors.

Then for Rust version, user shouldn't encode PropertyInfo in macro
directly, instead he should implement PropertyType trait.


Opens
=====

But because of the change in the way properties are defined, we need to
think about how to support custom PropertyInfo: this happens when QEMU
needs to support different PropertyInfo's for the same type, and Rust
trait can't implement it multiple times for the same type.

There're 2 options:

1. Introduce a new macro to support $prop parameter, and user could
specify their custom PropertyInfo.

This is aligned with original C version and reverts to the current code
prior to modifications.


2. Don't allow the user to override the PropertyInfo of the type. Instead
user can wrap the type in truple struct and then implement trait for the
new truple type. For example,

struct MyBool(bool);

impl PropertyType for MyBool {
    ...
}

This way is more like Rust style, but users have to add ".0" whenever
they want to access the wrapped property value.


In both cases we allow the user to specify the pointer to PropertyInfo.
But there is no way to check if the provided PropertyInfo is consistent
with the property type (afterall, we haven't implemented the property
registration mechanism in Rust yet), and we have to assume the user will
ensure the consistency. So both approaches have the same level of safety.

Which option do you think is better?

Thanks and Best Regards,
Zhao

---
Junjie Mao (1):
  rust/qemu-api: Fix fragment-specifiers in define_property macro

Zhao Liu (1):
  rust/qemu-api: Bind PropertyInfo to type

 rust/hw/char/pl011/src/device_class.rs |  4 ---
 rust/qemu-api/src/device_class.rs      | 48 +++++++++++++++++++++++---
 rust/qemu-api/src/tests.rs             |  4 ---
 3 files changed, 43 insertions(+), 13 deletions(-)

-- 
2.34.1

[RFC 1/2] rust/qemu-api: Fix fragment-specifiers in define_property macro

[Zhao Liu]

From: Junjie Mao <junjie.mao@hotmail.com>

For the matcher of macro, "expr" is used for expressions, while "ident"
is used for variable/function names, and "ty" matches types.

In define_property macro, $field is a member name of type $state, so it
should be defined as "ident", though offset_of! doesn't complain about
this. $type is the type of $field, since it is not used in the macro, so
that no type mismatch error is triggered either.

Fix fragment-specifiers of $field and $type.

Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
Co-developed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 rust/qemu-api/src/device_class.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index 1ea95beb78db..be363fd63223 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -26,7 +26,7 @@ macro_rules! device_class_init {
 
 #[macro_export]
 macro_rules! define_property {
-    ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
+    ($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty, default = $defval:expr$(,)*) => {
         $crate::bindings::Property {
             name: {
                 #[used]
@@ -47,7 +47,7 @@ macro_rules! define_property {
             link_type: ::core::ptr::null(),
         }
     };
-    ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => {
+    ($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty$(,)*) => {
         $crate::bindings::Property {
             name: {
                 #[used]
-- 
2.34.1

[RFC 2/2] rust/qemu-api: Bind PropertyInfo to type

[Zhao Liu]

For traditional property definitions implemented in C, since the C
language cannot bind variable types to specific methods or variables,
user must specify the concrete PropertyInfo and variable type in the
DEFINE_PROP macro and its variants. Then the property macro effectively
associate PropertyInfo with variable type through clever check.

However, in Rust, this process can be more elegant. By introducing the
PropertyType trait, QAPI (in Rust) can associate PropertyInfo with
specific types, allowing Rust to infer the predefined PropertyInfo from
the variable type.

This avoids user errors in passing incorrect PropertyInfo and simplifies
property definitions. And based on this enhancement, the property
definition macro can eliminate the need for the type and prop parameters.

Co-developed-by: Junjie Mao <junjie.mao@hotmail.com>
Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 rust/hw/char/pl011/src/device_class.rs |  4 ---
 rust/qemu-api/src/device_class.rs      | 48 +++++++++++++++++++++++---
 rust/qemu-api/src/tests.rs             |  4 ---
 3 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index b7ab31af02d7..295e970e8564 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -21,15 +21,11 @@
         c"chardev",
         PL011State,
         char_backend,
-        unsafe { &qdev_prop_chr },
-        CharBackend
     ),
     qemu_api::define_property!(
         c"migrate-clk",
         PL011State,
         migrate_clock,
-        unsafe { &qdev_prop_bool },
-        bool
     ),
 }
 
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index be363fd63223..0b9d6ca705d3 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -4,7 +4,7 @@
 
 use std::sync::OnceLock;
 
-use crate::bindings::Property;
+use crate::bindings::{CharBackend, Property, PropertyInfo, qdev_prop_chr, qdev_prop_bool};
 
 #[macro_export]
 macro_rules! device_class_init {
@@ -24,16 +24,54 @@ macro_rules! device_class_init {
     };
 }
 
+pub trait PropertyType {
+    fn get_prop_info() -> *const PropertyInfo;
+}
+
+pub trait PropertyTypeImpl<T: PropertyType> {
+    fn get_prop_info(_: *const T) -> *const PropertyInfo;
+}
+
+impl<T: PropertyType> PropertyTypeImpl<T> for () {
+    fn get_prop_info(_: *const T) -> *const PropertyInfo {
+        T::get_prop_info()
+    }
+}
+
+impl PropertyType for CharBackend {
+    fn get_prop_info() -> *const PropertyInfo {
+        // SAFETY: Access to a defined c-structure, no other operation is performed.
+        unsafe { std::ptr::addr_of!(qdev_prop_chr) }
+    }
+}
+
+impl PropertyType for bool {
+    fn get_prop_info() -> *const PropertyInfo {
+        // SAFETY: Access to a defined c-structure, no other operation is performed.
+        unsafe { std::ptr::addr_of!(qdev_prop_bool) }
+    }
+}
+
+#[macro_export]
+macro_rules! get_prop_info {
+    ($state:ty, $field:ident) => {{
+    use $crate::device_class::PropertyTypeImpl;
+    let base = std::mem::MaybeUninit::<$state>::uninit().as_ptr();
+    <()>::get_prop_info(unsafe { std::ptr::addr_of!((*base).$field) })
+    }};
+}
+
+
 #[macro_export]
 macro_rules! define_property {
-    ($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty, default = $defval:expr$(,)*) => {
+    ($name:expr, $state:ty, $field:ident, default = $defval:expr$(,)*) => {
         $crate::bindings::Property {
             name: {
                 #[used]
                 static _TEMP: &::core::ffi::CStr = $name;
                 _TEMP.as_ptr()
             },
-            info: $prop,
+            info: $crate::get_prop_info!($state, $field),
             offset: ::core::mem::offset_of!($state, $field)
                 .try_into()
                 .expect("Could not fit offset value to type"),
@@ -47,14 +85,14 @@ macro_rules! define_property {
             link_type: ::core::ptr::null(),
         }
     };
-    ($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty$(,)*) => {
+    ($name:expr, $state:ty, $field:ident$(,)*) => {
         $crate::bindings::Property {
             name: {
                 #[used]
                 static _TEMP: &::core::ffi::CStr = $name;
                 _TEMP.as_ptr()
             },
-            info: $prop,
+            info: $crate::get_prop_info!($state, $field),
             offset: ::core::mem::offset_of!($state, $field)
                 .try_into()
                 .expect("Could not fit offset value to type"),
diff --git a/rust/qemu-api/src/tests.rs b/rust/qemu-api/src/tests.rs
index df54edbd4e27..d2b7451ef707 100644
--- a/rust/qemu-api/src/tests.rs
+++ b/rust/qemu-api/src/tests.rs
@@ -27,15 +27,11 @@ pub struct DummyState {
                 c"chardev",
                 DummyState,
                 char_backend,
-                unsafe { &qdev_prop_chr },
-                CharBackend
             ),
             define_property!(
                 c"migrate-clk",
                 DummyState,
                 migrate_clock,
-                unsafe { &qdev_prop_bool },
-                bool
             ),
     }
 
-- 
2.34.1