From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Among the QEMU developers, there is a desire to use Rust. (see previous
thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm
related projects and other experiments).

Thanks to our QAPI type system and the associate code generator, it is
relatively straightforward to create Rust bindings for the generated C
types (also called sys/ffi binding) and functions. (rust-bindgen could
probably do a similar job, but it would probably bring other issues).
This provides an important internal API already.

Slightly more complicated is to expose a Rust API for those, and provide
convenient conversions C<->Rust. Taking inspiration from glib-rs
binding, I implemented a simplified version of the FromGlib/ToGlib
traits, with simpler ownership model, sufficient for QAPI needs.

The usage is relatively simple:

- from_qemu_none(ptr: *const sys::P) -> T
  Return a Rust type T for a const ffi pointer P.

- from_qemu_full(ptr: *mut sys::P) -> T
  Return a Rust type T for a ffi pointer P, taking ownership.

- T::to_qemu_none() -> Stash<P>
  Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
  storage data, if any).

- T::to_qemu_full() -> P
  Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)

With those traits, it's relatively easy to implement the QMP callbacks.
With enough interest, we could eventually start rewriting QGA in
Rust, as it is a simple service. See qga/qmp.rs for some examples.
We could also try to tackle qemu itself.

Finally, given that the QAPI types are easy to serialize, it was simple
to use "serde" on them, and provide a D-Bus interface for QMP with zbus.
(a similar approach could probably be taken for other protocols, that
could be dynamically loaded... anyone like protobuf better?)

This PoC modifies qemu-ga to provide the interface on the session bus:
$ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v
$ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
...
$ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1
...

Note: the generated code doesn't work with the qemu schema, there is a
couple of fixme/todo left.

Shameful pain point: meson & cargo don't play nicely together.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 Cargo.toml               |   2 +
 meson.build              |   6 +-
 qga/Cargo.toml           |  22 +++
 qga/commands-posix.c     |  30 -----
 qga/commands.c           |  20 ---
 qga/error.rs             |  90 +++++++++++++
 qga/lib.rs               |  11 ++
 qga/main.c               |  24 ++++
 qga/meson.build          |  42 +++++-
 qga/qapi.rs              | 121 +++++++++++++++++
 qga/qapi_dbus.rs         |  99 ++++++++++++++
 qga/qapi_sys.rs          |   5 +
 qga/qemu.rs              |  30 +++++
 qga/qemu_sys.rs          |  50 +++++++
 qga/qmp.rs               |  67 ++++++++++
 qga/translate.rs         | 173 ++++++++++++++++++++++++
 scripts/cargo.sh         |  29 ++++
 scripts/qapi-gen.py      |  22 ++-
 scripts/qapi/common.py   |   4 +
 scripts/qapi/parser.py   |   2 +
 scripts/qapi/rs.py       | 199 +++++++++++++++++++++++++++
 scripts/qapi/rs_dbus.py  |  86 ++++++++++++
 scripts/qapi/rs_sys.py   | 183 +++++++++++++++++++++++++
 scripts/qapi/rs_types.py | 281 +++++++++++++++++++++++++++++++++++++++
 scripts/qapi/schema.py   |  14 +-
 25 files changed, 1550 insertions(+), 62 deletions(-)
 create mode 100644 Cargo.toml
 create mode 100644 qga/Cargo.toml
 create mode 100644 qga/error.rs
 create mode 100644 qga/lib.rs
 create mode 100644 qga/qapi.rs
 create mode 100644 qga/qapi_dbus.rs
 create mode 100644 qga/qapi_sys.rs
 create mode 100644 qga/qemu.rs
 create mode 100644 qga/qemu_sys.rs
 create mode 100644 qga/qmp.rs
 create mode 100644 qga/translate.rs
 create mode 100755 scripts/cargo.sh
 create mode 100644 scripts/qapi/rs.py
 create mode 100644 scripts/qapi/rs_dbus.py
 create mode 100644 scripts/qapi/rs_sys.py
 create mode 100644 scripts/qapi/rs_types.py

diff --git a/Cargo.toml b/Cargo.toml
new file mode 100644
index 0000000000..e69b04200f
--- /dev/null
+++ b/Cargo.toml
@@ -0,0 +1,2 @@
+[workspace]
+members = ["qga"]
diff --git a/meson.build b/meson.build
index 5421eca66a..27a7064a47 100644
--- a/meson.build
+++ b/meson.build
@@ -637,7 +637,11 @@ qapi_gen_depends = [ meson.source_root() / 'scripts/qapi/__init__.py',
                      meson.source_root() / 'scripts/qapi/visit.py',
                      meson.source_root() / 'scripts/qapi/common.py',
                      meson.source_root() / 'scripts/qapi/doc.py',
-                     meson.source_root() / 'scripts/qapi-gen.py'
+                     meson.source_root() / 'scripts/qapi/rs.py',
+                     meson.source_root() / 'scripts/qapi/rs_sys.py',
+                     meson.source_root() / 'scripts/qapi/rs_types.py',
+                     meson.source_root() / 'scripts/qapi/rs_dbus.py',
+                     meson.source_root() / 'scripts/qapi-gen.py',
 ]
 
 tracetool = [
diff --git a/qga/Cargo.toml b/qga/Cargo.toml
new file mode 100644
index 0000000000..b0e6fe62ce
--- /dev/null
+++ b/qga/Cargo.toml
@@ -0,0 +1,22 @@
+[package]
+name = "qga"
+version = "0.1.0"
+edition = "2018"
+
+[features]
+default = ["dbus"]
+dbus = ["serde", "serde_repr", "zbus", "zvariant", "zvariant_derive"]
+
+[dependencies]
+libc = "^0.2.76"
+serde = { version = "^1.0.115", optional = true }
+serde_repr = { version = "0.1.6", optional = true }
+zbus = { git = "https://gitlab.freedesktop.org/zeenix/zbus", optional = true }
+zvariant = { git = "https://gitlab.freedesktop.org/zeenix/zbus", optional = true }
+zvariant_derive = { git = "https://gitlab.freedesktop.org/zeenix/zbus", optional = true }
+hostname = "0.3.1"
+
+[lib]
+name = "qga"
+path = "lib.rs"
+crate-type = ["staticlib"]
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1a62a3a70d..244bf04acb 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2160,36 +2160,6 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
     return NULL;
 }
 
-int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
-{
-    int64_t processed;
-    Error *local_err = NULL;
-
-    processed = 0;
-    while (vcpus != NULL) {
-        char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
-                                     vcpus->value->logical_id);
-
-        transfer_vcpu(vcpus->value, false, path, &local_err);
-        g_free(path);
-        if (local_err != NULL) {
-            break;
-        }
-        ++processed;
-        vcpus = vcpus->next;
-    }
-
-    if (local_err != NULL) {
-        if (processed == 0) {
-            error_propagate(errp, local_err);
-        } else {
-            error_free(local_err);
-        }
-    }
-
-    return processed;
-}
-
 void qmp_guest_set_user_password(const char *username,
                                  const char *password,
                                  bool crypted,
diff --git a/qga/commands.c b/qga/commands.c
index d3fec807c1..2a0db4623e 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -512,26 +512,6 @@ int ga_parse_whence(GuestFileWhence *whence, Error **errp)
     return -1;
 }
 
-GuestHostName *qmp_guest_get_host_name(Error **errp)
-{
-    GuestHostName *result = NULL;
-    g_autofree char *hostname = qemu_get_host_name(errp);
-
-    /*
-     * We want to avoid using g_get_host_name() because that
-     * caches the result and we wouldn't reflect changes in the
-     * host name.
-     */
-
-    if (!hostname) {
-        hostname = g_strdup("localhost");
-    }
-
-    result = g_new0(GuestHostName, 1);
-    result->host_name = g_steal_pointer(&hostname);
-    return result;
-}
-
 GuestTimezone *qmp_guest_get_timezone(Error **errp)
 {
     GuestTimezone *info = NULL;
diff --git a/qga/error.rs b/qga/error.rs
new file mode 100644
index 0000000000..edd2eb9fb2
--- /dev/null
+++ b/qga/error.rs
@@ -0,0 +1,90 @@
+use std::{self, ffi, fmt, ptr, io};
+
+use crate::qemu;
+use crate::qemu_sys;
+use crate::translate::*;
+
+#[derive(Debug)]
+pub enum Error {
+    FailedAt(String, &'static str, u32),
+    Io(io::Error),
+}
+
+pub type Result<T> = std::result::Result<T, Error>;
+
+impl Error {
+    fn message(&self) -> String {
+        use Error::*;
+        match self {
+            FailedAt(msg, _, _) => msg.into(),
+            Io(io) => format!("IO error: {}", io),
+        }
+    }
+
+    fn location(&self) -> Option<(&'static str, u32)> {
+        use Error::*;
+        match self {
+            FailedAt(_, file, line) => Some((file, *line)),
+            Io(_) => None,
+        }
+    }
+}
+
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use Error::*;
+        match self {
+            FailedAt(msg, file, line) => write!(f, "Failed: {} ({}:{})", msg, file, line),
+            Io(io) => write!(f, "IO error: {}", io),
+        }
+    }
+}
+
+impl From<io::Error> for Error {
+    fn from(val: io::Error) -> Self {
+        Error::Io(val)
+    }
+}
+
+impl QemuPtrDefault for Error {
+    type QemuType = *mut qemu_sys::Error;
+}
+
+impl<'a> ToQemuPtr<'a, *mut qemu_sys::Error> for Error {
+    type Storage = qemu::Error;
+
+    fn to_qemu_none(&'a self) -> Stash<'a, *mut qemu_sys::Error, Self> {
+        let err = self.to_qemu_full();
+
+        Stash(err, unsafe { qemu::Error::from_raw(err) })
+    }
+
+    fn to_qemu_full(&self) -> *mut qemu_sys::Error {
+        let cmsg =
+            ffi::CString::new(self.message()).expect("ToQemuPtr<Error>: unexpected '\0' character");
+        let mut csrc = ffi::CString::new("").unwrap();
+        let (src, line) = self.location().map_or((ptr::null(), 0 as i32), |loc| {
+            csrc = ffi::CString::new(loc.0).expect("ToQemuPtr<Error>:: unexpected '\0' character");
+            (csrc.as_ptr() as *const libc::c_char, loc.1 as i32)
+        });
+        let func = ptr::null();
+
+        let mut err: *mut qemu_sys::Error = ptr::null_mut();
+        unsafe {
+            qemu_sys::error_setg_internal(
+                &mut err as *mut *mut _,
+                src,
+                line,
+                func,
+                cmsg.as_ptr() as *const libc::c_char,
+            );
+            err
+        }
+    }
+}
+
+macro_rules! err {
+    ($err:expr) => {
+        Err(crate::error::Error::FailedAt($err.into(), file!(), line!()))
+    };
+}
diff --git a/qga/lib.rs b/qga/lib.rs
new file mode 100644
index 0000000000..6e927fd03b
--- /dev/null
+++ b/qga/lib.rs
@@ -0,0 +1,11 @@
+#[macro_use]
+mod error;
+mod qapi;
+mod qapi_sys;
+mod qemu;
+mod qemu_sys;
+mod translate;
+mod qmp;
+
+#[cfg(feature = "dbus")]
+mod qapi_dbus;
diff --git a/qga/main.c b/qga/main.c
index 3febf3b0fd..89eec0d425 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -17,6 +17,7 @@
 #ifndef _WIN32
 #include <syslog.h>
 #include <sys/wait.h>
+#include <glib-unix.h>
 #endif
 #include "qemu-common.h"
 #include "qapi/qmp/json-parser.h"
@@ -73,6 +74,13 @@ typedef struct GAPersistentState {
 
 typedef struct GAConfig GAConfig;
 
+typedef struct _QemuDBus QemuDBus;
+
+extern QemuDBus *qemu_dbus_new(void);
+extern void qemu_dbus_free(QemuDBus *dbus);
+extern int qemu_dbus_fd(QemuDBus *dbus);
+extern void qemu_dbus_next(QemuDBus *dbus);
+
 struct GAState {
     JSONMessageParser parser;
     GMainLoop *main_loop;
@@ -102,6 +110,7 @@ struct GAState {
     GAConfig *config;
     int socket_activation;
     bool force_exit;
+    QemuDBus *dbus;
 };
 
 struct GAState *ga_state;
@@ -1261,6 +1270,13 @@ static bool check_is_frozen(GAState *s)
     return false;
 }
 
+static gboolean dbus_cb(gint fd, GIOCondition condition, gpointer data)
+{
+    GAState *s = data;
+    qemu_dbus_next(s->dbus);
+    return G_SOURCE_CONTINUE;
+}
+
 static GAState *initialize_agent(GAConfig *config, int socket_activation)
 {
     GAState *s = g_new0(GAState, 1);
@@ -1354,6 +1370,14 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
 
     s->main_loop = g_main_loop_new(NULL, false);
 
+    {
+        s->dbus = qemu_dbus_new();
+        int fd = qemu_dbus_fd(s->dbus);
+        GSource *source = g_unix_fd_source_new(fd, G_IO_IN);
+        g_source_set_callback(source, (GSourceFunc) dbus_cb, s, NULL);
+        g_source_attach(source, NULL);
+    }
+
     s->config = config;
     s->socket_activation = socket_activation;
 
diff --git a/qga/meson.build b/qga/meson.build
index e5c5778a3e..ec8b7e7f39 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -46,9 +46,49 @@ qga_ss.add(when: 'CONFIG_WIN32', if_true: files(
 
 qga_ss = qga_ss.apply(config_host, strict: false)
 
+find_program('cargo', required: true)
+find_program('rustfmt', required: true)
+cargo_sh = find_program('../scripts/cargo.sh', required: true)
+
+run_target('cargo-clippy',
+           command: [cargo_sh, 'clippy'])
+
+qga_qapi_rs_outputs = [
+  'qga-qapi-sys-types.rs',
+  'qga-qapi-types.rs',
+  'qga-qapi-dbus.rs',
+]
+
+qga_qapi_rs_files = custom_target('QGA QAPI Rust files',
+                               output: qga_qapi_rs_outputs,
+                               input: 'qapi-schema.json',
+                               command: [ qapi_gen, '-r', '-o', 'qga', '-p', 'qga-', '@INPUT0@' ],
+                               depend_files: qapi_gen_depends)
+
+rs_files = files(
+  'Cargo.toml',
+  'error.rs',
+  'lib.rs',
+  'qapi.rs',
+  'qapi_dbus.rs',
+  'qapi_sys.rs',
+  'qemu.rs',
+  'qemu_sys.rs',
+  'qmp.rs',
+  'translate.rs',
+)
+
+buildtype = get_option('buildtype')
+cargo_build = custom_target('cargo-build',
+                            input: [qga_qapi_rs_files, rs_files],
+                            output: ['cargo-build.stamp'],
+                            console: true,
+                            command: [cargo_sh, 'build', buildtype, meson.current_build_dir(), meson.source_root(), meson.build_root()])
+qga_rs = declare_dependency(link_args: ['-lrt', '-ldl', 'rs-target/@0@/libqga.a'.format(buildtype)], sources: cargo_build)
+
 qga = executable('qemu-ga', qga_ss.sources(),
                  link_args: config_host['LIBS_QGA'].split(),
-                 dependencies: [qemuutil, libudev],
+                 dependencies: [qemuutil, libudev, qga_rs],
                  install: true)
 all_qga = [qga]
 
diff --git a/qga/qapi.rs b/qga/qapi.rs
new file mode 100644
index 0000000000..5065bb6930
--- /dev/null
+++ b/qga/qapi.rs
@@ -0,0 +1,121 @@
+#![allow(dead_code)]
+use std::convert::{TryFrom, TryInto};
+use std::{ptr, str};
+
+#[cfg(feature = "dbus")]
+use zvariant::OwnedValue;
+#[cfg(feature = "dbus")]
+use serde::{Deserialize, Serialize};
+#[cfg(feature = "dbus")]
+use zvariant_derive::{Type, TypeDict, SerializeDict, DeserializeDict};
+
+use crate::translate::*;
+
+use crate::translate;
+use crate::qapi_sys;
+use crate::qemu_sys;
+
+include!(concat!(env!("MESON_BUILD_ROOT"), "/qga/qga-qapi-types.rs"));
+
+impl<'a> ToQemuPtr<'a, *mut qapi_sys::GuestFileWhence> for GuestFileWhence {
+    type Storage = Box<qapi_sys::GuestFileWhence>;
+
+    #[inline]
+    fn to_qemu_none(&'a self) -> Stash<'a, *mut qapi_sys::GuestFileWhence, GuestFileWhence> {
+        let mut w = Box::new(self.into());
+
+        Stash(&mut *w, w)
+    }
+}
+
+impl From<&GuestFileWhence> for qapi_sys::GuestFileWhence {
+    fn from(w: &GuestFileWhence) -> Self {
+        match *w {
+            GuestFileWhence::Name(name) => Self {
+                ty: QType::Qstring,
+                u: qapi_sys::GuestFileWhenceUnion { name },
+            },
+            GuestFileWhence::Value(value) => Self {
+                ty: QType::Qnum,
+                u: qapi_sys::GuestFileWhenceUnion { value },
+            },
+        }
+    }
+}
+
+#[cfg(feature = "dbus")]
+impl From<GuestFileWhence> for OwnedValue {
+    fn from(_w: GuestFileWhence) -> Self {
+        unimplemented!()
+    }
+}
+
+#[cfg(feature = "dbus")]
+impl TryFrom<OwnedValue> for GuestFileWhence {
+    type Error = &'static str;
+
+    fn try_from(value: OwnedValue) -> Result<Self, Self::Error> {
+        if let Ok(val) = (&value).try_into() {
+            return Ok(Self::Name(match val {
+                "set" => QGASeek::Set,
+                "cur" => QGASeek::Cur,
+                "end" => QGASeek::End,
+                _ => return Err("Invalid seek value"),
+            }));
+        }
+        if let Ok(val) = value.try_into() {
+            return Ok(Self::Value(val));
+        };
+        Err("Invalid whence")
+    }
+}
+
+macro_rules! vec_to_qemu_ptr {
+    ($rs:ident, $sys:ident) => {
+        #[allow(non_camel_case_types)]
+        pub struct $sys(*mut qapi_sys::$sys);
+
+        impl Drop for $sys {
+            fn drop(&mut self) {
+                let mut list = self.0;
+                unsafe {
+                    while !list.is_null() {
+                        let next = (*list).next;
+                        Box::from_raw(list);
+                        list = next;
+                    }
+                }
+            }
+        }
+
+        impl<'a> ToQemuPtr<'a, *mut qapi_sys::$sys> for Vec<$rs> {
+            type Storage = (
+                Option<$sys>,
+                Vec<Stash<'a, <$rs as QemuPtrDefault>::QemuType, $rs>>,
+            );
+
+            #[inline]
+            fn to_qemu_none(&self) -> Stash<*mut qapi_sys::$sys, Self> {
+                let stash_vec: Vec<_> = self.iter().rev().map(ToQemuPtr::to_qemu_none).collect();
+                let mut list: *mut qapi_sys::$sys = ptr::null_mut();
+                for stash in &stash_vec {
+                    let b = Box::new(qapi_sys::$sys {
+                        next: list,
+                        value: Ptr::to(stash.0),
+                    });
+                    list = Box::into_raw(b);
+                }
+                Stash(list, (Some($sys(list)), stash_vec))
+            }
+        }
+    };
+}
+
+// TODO: could probably be templated instead
+vec_to_qemu_ptr!(String, strList);
+vec_to_qemu_ptr!(GuestAgentCommandInfo, GuestAgentCommandInfoList);
+vec_to_qemu_ptr!(GuestFilesystemTrimResult, GuestFilesystemTrimResultList);
+vec_to_qemu_ptr!(GuestIpAddress, GuestIpAddressList);
+vec_to_qemu_ptr!(GuestDiskAddress, GuestDiskAddressList);
+vec_to_qemu_ptr!(GuestLogicalProcessor, GuestLogicalProcessorList);
+vec_to_qemu_ptr!(GuestMemoryBlock, GuestMemoryBlockList);
diff --git a/qga/qapi_dbus.rs b/qga/qapi_dbus.rs
new file mode 100644
index 0000000000..2713c6e1a4
--- /dev/null
+++ b/qga/qapi_dbus.rs
@@ -0,0 +1,99 @@
+use std::convert::TryInto;
+use std::error::Error;
+use std::ffi::CString;
+use std::os::unix::io::{AsRawFd, RawFd};
+use std::ptr;
+
+use zbus::fdo;
+use zbus::{dbus_interface, Connection, DBusError, ObjectServer};
+
+use crate::qapi;
+use crate::qapi_sys;
+use crate::qemu;
+use crate::qemu_sys;
+use crate::translate::*;
+
+include!(concat!(env!("MESON_BUILD_ROOT"), "/qga/qga-qapi-dbus.rs"));
+
+#[derive(Debug, DBusError)]
+#[dbus_error(prefix = "org.qemu.QapiError")]
+pub enum QapiError {
+    /// ZBus error
+    ZBus(zbus::Error),
+    /// QMP error
+    Failed(String),
+}
+
+impl FromQemuPtrFull<*mut qemu_sys::Error> for QapiError {
+    unsafe fn from_qemu_full(ptr: *mut qemu_sys::Error) -> Self {
+        QapiError::Failed(qemu::Error::from_raw(ptr).pretty().to_string())
+    }
+}
+
+type Result<T> = std::result::Result<T, QapiError>;
+
+#[derive(Debug)]
+pub struct QemuDBus {
+    pub connection: Connection,
+    pub server: ObjectServer<'static>,
+}
+
+impl QemuDBus {
+    fn open(name: &str) -> std::result::Result<Self, Box<dyn Error>> {
+        let connection = Connection::new_session()?;
+
+        fdo::DBusProxy::new(&connection)?
+            .request_name(name, fdo::RequestNameFlags::ReplaceExisting.into())?;
+
+        let server = ObjectServer::new(&connection);
+        Ok(Self { connection, server })
+    }
+}
+
+#[no_mangle]
+extern "C" fn qemu_dbus_new() -> *mut QemuDBus {
+    let mut dbus = match QemuDBus::open(&"org.qemu.qga") {
+        Ok(dbus) => dbus,
+        Err(e) => {
+            eprintln!("{}", e);
+            return std::ptr::null_mut();
+        }
+    };
+    dbus.server
+        .at(&"/org/qemu/qga".try_into().unwrap(), QgaQapi)
+        .unwrap();
+
+    Box::into_raw(Box::new(dbus))
+}
+
+#[no_mangle]
+extern "C" fn qemu_dbus_free(dbus: *mut QemuDBus) {
+    let dbus = unsafe {
+        assert!(!dbus.is_null());
+        Box::from_raw(dbus)
+    };
+    // let's be explicit:
+    drop(dbus)
+}
+
+#[no_mangle]
+extern "C" fn qemu_dbus_fd(dbus: *mut QemuDBus) -> RawFd {
+    let dbus = unsafe {
+        assert!(!dbus.is_null());
+        &mut *dbus
+    };
+
+    dbus.connection.as_raw_fd()
+}
+
+#[no_mangle]
+extern "C" fn qemu_dbus_next(dbus: *mut QemuDBus) {
+    let dbus = unsafe {
+        assert!(!dbus.is_null());
+        &mut *dbus
+    };
+
+    if let Err(err) = dbus.server.try_handle_next() {
+        eprintln!("{}", err);
+    }
+}
diff --git a/qga/qapi_sys.rs b/qga/qapi_sys.rs
new file mode 100644
index 0000000000..06fc49b826
--- /dev/null
+++ b/qga/qapi_sys.rs
@@ -0,0 +1,5 @@
+#![allow(dead_code)]
+include!(concat!(
+    env!("MESON_BUILD_ROOT"),
+    "/qga/qga-qapi-sys-types.rs"
+));
diff --git a/qga/qemu.rs b/qga/qemu.rs
new file mode 100644
index 0000000000..5aad9a2b55
--- /dev/null
+++ b/qga/qemu.rs
@@ -0,0 +1,30 @@
+use std::ffi::CStr;
+/// or do something full-fledged like glib-rs boxed MM...
+use std::ptr;
+use std::str;
+
+use crate::qemu_sys;
+
+pub struct Error(ptr::NonNull<qemu_sys::Error>);
+
+impl Error {
+    pub unsafe fn from_raw(ptr: *mut qemu_sys::Error) -> Self {
+        assert!(!ptr.is_null());
+        Self(ptr::NonNull::new_unchecked(ptr))
+    }
+
+    pub fn pretty(&self) -> &str {
+        unsafe {
+            let pretty = qemu_sys::error_get_pretty(self.0.as_ptr());
+            let bytes = CStr::from_ptr(pretty).to_bytes();
+            str::from_utf8(bytes)
+                .unwrap_or_else(|err| str::from_utf8(&bytes[..err.valid_up_to()]).unwrap())
+        }
+    }
+}
+
+impl Drop for Error {
+    fn drop(&mut self) {
+        unsafe { qemu_sys::error_free(self.0.as_ptr()) }
+    }
+}
diff --git a/qga/qemu_sys.rs b/qga/qemu_sys.rs
new file mode 100644
index 0000000000..04fc0d9f9d
--- /dev/null
+++ b/qga/qemu_sys.rs
@@ -0,0 +1,50 @@
+use libc::{c_char, c_void, size_t};
+
+extern "C" {
+    pub fn g_malloc0(n_bytes: size_t) -> *mut c_void;
+    pub fn g_free(ptr: *mut c_void);
+    pub fn g_strndup(str: *const c_char, n: size_t) -> *mut c_char;
+}
+
+#[repr(C)]
+pub struct QObject(c_void);
+
+impl ::std::fmt::Debug for QObject {
+    fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
+        f.debug_struct(&format!("QObject @ {:?}", self as *const _))
+            .finish()
+    }
+}
+
+#[repr(C)]
+pub struct QNull(c_void);
+
+impl ::std::fmt::Debug for QNull {
+    fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
+        f.debug_struct(&format!("QNull @ {:?}", self as *const _))
+            .finish()
+    }
+}
+
+#[repr(C)]
+pub struct Error(c_void);
+
+impl ::std::fmt::Debug for Error {
+    fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
+        f.debug_struct(&format!("Error @ {:?}", self as *const _))
+            .finish()
+    }
+}
+
+extern "C" {
+    pub fn error_setg_internal(
+        errp: *mut *mut Error,
+        src: *const c_char,
+        line: i32,
+        func: *const c_char,
+        fmt: *const c_char,
+        ...
+    );
+    pub fn error_get_pretty(err: *const Error) -> *const c_char;
+    pub fn error_free(err: *mut Error);
+}
diff --git a/qga/qmp.rs b/qga/qmp.rs
new file mode 100644
index 0000000000..0224e7e4fb
--- /dev/null
+++ b/qga/qmp.rs
@@ -0,0 +1,67 @@
+use std::ptr;
+
+use crate::error::Result;
+use crate::qapi;
+use crate::qapi_sys;
+use crate::qemu_sys;
+use crate::translate::*;
+
+macro_rules! qmp {
+    // the basic return value variant
+    ($e:expr, $errp:ident, $errval:expr) => {{
+        assert!(!$errp.is_null());
+        unsafe {
+            *$errp = ptr::null_mut();
+        }
+
+        match $e {
+            Ok(val) => val,
+            Err(err) => unsafe {
+                *$errp = err.to_qemu_full();
+                $errval
+            },
+        }
+    }};
+    // the ptr return value variant
+    ($e:expr, $errp:ident) => {{
+        assert!(!$errp.is_null());
+        unsafe {
+            *$errp = ptr::null_mut();
+        }
+
+        match $e {
+            Ok(val) => val.to_qemu_full(),
+            Err(err) => unsafe {
+                *$errp = err.to_qemu_full();
+                ptr::null_mut()
+            },
+        }
+    }};
+}
+
+fn guest_host_name() -> Result<qapi::GuestHostName> {
+    Ok(qapi::GuestHostName {
+        host_name: hostname::get()?.into_string().or(err!("Invalid hostname"))?,
+    })
+}
+
+#[no_mangle]
+extern "C" fn qmp_guest_get_host_name(
+    errp: *mut *mut qemu_sys::Error,
+) -> *mut qapi_sys::GuestHostName {
+    qmp!(guest_host_name(), errp)
+}
+
+fn guest_set_vcpus(vcpus: Vec<qapi::GuestLogicalProcessor>) -> Result<i64> {
+    dbg!(vcpus);
+    err!("unimplemented")
+}
+
+#[no_mangle]
+extern "C" fn qmp_guest_set_vcpus(
+    vcpus: *const qapi_sys::GuestLogicalProcessorList,
+    errp: *mut *mut qemu_sys::Error,
+) -> libc::c_longlong {
+    let vcpus = unsafe { from_qemu_none(vcpus) };
+    qmp!(guest_set_vcpus(vcpus), errp, -1)
+}
diff --git a/qga/translate.rs b/qga/translate.rs
new file mode 100644
index 0000000000..715951f2ba
--- /dev/null
+++ b/qga/translate.rs
@@ -0,0 +1,173 @@
+// largely adapted from glib-rs
+// we don't depend on glib-rs as this brings a lot more code that we may not need
+// and also because there are issues with the conversion traits for our sys::*mut.
+use libc::{c_char, size_t};
+use std::ffi::{CStr, CString};
+use std::ptr;
+
+use crate::qemu_sys;
+
+pub trait Ptr: Copy + 'static {
+    fn is_null(&self) -> bool;
+    fn from<X>(ptr: *mut X) -> Self;
+    fn to<X>(self) -> *mut X;
+}
+
+impl<T: 'static> Ptr for *const T {
+    #[inline]
+    fn is_null(&self) -> bool {
+        (*self).is_null()
+    }
+
+    #[inline]
+    fn from<X>(ptr: *mut X) -> *const T {
+        ptr as *const T
+    }
+
+    #[inline]
+    fn to<X>(self) -> *mut X {
+        self as *mut X
+    }
+}
+
+impl<T: 'static> Ptr for *mut T {
+    #[inline]
+    fn is_null(&self) -> bool {
+        (*self).is_null()
+    }
+
+    #[inline]
+    fn from<X>(ptr: *mut X) -> *mut T {
+        ptr as *mut T
+    }
+
+    #[inline]
+    fn to<X>(self) -> *mut X {
+        self as *mut X
+    }
+}
+
+/// Provides the default pointer type to be used in some container conversions.
+///
+/// It's `*mut c_char` for `String`, `*mut GtkButton` for `gtk::Button`, etc.
+pub trait QemuPtrDefault {
+    type QemuType: Ptr;
+}
+
+impl QemuPtrDefault for String {
+    type QemuType = *mut c_char;
+}
+
+pub struct Stash<'a, P: Copy, T: ?Sized + ToQemuPtr<'a, P>>(
+    pub P,
+    pub <T as ToQemuPtr<'a, P>>::Storage,
+);
+
+/// Translate to a pointer.
+pub trait ToQemuPtr<'a, P: Copy> {
+    type Storage;
+
+    /// Transfer: none.
+    ///
+    /// The pointer in the `Stash` is only valid for the lifetime of the `Stash`.
+    fn to_qemu_none(&'a self) -> Stash<'a, P, Self>;
+
+    /// Transfer: full.
+    ///
+    /// We transfer the ownership to the foreign library.
+    fn to_qemu_full(&self) -> P {
+        unimplemented!();
+    }
+}
+
+impl<'a, P: Ptr, T: ToQemuPtr<'a, P>> ToQemuPtr<'a, P> for Option<T> {
+    type Storage = Option<<T as ToQemuPtr<'a, P>>::Storage>;
+
+    #[inline]
+    fn to_qemu_none(&'a self) -> Stash<'a, P, Option<T>> {
+        self.as_ref()
+            .map_or(Stash(Ptr::from::<()>(ptr::null_mut()), None), |s| {
+                let s = s.to_qemu_none();
+                Stash(s.0, Some(s.1))
+            })
+    }
+
+    #[inline]
+    fn to_qemu_full(&self) -> P {
+        self.as_ref()
+            .map_or(Ptr::from::<()>(ptr::null_mut()), ToQemuPtr::to_qemu_full)
+    }
+}
+
+impl<'a> ToQemuPtr<'a, *mut c_char> for String {
+    type Storage = CString;
+
+    #[inline]
+    fn to_qemu_none(&self) -> Stash<'a, *mut c_char, String> {
+        let tmp = CString::new(&self[..])
+            .expect("String::ToQemuPtr<*mut c_char>: unexpected '\0' character");
+        Stash(tmp.as_ptr() as *mut c_char, tmp)
+    }
+
+    #[inline]
+    fn to_qemu_full(&self) -> *mut c_char {
+        unsafe { qemu_sys::g_strndup(self.as_ptr() as *const c_char, self.len() as size_t) }
+    }
+}
+
+pub trait FromQemuPtrNone<P: Ptr>: Sized {
+    unsafe fn from_qemu_none(ptr: P) -> Self;
+}
+
+pub trait FromQemuPtrFull<P: Ptr>: Sized {
+    unsafe fn from_qemu_full(ptr: P) -> Self;
+}
+
+#[inline]
+pub unsafe fn from_qemu_none<P: Ptr, T: FromQemuPtrNone<P>>(ptr: P) -> T {
+    FromQemuPtrNone::from_qemu_none(ptr)
+}
+
+#[inline]
+pub unsafe fn from_qemu_full<P: Ptr, T: FromQemuPtrFull<P>>(ptr: P) -> T {
+    FromQemuPtrFull::from_qemu_full(ptr)
+}
+
+impl<P: Ptr, T: FromQemuPtrNone<P>> FromQemuPtrNone<P> for Option<T> {
+    #[inline]
+    unsafe fn from_qemu_none(ptr: P) -> Option<T> {
+        if ptr.is_null() {
+            None
+        } else {
+            Some(from_qemu_none(ptr))
+        }
+    }
+}
+
+impl<P: Ptr, T: FromQemuPtrFull<P>> FromQemuPtrFull<P> for Option<T> {
+    #[inline]
+    unsafe fn from_qemu_full(ptr: P) -> Option<T> {
+        if ptr.is_null() {
+            None
+        } else {
+            Some(from_qemu_full(ptr))
+        }
+    }
+}
+
+impl FromQemuPtrNone<*const c_char> for String {
+    #[inline]
+    unsafe fn from_qemu_none(ptr: *const c_char) -> Self {
+        assert!(!ptr.is_null());
+        String::from_utf8_lossy(CStr::from_ptr(ptr).to_bytes()).into_owned()
+    }
+}
+
+impl FromQemuPtrFull<*mut c_char> for String {
+    #[inline]
+    unsafe fn from_qemu_full(ptr: *mut c_char) -> Self {
+        let res = from_qemu_none(ptr as *const _);
+        qemu_sys::g_free(ptr as *mut _);
+        res
+    }
+}
diff --git a/scripts/cargo.sh b/scripts/cargo.sh
new file mode 100755
index 0000000000..bc000ef62c
--- /dev/null
+++ b/scripts/cargo.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+export CARGO_CMD="$1"
+shift
+
+if [ "$CARGO_CMD" = "build" ]; then
+    export MESON_BUILD_TYPE="$1"
+    shift
+    export MESON_CURRENT_BUILD_DIR="$1"
+    shift
+    export MESON_SOURCE_ROOT="$1"
+    shift
+    export MESON_BUILD_ROOT="$1"
+    shift
+fi
+
+OUTDIR=debug
+
+if [[ "$MESON_BUILD_TYPE" = release ]]
+then
+    EXTRA_ARGS="--release"
+    OUTDIR=release
+fi
+
+cargo "$CARGO_CMD" --manifest-path "$MESON_SOURCE_ROOT/Cargo.toml" --target-dir="$MESON_BUILD_ROOT/rs-target" $EXTRA_ARGS "$@"
+
+if [ "$CARGO_CMD" = "build" ]; then
+    touch "$MESON_CURRENT_BUILD_DIR"/cargo-build.stamp
+fi
diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
index 4b03f7d53b..9743ea164b 100644
--- a/scripts/qapi-gen.py
+++ b/scripts/qapi-gen.py
@@ -17,10 +17,15 @@ from qapi.schema import QAPIError, QAPISchema
 from qapi.types import gen_types
 from qapi.visit import gen_visit
 
+from qapi.rs_sys import gen_rs_systypes
+from qapi.rs_types import gen_rs_types
+from qapi.rs_dbus import gen_rs_dbus
 
 def main(argv):
     parser = argparse.ArgumentParser(
         description='Generate code from a QAPI schema')
+    parser.add_argument('-r', '--rust', action='store_true',
+                        help="generate Rust code")
     parser.add_argument('-b', '--builtins', action='store_true',
                         help="generate code for built-in types")
     parser.add_argument('-o', '--output-dir', action='store', default='',
@@ -46,12 +51,17 @@ def main(argv):
         print(err, file=sys.stderr)
         exit(1)
 
-    gen_types(schema, args.output_dir, args.prefix, args.builtins)
-    gen_visit(schema, args.output_dir, args.prefix, args.builtins)
-    gen_commands(schema, args.output_dir, args.prefix)
-    gen_events(schema, args.output_dir, args.prefix)
-    gen_introspect(schema, args.output_dir, args.prefix, args.unmask)
-    gen_doc(schema, args.output_dir, args.prefix)
+    if args.rust:
+        gen_rs_systypes(schema, args.output_dir, args.prefix, args.builtins)
+        gen_rs_types(schema, args.output_dir, args.prefix, args.builtins)
+        gen_rs_dbus(schema, args.output_dir, args.prefix)
+    else:
+        gen_types(schema, args.output_dir, args.prefix, args.builtins)
+        gen_visit(schema, args.output_dir, args.prefix, args.builtins)
+        gen_commands(schema, args.output_dir, args.prefix)
+        gen_events(schema, args.output_dir, args.prefix)
+        gen_introspect(schema, args.output_dir, args.prefix, args.unmask)
+        gen_doc(schema, args.output_dir, args.prefix)
 
 
 if __name__ == '__main__':
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index ba35abea47..580b06c72a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -42,6 +42,10 @@ def c_enum_const(type_name, const_name, prefix=None):
     return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
 
 
+def to_camel_case(value):
+    return ''.join(word.title() for word in filter(None, re.split("[-_]+", value)))
+
+
 c_name_trans = str.maketrans('.-', '__')
 
 
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 165925ca72..e998168ebe 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -342,6 +342,7 @@ class QAPIDoc:
         # servicable, but action at a distance.
         self._parser = parser
         self.info = info
+        self.text = '' # unprocessed text
         self.symbol = None
         self.body = QAPIDoc.Section()
         # dict mapping parameter name to ArgSection
@@ -371,6 +372,7 @@ class QAPIDoc:
         * A features section: ._append_line is ._append_features_line
         * An additional section: ._append_line is ._append_various_line
         """
+        self.text += line + '\n'
         line = line[1:]
         if not line:
             self._append_freeform(line)
diff --git a/scripts/qapi/rs.py b/scripts/qapi/rs.py
new file mode 100644
index 0000000000..7336df62e9
--- /dev/null
+++ b/scripts/qapi/rs.py
@@ -0,0 +1,199 @@
+"""
+QAPI Rust generator
+"""
+
+import os
+import subprocess
+
+from qapi.common import *
+from qapi.gen import QAPIGen, QAPISchemaVisitor
+
+
+from_list = set()
+
+
+def rs_name(name, protect=True):
+    name = c_name(name, protect)
+    if name == 'type':
+        name = "r#type"
+    return name
+
+
+def rs_type(c_type, ns='qapi::', optional=False):
+    vec = False
+    to_rs = {
+        'char': 'i8',
+        'int8_t': 'i8',
+        'uint8_t': 'u8',
+        'int16_t': 'i16',
+        'uint16_t': 'u16',
+        'int32_t': 'i32',
+        'uint32_t': 'u32',
+        'int64_t': 'i64',
+        'uint64_t': 'u64',
+        'double': 'f64',
+        'bool': 'bool',
+        'str': 'String',
+    }
+    if c_type.startswith('const '):
+        c_type = c_type[6:]
+    if c_type.endswith(pointer_suffix):
+        c_type = c_type.rstrip(pointer_suffix).strip()
+        if c_type.endswith('List'):
+            c_type = c_type[:-4]
+            vec = True
+        else:
+            to_rs = {
+                'char': 'String',
+            }
+
+    if c_type in to_rs:
+        ret = to_rs[c_type]
+    else:
+        ret = ns + c_type
+
+    if vec:
+        ret = 'Vec<%s>' % ret
+    if optional:
+        return 'Option<%s>' % ret
+    else:
+        return ret
+
+
+def rs_systype(c_type, sys_ns='qapi_sys::'):
+    is_pointer = False
+    is_const = False
+    if c_type.endswith(pointer_suffix):
+        is_pointer = True
+        c_type = c_type.rstrip(pointer_suffix).strip()
+
+    if c_type.startswith('const '):
+        c_type = c_type[6:]
+        is_const = True
+
+    to_rs = {
+        'char': 'libc::c_char',
+        'int8_t': 'i8',
+        'uint8_t': 'u8',
+        'int16_t': 'i16',
+        'uint16_t': 'u16',
+        'int32_t': 'i32',
+        'uint32_t': 'u32',
+        'int64_t': 'libc::c_longlong',
+        'uint64_t': 'libc::c_ulonglong',
+        'double': 'libc::c_double',
+        'bool': 'bool',
+    }
+
+    rs = ''
+    if is_const and is_pointer:
+        rs += '*const '
+    elif is_pointer:
+        rs += '*mut '
+    if c_type in to_rs:
+        rs += to_rs[c_type]
+    else:
+        rs += sys_ns + c_type
+
+    return rs
+
+
+def build_params(arg_type, boxed, typefn=rs_systype, extra=[]):
+    ret = []
+    if boxed:
+        assert arg_type
+        ret.append('arg: %s' % arg_type.c_param_type(const=True))
+    elif arg_type:
+        assert not arg_type.variants
+        for memb in arg_type.members:
+            if memb.optional:
+                ret.append('has_%s: bool' % c_name(memb.name))
+            ret.append('%s: %s' % (c_name(memb.name), typefn(memb.type.c_param_type(const=True))))
+    ret.extend(extra)
+    return ', '.join(ret)
+
+
+class QAPIGenRs(QAPIGen):
+
+    def __init__(self, fname):
+        super().__init__(fname)
+
+
+class QAPISchemaRsVisitor(QAPISchemaVisitor):
+
+    def __init__(self, prefix, what):
+        self._prefix = prefix
+        self._what = what
+        self._gen = QAPIGenRs(self._prefix + self._what + '.rs')
+
+    def write(self, output_dir):
+        self._gen.write(output_dir)
+
+        pathname = os.path.join(output_dir, self._gen.fname)
+        subprocess.call(['rustfmt', pathname])
+
+
+def to_qemu_none(c_type, name):
+    is_pointer = False
+    is_const = False
+    if c_type.endswith(pointer_suffix):
+        is_pointer = True
+        c_type = c_type.rstrip(pointer_suffix).strip()
+        sys_type = rs_systype(c_type)
+
+    if c_type.startswith('const '):
+        c_type = c_type[6:]
+        is_const = True
+
+    if is_pointer:
+        if c_type == 'char':
+            return mcgen('''
+    let %(name)s_ = CString::new(%(name)s).unwrap();
+    let %(name)s = %(name)s_.as_ptr();
+''', name=name)
+        else:
+            return mcgen('''
+    let %(name)s_ = %(name)s.to_qemu_none();
+    let %(name)s = %(name)s_.0;
+''', name=name, sys_type=sys_type)
+    return ''
+
+
+def gen_call(name, arg_type, boxed, ret_type):
+    ret = ''
+
+    argstr = ''
+    if boxed:
+        assert arg_type
+        argstr = '&arg, '
+    elif arg_type:
+        assert not arg_type.variants
+        for memb in arg_type.members:
+            if memb.optional:
+                argstr += 'has_%s, ' % c_name(memb.name)
+            ret += to_qemu_none(memb.type.c_type(), c_name(memb.name))
+            argstr += ' %s, ' % c_name(memb.name)
+
+    lhs = ''
+    if ret_type:
+        lhs = 'let retval_ = '
+
+    ret += mcgen('''
+
+%(lhs)sqmp_%(c_name)s(%(args)s&mut err_);
+''',
+                c_name=c_name(name), args=argstr, lhs=lhs)
+    return ret
+
+
+def from_qemu(var_name, c_type, full=False):
+    if c_type.endswith('List' + pointer_suffix):
+        from_list.add(c_type)
+    is_pointer = c_type.endswith(pointer_suffix)
+    if is_pointer:
+        if full:
+            return 'from_qemu_full(%s as *mut _)' % var_name
+        else:
+            return 'from_qemu_none(%s as *const _)' % var_name
+    else:
+        return var_name
diff --git a/scripts/qapi/rs_dbus.py b/scripts/qapi/rs_dbus.py
new file mode 100644
index 0000000000..5036e774a8
--- /dev/null
+++ b/scripts/qapi/rs_dbus.py
@@ -0,0 +1,86 @@
+"""
+QAPI Rust DBus interface generator
+"""
+
+from qapi.common import *
+from qapi.rs import QAPISchemaRsVisitor, rs_systype, from_qemu, build_params, rs_type, gen_call
+
+
+class QAPISchemaGenRsDBusVisitor(QAPISchemaRsVisitor):
+
+    def __init__(self, prefix):
+        super().__init__(prefix, 'qapi-dbus')
+        self._cur_doc = None
+        self._dbus_gen = ''
+
+    def visit_begin(self, schema):
+        self.schema = schema
+        self._gen.add(
+            mcgen('''
+// generated by qapi-gen, DO NOT EDIT
+
+extern "C" {
+'''))
+
+    def visit_end(self):
+        self._gen.add(mcgen('''
+}
+
+pub(crate) struct %(name)s;
+
+#[dbus_interface(name = "org.qemu.%(name)s")]
+impl %(name)s {
+%(dbus)s
+}
+''', name=to_camel_case(self._prefix + 'Qapi'), dbus=self._dbus_gen))
+
+    def visit_command(self, name, info, ifcond, features,
+                      arg_type, ret_type, gen, success_response, boxed,
+                      allow_oob, allow_preconfig):
+        if not gen:
+            return
+
+        ret = ''
+        retval = '()'
+        if ret_type:
+            ret = ' -> %s' % rs_systype(ret_type.c_type())
+            retval = from_qemu('retval_', ret_type.c_type(), full=True)
+
+        doc = ''
+        for s in self.schema.docs:
+            if s.symbol == name:
+                for l in s.text.splitlines():
+                    doc += '///%s\n' % l[1:]
+                if doc.endswith('\n'):
+                    doc = doc[:-1]
+
+        self._gen.add(mcgen('''
+    fn qmp_%(name)s(%(params)s)%(ret)s;
+''', name = c_name(name), params=build_params(arg_type, boxed, extra=['errp: *mut *mut qemu_sys::Error']), ret=ret))
+
+        ret = ' -> Result<()>'
+        if ret_type:
+            ret = ' -> Result<%s>' % rs_type(ret_type.c_type())
+
+        self._dbus_gen += mcgen('''
+    %(doc)s
+    fn %(name)s(&self, %(params)s)%(ret)s {
+        unsafe {
+            let mut err_ = ptr::null_mut();
+            %(call)s
+            if err_.is_null() {
+                Ok(%(retval)s)
+            } else {
+                Err(from_qemu_full(err_))
+            }
+        }
+    }
+
+''', doc = doc, call = gen_call(name, arg_type, boxed, ret_type),
+     name = c_name(name), params=build_params(arg_type, boxed, typefn=rs_type), ret=ret, retval=retval)
+
+
+def gen_rs_dbus(schema, output_dir, prefix):
+    vis = QAPISchemaGenRsDBusVisitor(prefix)
+    schema.visit(vis)
+    vis.write(output_dir)
diff --git a/scripts/qapi/rs_sys.py b/scripts/qapi/rs_sys.py
new file mode 100644
index 0000000000..5128398cb9
--- /dev/null
+++ b/scripts/qapi/rs_sys.py
@@ -0,0 +1,183 @@
+"""
+QAPI Rust sys/ffi generator
+"""
+
+from qapi.common import *
+from qapi.schema import QAPISchemaEnumMember, QAPISchemaObjectType
+from qapi.rs import QAPISchemaRsVisitor, rs_systype, rs_name
+
+
+objects_seen = set()
+
+
+def gen_rs_sys_enum(name, members, prefix=None):
+    # append automatically generated _max value
+    enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
+
+    ret = mcgen('''
+
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+#[cfg_attr(feature = "dbus", derive(Deserialize_repr, Serialize_repr, Type))]
+#[repr(i32)] // FIXME: serde-repr#10
+pub enum %(rs_name)s {
+''',
+    rs_name=rs_name(name))
+
+    for m in enum_members:
+        ret += mcgen('''
+    %(c_enum)s,
+''',
+                     c_enum=to_camel_case(rs_name(m.name, False)))
+    ret += mcgen('''
+}
+''')
+    return ret
+
+
+def gen_rs_sys_struct_members(members):
+    ret = ''
+    for memb in members:
+        if memb.optional:
+            ret += mcgen('''
+    pub has_%(rs_name)s: bool,
+''',
+                         rs_name=rs_name(memb.name))
+        ret += mcgen('''
+    pub %(rs_name)s: %(rs_systype)s,
+''',
+                     rs_systype=rs_systype(memb.type.c_type(), ''), rs_name=rs_name(memb.name))
+    return ret
+
+
+def gen_rs_sys_free(ty):
+    return mcgen('''
+
+extern "C" {
+    pub fn qapi_free_%(ty)s(obj: *mut %(ty)s);
+}
+''', ty=ty)
+
+
+def gen_rs_sys_object(name, ifcond, base, members, variants):
+    if name in objects_seen:
+        return ''
+
+    ret = ''
+    objects_seen.add(name)
+    if variants:
+        ret += 'variants TODO'
+
+    ret += gen_rs_sys_free(rs_name(name))
+    ret += mcgen('''
+
+#[repr(C)]
+#[derive(Debug)]
+pub struct %(rs_name)s {
+''',
+                 rs_name=rs_name(name))
+
+    if base:
+        ret += 'Base TODO'
+
+    ret += gen_rs_sys_struct_members(members)
+
+    ret += mcgen('''
+}
+''')
+    return ret
+
+
+def gen_rs_sys_variant(name, ifcond, variants):
+    if name in objects_seen:
+        return ''
+
+    objects_seen.add(name)
+
+    vs = ''
+    for var in variants.variants:
+        if var.type.name == 'q_empty':
+            continue
+        vs += mcgen('''
+    pub %(mem_name)s: %(rs_systype)s,
+''',
+                     rs_systype=rs_systype(var.type.c_unboxed_type(), ''),
+                     mem_name=rs_name(var.name))
+
+    return mcgen('''
+
+#[repr(C)]
+pub union %(rs_name)sUnion {
+    %(variants)s
+}
+
+#[repr(C)]
+pub struct %(rs_name)s {
+    pub ty: QType,
+    pub u: %(rs_name)sUnion,
+}
+''',
+                 rs_name=rs_name(name), variants=vs)
+
+
+def gen_rs_sys_array(name, element_type):
+    ret = mcgen('''
+
+#[repr(C)]
+pub struct %(rs_name)s {
+    pub next: *mut %(rs_name)s,
+    pub value: %(rs_systype)s,
+}
+
+impl ::std::fmt::Debug for %(rs_name)s {
+    fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
+        f.debug_struct(&format!("%(rs_name)s @ {:?}", self as *const _))
+            .finish()
+    }
+}
+''',
+                 rs_name=rs_name(name), rs_systype=rs_systype(element_type.c_type(), ''))
+    ret += gen_rs_sys_free(rs_name(name))
+    return ret
+
+
+class QAPISchemaGenRsSysTypeVisitor(QAPISchemaRsVisitor):
+
+    def __init__(self, prefix):
+        super().__init__(prefix, 'qapi-sys-types')
+
+    def visit_begin(self, schema):
+        # gen_object() is recursive, ensure it doesn't visit the empty type
+        objects_seen.add(schema.the_empty_object_type.name)
+        self._gen.preamble_add(
+            mcgen('''
+// generated by qapi-gen, DO NOT EDIT
+
+#[cfg(feature = "dbus")]
+use serde_repr::{Deserialize_repr, Serialize_repr};
+#[cfg(feature = "dbus")]
+use zvariant_derive::Type;
+
+use crate::qemu_sys::{QNull, QObject};
+
+'''))
+
+    def visit_enum_type(self, name, info, ifcond, features, members, prefix):
+        self._gen.add(gen_rs_sys_enum(name, members, prefix))
+
+    def visit_array_type(self, name, info, ifcond, element_type):
+        self._gen.add(gen_rs_sys_array(name, element_type))
+
+    def visit_object_type(self, name, info, ifcond, features,
+                          base, members, variants):
+        if name.startswith('q_'):
+            return
+        self._gen.add(gen_rs_sys_object(name, ifcond, base, members, variants))
+
+    def visit_alternate_type(self, name, info, ifcond, features, variants):
+        self._gen.add(gen_rs_sys_variant(name, ifcond, variants))
+
+
+def gen_rs_systypes(schema, output_dir, prefix, opt_builtins):
+    vis = QAPISchemaGenRsSysTypeVisitor(prefix)
+    schema.visit(vis)
+    vis.write(output_dir)
diff --git a/scripts/qapi/rs_types.py b/scripts/qapi/rs_types.py
new file mode 100644
index 0000000000..8b22605bf9
--- /dev/null
+++ b/scripts/qapi/rs_types.py
@@ -0,0 +1,281 @@
+"""
+QAPI Rust types generator
+"""
+
+from qapi.common import *
+from qapi.rs import QAPISchemaRsVisitor, rs_systype, rs_name, from_qemu, rs_type, from_list
+
+
+objects_seen = set()
+
+
+def gen_rs_object(name, ifcond, base, members, variants):
+    if name in objects_seen:
+        return ''
+
+    ret = ''
+    objects_seen.add(name)
+    has_options = False
+    for memb in members:
+        if memb.optional:
+            has_options = True
+
+    if variants:
+        ret += 'variants TODO'
+
+    derive = 'Serialize, Deserialize, Type'
+    serde = 'serde'
+    if has_options:
+        derive = 'SerializeDict, DeserializeDict, TypeDict'
+        serde = 'zvariant'
+
+    ret += mcgen('''
+
+#[derive(Clone, Debug)]
+#[cfg_attr(feature = "dbus", derive(%(derive)s))]
+pub struct %(rs_name)s {
+''',
+                 rs_name=rs_name(name), derive=derive)
+
+    if base:
+        ret += 'Base TODO'
+
+    memb_names = []
+    for memb in members:
+        memb_names.append(rs_name(memb.name))
+        rsname = rs_name(memb.name)
+        if rsname != memb.name:
+            ret += mcgen('''
+   #[cfg_attr(feature = "dbus", %(serde)s(rename = "%(name)s"))]
+''', serde=serde, name=memb.name)
+
+        ret += mcgen('''
+    pub %(rs_name)s: %(rs_type)s,
+''',
+                     rs_type=rs_type(memb.type.c_type(), '', optional=memb.optional), rs_name=rsname)
+
+    ret += mcgen('''
+}
+
+impl FromQemuPtrFull<*mut qapi_sys::%(rs_name)s> for %(rs_name)s {
+    unsafe fn from_qemu_full(sys: *mut qapi_sys::%(rs_name)s) -> Self {
+        let ret = from_qemu_none(sys as *const _);
+        qapi_sys::qapi_free_%(rs_name)s(sys);
+        ret
+    }
+}
+
+impl FromQemuPtrNone<*const qapi_sys::%(rs_name)s> for %(rs_name)s {
+    unsafe fn from_qemu_none(sys: *const qapi_sys::%(rs_name)s) -> Self {
+        let sys = & *sys;
+''', rs_name=rs_name(name))
+
+    for memb in members:
+        memb_name = rs_name(memb.name)
+        val = from_qemu('sys.' + memb_name, memb.type.c_type())
+        if memb.optional:
+            val = mcgen('''{
+            if sys.has_%(memb_name)s {
+                Some(%(val)s)
+            } else {
+                None
+            }
+}''', memb_name=memb_name, val=val)
+
+        ret += mcgen('''
+        let %(memb_name)s = %(val)s;
+''', memb_name=memb_name, val=val)
+
+    ret += mcgen('''
+            Self { %(memb_names)s }
+        }
+}
+''', rs_name=rs_name(name), memb_names=', '.join(memb_names))
+
+    storage = []
+    stash = []
+    sys_memb = []
+    memb_none = ''
+    memb_full = ''
+    for memb in members:
+        memb_name = rs_name(memb.name)
+        c_type = memb.type.c_type()
+        is_pointer = c_type.endswith(pointer_suffix)
+        if is_pointer:
+            t = rs_type(memb.type.c_type(), optional=memb.optional, ns='')
+            p = rs_systype(memb.type.c_type())
+            s = "translate::Stash<'a, %s, %s>" % (p, t)
+            storage.append(s)
+        if memb.optional:
+            sys_memb.append('has_%s' % memb_name)
+            has_memb = mcgen('''
+    let has_%(memb_name)s = self.%(memb_name)s.is_some();
+''', memb_name=memb_name)
+            memb_none += has_memb
+            memb_full += has_memb
+
+        to_qemu = ''
+        if is_pointer:
+            memb_none += mcgen('''
+    let %(memb_name)s_stash_ = self.%(memb_name)s.to_qemu_none();
+    let %(memb_name)s = %(memb_name)s_stash_.0;
+''', memb_name=memb_name)
+            stash.append('%s_stash_' % memb_name)
+            memb_full += mcgen('''
+    let %(memb_name)s = self.%(memb_name)s.to_qemu_full();
+''', memb_name=memb_name)
+        else:
+            unwrap = ''
+            if memb.optional:
+                unwrap = '.unwrap_or_default()'
+            memb = mcgen('''
+    let %(memb_name)s = self.%(memb_name)s%(unwrap)s;
+''', memb_name=memb_name, unwrap=unwrap)
+            memb_none += memb
+            memb_full += memb
+
+        sys_memb.append(memb_name)
+
+    ret += mcgen('''
+
+impl translate::QemuPtrDefault for %(rs_name)s {
+    type QemuType = *mut qapi_sys::%(rs_name)s;
+}
+
+impl<'a> translate::ToQemuPtr<'a, *mut qapi_sys::%(rs_name)s> for %(rs_name)s {
+    type Storage = (Box<qapi_sys::%(rs_name)s>, %(storage)s);
+
+    #[inline]
+    fn to_qemu_none(&'a self) -> translate::Stash<'a, *mut qapi_sys::%(rs_name)s, %(rs_name)s> {
+        %(memb_none)s
+        let mut box_ = Box::new(qapi_sys::%(rs_name)s { %(sys_memb)s });
+
+        translate::Stash(&mut *box_, (box_, %(stash)s))
+    }
+
+    #[inline]
+    fn to_qemu_full(&self) -> *mut qapi_sys::%(rs_name)s {
+        unsafe {
+            %(memb_full)s
+            let ptr = qemu_sys::g_malloc0(std::mem::size_of::<*const %(rs_name)s>()) as *mut _;
+            *ptr = qapi_sys::%(rs_name)s { %(sys_memb)s };
+            ptr
+        }
+    }
+}
+''', rs_name=rs_name(name), storage=', '.join(storage),
+                 sys_memb=', '.join(sys_memb), memb_none=memb_none, memb_full=memb_full, stash=', '.join(stash))
+
+    return ret
+
+
+def gen_rs_variant(name, ifcond, variants):
+    if name in objects_seen:
+        return ''
+
+    ret = ''
+    objects_seen.add(name)
+
+    ret += mcgen('''
+
+// Implement manual Value conversion (other option via some zbus macros?)
+#[cfg(feature = "dbus")]
+impl zvariant::Type for %(rs_name)s {
+    fn signature() -> zvariant::Signature<'static> {
+        zvariant::Value::signature()
+    }
+}
+
+#[derive(Clone,Debug)]
+#[cfg_attr(feature = "dbus", derive(Deserialize, Serialize), serde(into = "zvariant::OwnedValue", try_from = "zvariant::OwnedValue"))]
+pub enum %(rs_name)s {
+''',
+                 rs_name=rs_name(name))
+
+    for var in variants.variants:
+        if var.type.name == 'q_empty':
+            continue
+        ret += mcgen('''
+        %(mem_name)s(%(rs_type)s),
+''',
+                     rs_type=rs_type(var.type.c_unboxed_type(), ''),
+                     mem_name=to_camel_case(rs_name(var.name)))
+    ret += mcgen('''
+}
+''')
+    return ret
+
+
+class QAPISchemaGenRsTypeVisitor(QAPISchemaRsVisitor):
+
+    def __init__(self, prefix):
+        super().__init__(prefix, 'qapi-types')
+
+    def visit_begin(self, schema):
+        # gen_object() is recursive, ensure it doesn't visit the empty type
+        objects_seen.add(schema.the_empty_object_type.name)
+        self._gen.preamble_add(
+            mcgen('''
+// generated by qapi-gen, DO NOT EDIT
+'''))
+
+    def visit_end(self):
+        for c_type in from_list:
+            sys = rs_systype(c_type, sys_ns='')[5:]
+            rs = rs_type(c_type, ns='')
+
+            self._gen.add(mcgen('''
+
+impl FromQemuPtrFull<*mut qapi_sys::%(sys)s> for %(rs)s {
+    #[inline]
+    unsafe fn from_qemu_full(sys: *mut qapi_sys::%(sys)s) -> Self {
+        let ret = from_qemu_none(sys as *const _);
+        qapi_sys::qapi_free_%(sys)s(sys);
+        ret
+    }
+}
+
+impl FromQemuPtrNone<*const qapi_sys::%(sys)s> for %(rs)s {
+    #[inline]
+    unsafe fn from_qemu_none(sys: *const qapi_sys::%(sys)s) -> Self {
+         let mut ret = vec![];
+         let mut it = sys;
+         while !it.is_null() {
+             let e = &*it;
+             ret.push(translate::from_qemu_none(e.value as *const _));
+             it = e.next;
+         }
+         ret
+    }
+}
+''', sys=sys, rs=rs))
+
+    def visit_command(self, name, info, ifcond, features,
+                      arg_type, ret_type, gen, success_response, boxed,
+                      allow_oob, allow_preconfig):
+        if not gen:
+            return
+        # hack: eventually register a from_list
+        if ret_type:
+            from_qemu('', ret_type.c_type())
+
+    def visit_object_type(self, name, info, ifcond, features,
+                          base, members, variants):
+        if name.startswith('q_'):
+            return
+        self._gen.add(gen_rs_object(name, ifcond, base, members, variants))
+
+    def visit_enum_type(self, name, info, ifcond, features, members, prefix):
+        self._gen.add(mcgen('''
+
+pub type %(rs_name)s = qapi_sys::%(rs_name)s;
+''', rs_name=rs_name(name)))
+
+    def visit_alternate_type(self, name, info, ifcond, features, variants):
+        self._gen.add(gen_rs_variant(name, ifcond, variants))
+
+
+def gen_rs_types(schema, output_dir, prefix, opt_builtins):
+    vis = QAPISchemaGenRsTypeVisitor(prefix)
+    schema.visit(vis)
+    vis.write(output_dir)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 78309a00f0..da48210509 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -167,8 +167,14 @@ class QAPISchemaType(QAPISchemaEntity):
         pass
 
     # Return the C type to be used in a parameter list.
-    def c_param_type(self):
-        return self.c_type()
+    #
+    # The argument should be considered const, since no ownership is given to the callee,
+    # but qemu C code frequently tweaks it. Set const=True for a stricter declaration.
+    def c_param_type(self, const=False):
+        c_type = self.c_type()
+        if const and c_type.endswith(pointer_suffix):
+            c_type = 'const ' + c_type
+        return c_type
 
     # Return the C type to be used where we suppress boxing.
     def c_unboxed_type(self):
@@ -221,10 +227,10 @@ class QAPISchemaBuiltinType(QAPISchemaType):
     def c_type(self):
         return self._c_type_name
 
-    def c_param_type(self):
+    def c_param_type(self, const=False):
         if self.name == 'str':
             return 'const ' + self._c_type_name
-        return self._c_type_name
+        return super().c_param_type(const)
 
     def json_type(self):
         return self._json_type_name
-- 
2.26.2

On Thu, Sep 10, 2020 at 7:49 PM <marcandre.lureau@redhat.com> wrote:
> The usage is relatively simple:
>
> - from_qemu_none(ptr: *const sys::P) -> T
>   Return a Rust type T for a const ffi pointer P.
>
> - from_qemu_full(ptr: *mut sys::P) -> T
>   Return a Rust type T for a ffi pointer P, taking ownership.
>
> - T::to_qemu_none() -> Stash<P>
>   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
>   storage data, if any).
>
> - T::to_qemu_full() -> P
>   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)

I know these come from glib-rs, but still the names are awful. :)

What about:

- an unsafe variant of From/Into for from_qemu_full:

    trait UnsafeFrom<T> {
        unsafe fn unsafe_from(_: T) -> Self;
    }
    trait UnsafeInto<T> {
        unsafe fn unsafe_into(self) -> T;
    }
    impl <T, U> UnsafeInto<U> for T where U: UnsafeFrom<T> {
        unsafe fn unsafe_into(self) -> U { U::unsafe_from(self) }
    }

Example:

    impl UnsafeFrom<*mut c_char> for String {
        unsafe fn unsafe_from(ptr: *mut c_char) -> Self {
            let res = Self::new_from_foreign(ptr);
            libc::free(ptr as *mut c_void);
            res
        }
    }

- likewise, a generic IntoRaw trait for to_qemu_full:

    trait IntoRaw<T> {
        fn into_raw(self) -> *mut T;
    }

Example:

    impl IntoRaw<c_char> for String {
        fn into_raw(self) -> *mut c_char {
            unsafe {
                libc::strndup(self.as_ptr() as *const c_char,
                              self.len() as size_t)
            }
        }
    }

- and a simpler/nicer version of Stash, from_qemu_none and to_qemu_none like this:

    pub struct BorrowedPointer<'a, P, T: 'a> {
        pub native: *const P,
        pub storage: T,
        _marker: PhantomData<&'a T>,
    }

    impl<'a, P: Copy, T: 'a> BorrowedPointer<'a, P, T> {
        fn new(native: *const P, storage: T) -> Self {
            BorrowedPointer {
                native,
                storage,
                _marker: PhantomData
            }
        }

        fn as_ptr(&self) -> *const P {
            self.native
        }
    }

    trait ForeignConvertible<'a> {
        type Native: Copy;
        type Storage: 'a;
         unsafe fn new_from_foreign(p: *const Self::Native) -> Self;
        fn as_foreign(&'a self) -> BorrowedPointer<'a, Self::Native, Self::Storage>;
    }

Implemented like this:

    impl ForeignConvertible<'_> for String {
        type Native = c_char;
        type Storage = CString;

        unsafe fn new_from_foreign(p: *const c_char) -> Self {
            let cstr = CStr::from_ptr(p);
            String::from_utf8_lossy(cstr.to_bytes()).into_owned()
        }
        fn as_foreign(&self) -> BorrowedPointer<c_char, CString> {
            let tmp = CString::new(&self[..]).unwrap();
            BorrowedPointer::new(tmp.as_ptr(), tmp)
        }
    }

and possibly:

    impl<'a, P: Copy, T: 'a> BorrowedMutPointer<'a, P, T> {
        fn new(native: *mut P, storage: T) -> Self {
            BorrowedMutPointer {
                native,
                storage,
                _marker: PhantomData
            }
        }

        fn as_ptr(&self) -> *const P {
            self.native
        }

        fn as_mut_ptr(&mut self) -> *mut P {
            self.native
        }
    }

    trait ForeignMutConvertible<'a>: ForeignConvertible<'a> {
      fn as_foreign_mut(&self) -> BorrowedMutPointer<Self::Native, Self::Storage>;
    }

I placed the source code for the above at https://github.com/bonzini/rust-ptr

I'll look later at the rest of the code.  It's quite big. :)

Paolo

On 11/09/20 12:24, Paolo Bonzini wrote:
>>
>> - from_qemu_none(ptr: *const sys::P) -> T
>>   Return a Rust type T for a const ffi pointer P.
>>
>> - from_qemu_full(ptr: *mut sys::P) -> T
>>   Return a Rust type T for a ffi pointer P, taking ownership.
>>
>> - T::to_qemu_none() -> Stash<P>
>>   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
>>   storage data, if any).
>>
>> - T::to_qemu_full() -> P
>>   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
>
> I know these come from glib-rs, but still the names are awful. :)

After studying a bit I managed to give a rational explanation of the
above gut feeling.  Conversions in Rust usually follow this naming
convention:

	Name		  Type			Price
	--------	  --------------------	-----------
	as_*, borrow	  Borrowed -> Borrowed	Cheap
	to_*, constructor Borrowed -> Owned	Expensive
	from, into_*	  Owned -> Owned	Any

and we have

	from_qemu_none	  Borrowed -> Owned
	to_qemu_none	  Borrowed -> Borrowed
	from_qemu_full	  Owned -> Owned
	to_qemu_full	  Owned -> Owned

So

- from_qemu_none should be a "to_*" or "constructor" conversion (I used
new_from_foreign)

- to_qemu_none, even though in some cases it can be expensive, should be
an "as_*" conversion (as_foreign).

- from_qemu_full and to_qemu_full should be "from" or "into_*"

to_qemu_full() could also be split into a more expensive but flexible
"to" variant and the cheaper "into" variant.  Just for the sake of
example, I updated my demo by replacing the IntoRaw trait with these two:

    trait ToForeign<T> {
        fn to_foreign(&self) -> *mut T;
    }

    trait IntoForeign<T> {
        fn into_foreign(self) -> *mut T;
    }

where the example implementations show the different cost quite clearly:

    impl ToForeign<c_char> for String {
        fn to_foreign(&self) -> *mut c_char {
            unsafe { libc::strndup(self.as_ptr() as *const c_char,
                                   self.len() as size_t) }
        }
    }

    impl IntoForeign<c_char> for String {
        fn into_foreign(self) -> *mut c_char {
            let ptr = self.as_ptr();
            forget(self);
            ptr as *mut _
        }
    }

Thanks,

Paolo

Hi

On Fri, Sep 11, 2020 at 5:08 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 11/09/20 12:24, Paolo Bonzini wrote:
> >>
> >> - from_qemu_none(ptr: *const sys::P) -> T
> >>   Return a Rust type T for a const ffi pointer P.
> >>
> >> - from_qemu_full(ptr: *mut sys::P) -> T
> >>   Return a Rust type T for a ffi pointer P, taking ownership.
> >>
> >> - T::to_qemu_none() -> Stash<P>
> >>   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
> >>   storage data, if any).
> >>
> >> - T::to_qemu_full() -> P
> >>   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
> >
> > I know these come from glib-rs, but still the names are awful. :)
>
> After studying a bit I managed to give a rational explanation of the
> above gut feeling.  Conversions in Rust usually follow this naming
> convention:
>
>         Name              Type                  Price
>         --------          --------------------  -----------
>         as_*, borrow      Borrowed -> Borrowed  Cheap
>         to_*, constructor Borrowed -> Owned     Expensive
>         from, into_*      Owned -> Owned        Any
>
> and we have
>
>         from_qemu_none    Borrowed -> Owned
>         to_qemu_none      Borrowed -> Borrowed
>         from_qemu_full    Owned -> Owned
>         to_qemu_full      Owned -> Owned
>
> So
>
> - from_qemu_none should be a "to_*" or "constructor" conversion (I used
> new_from_foreign)
>

new_ prefix is not very rusty.

from_raw() is common, and takes ownership.
from_ptr() (as in CStr) could be a candidate (although it wouldn't have the
same lifetime requirements)


> - to_qemu_none, even though in some cases it can be expensive, should be
> an "as_*" conversion (as_foreign).
>

as_ptr(), but since it's more complicated than just a pointer due to
temporary storage, it's not the best fit either...


> - from_qemu_full and to_qemu_full should be "from" or "into_*"
>

ok


>
> to_qemu_full() could also be split into a more expensive but flexible
> "to" variant and the cheaper "into" variant.  Just for the sake of
> example, I updated my demo by replacing the IntoRaw trait with these two:
>
>     trait ToForeign<T> {
>         fn to_foreign(&self) -> *mut T;
>     }
>
>     trait IntoForeign<T> {
>         fn into_foreign(self) -> *mut T;
>     }
>
> where the example implementations show the different cost quite clearly:
>
>     impl ToForeign<c_char> for String {
>         fn to_foreign(&self) -> *mut c_char {
>             unsafe { libc::strndup(self.as_ptr() as *const c_char,
>                                    self.len() as size_t) }
>         }
>     }
>
>     impl IntoForeign<c_char> for String {
>         fn into_foreign(self) -> *mut c_char {
>             let ptr = self.as_ptr();
>             forget(self);
>             ptr as *mut _
>         }
>     }
>
>
You corrected the \0-ending in the git tree. However, the memory allocator
(or the stack) may not be compatible with the one used in C.

 As for the general comparison with glib-rs traits, it's hard for me to say
upfront the pros and cons, I would need to modify this PoC for example. But
I must say I feel quite comfortable with the glib approach. It would be
nice to have some feedback from glib-rs maintainers about your proposal.

(my gut feeling is that we would be better sticking with something close to
glib, as it is likely we will end up using glib-rs at some point, and
having similar patterns should help each other)

On 11/09/20 16:00, Marc-André Lureau wrote:
>     - from_qemu_none should be a "to_*" or "constructor" conversion (I used
>     new_from_foreign)
> 
> new_ prefix is not very rusty.

Right, I have changed it to with_foreign so now there is
{as,with,to,into}_foreign, plus unsafe_{from,into}.

These two could even be renamed to from_foreign and into_native at the
cost of making the trait less general purpose.  This way we have the
typical Rust names: as_* for Borrowed->Borrowed, with_*/to_* for
Borrowed->Owned, from_*/into_* for Owned->Owned.

> However, the memory allocator (or the stack) may not be compatible
> with the one used in C.

Hmm, that's a good point.  The simplest solution might be just to get
rid of IntoForeign, it's just an optimization.

> from_raw() is common, and takes ownership.

from_raw()/into_raw() would be equivalent to
into_foreign()/from_foreign().  However as you point out the allocators
are different, so it's a good idea IMHO to separate
into_raw()/from_raw() for the Rust allocator from
into_foreign()/from_foreign() for the libc allocator.

> I would need to modify this PoC for example

Yes of course.  Can you just try splitting the PoC in multiple patches?
 That should also make it easier to review, so far all I did was
comparing against glib-rs.

> But I must say I feel quite comfortable with the glib approach. It
> would be nice to have some feedback from glib-rs maintainers about your
> proposal.

QAPI is not tied to glib-rs, so I don't think qemu-ga will need to use
glib-rs.  I think either we use glib-rs, or if we are to roll our own we
should not be tied to the naming.  We don't use GObject introspection,
so none/full means nothing to most QEMU developers (and to Rust
developers too).

There are other things I don't like very much in glib-rs, for example
the use of tuples and public fields and the somewhat messy usage of
*const/*mut (I tried to be stricter on that).

Paolo

Hi

On Fri, Sep 11, 2020 at 7:17 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 11/09/20 16:00, Marc-André Lureau wrote:
> >     - from_qemu_none should be a "to_*" or "constructor" conversion (I
> used
> >     new_from_foreign)
> >
> > new_ prefix is not very rusty.
>
> Right, I have changed it to with_foreign so now there is
> {as,with,to,into}_foreign, plus unsafe_{from,into}.
>
> These two could even be renamed to from_foreign and into_native at the
> cost of making the trait less general purpose.  This way we have the
> typical Rust names: as_* for Borrowed->Borrowed, with_*/to_* for
> Borrowed->Owned, from_*/into_* for Owned->Owned.
>
> > However, the memory allocator (or the stack) may not be compatible
> > with the one used in C.
>
> Hmm, that's a good point.  The simplest solution might be just to get
> rid of IntoForeign, it's just an optimization.
>
> > from_raw() is common, and takes ownership.
>
> from_raw()/into_raw() would be equivalent to
> into_foreign()/from_foreign().  However as you point out the allocators
> are different, so it's a good idea IMHO to separate
> into_raw()/from_raw() for the Rust allocator from
> into_foreign()/from_foreign() for the libc allocator.
>
> > I would need to modify this PoC for example
>
> Yes of course.  Can you just try splitting the PoC in multiple patches?
>  That should also make it easier to review, so far all I did was
> comparing against glib-rs.
>
> > But I must say I feel quite comfortable with the glib approach. It
> > would be nice to have some feedback from glib-rs maintainers about your
> > proposal.
>
> QAPI is not tied to glib-rs, so I don't think qemu-ga will need to use
> glib-rs.  I think either we use glib-rs, or if we are to roll our own we
> should not be tied to the naming.  We don't use GObject introspection,
> so none/full means nothing to most QEMU developers (and to Rust
> developers too).
>
> There are other things I don't like very much in glib-rs, for example
> the use of tuples and public fields and the somewhat messy usage of
> *const/*mut (I tried to be stricter on that).
>
>
I am trying to wrap my head around your proposal (based on
https://github.com/bonzini/rust-ptr), and trying to understand the
limitations/unrustiness of the glib-rs translate traits I used in this PoC.

First let's clarify the requirements. We need those conversions for now:
- const *P -> T
- mut *P -> T
And:
- &T -> const *P
- &T -> mut *P

Note that glib-rs has more advanced conversions, because of partial
ownership transfer with containers, and ref-counted types etc. Those could
soon become necessary for QEMU to bind other types than QAPI, in particular
QOM and our usage of glib in general. I kept that in mind by carefully
choosing glib-rs as a reference. I think it's important to take it into
account from the start (sadly, some limitations don't allow us to simply
use glib-rs traits, for reasons that aren't 100% clear to me, but are clear
to the compiler and others :)

Some other remarks:
- "mut *P -> T" is often just "const *P -> T" with P being freed after
conversion
- "&T -> const *P" can be "&T -> mut *P" with Rust side freeing P after
usage thanks to a stash, but can also be very different and not require it
(strings for example, the constP uses CString, while the mutP version is
just a g_strndup)
- it is nice (or necessary) to have to allow some form of composition for
container-like types (Option<T>, Vec<T>, struct T(U,V) inside etc) to avoid
duplication
- Rust naming conventions guide us towards using to_ and into_ (for
owned->owned) prefixes.


The glib-rs traits map the conversion functions respectively to (I removed
the Glib/Qemu prefix, because the subset used in both are very close):
- FromPtrNone::from_none
- FromPtrFull::from_full (usually just calls from_none() and free(P))
And:
- ToPtr::to_none (with the Stash)
- ToPtr::to_full

The symmetry is clear, and arguably easy to remember. fwiw, I don't know
why ToPtr wasn't split the same way FromPtr was (they used to be on the
same FromPtr trait).

The usage of to_ prefix is in accordance with the Rust conventions here.
The usage of from_ is perhaps not ideal?, but from_full is not incompatible
with the symmetrical into_ (as in From<T> for U implies Into<U> for T).

Experience shows that the combination of Stash & ToPtr design makes it
convenient for type composition too.


My understanding of what you propose is:
- ForeignConvert::with_foreign
- FromForeign::from_foreign (with implied into_native)
And:
- ForeignConvert::as_foreign (with the BorrowedPointer/stash-like)
- ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems
wrongly designed in your proposal and unnecessary for now)

I excluded IntoForeign::into_foreign, since "T -> P" can't really be done
better than "&T -> *P" due to different memory allocators etc.

I don't have your head, so I find it hard to remember & work with. It uses
all possible prefixes: with_, from_, as_, as_mut, to_, and into_. That just
blows my mind, sorry :)

Then, I don't understand why ForeignConvert should hold both the "const *P
-> T" and "&T -> const *P" conversions. Except the common types, what's the
relation between the two?

Finally, I thought you introduced some differences with the stash design,
but in fact I can see that ForeignConvert::Storage works just the way as
ToPtr::Storage. So composition should be similar. Only your example code is
more repetitive as it doesn't indirectly refer to the trait Storage the
same way as glib-rs does (via <T as ToPtr>::Storage).

I am not making any conclusions yet, but I am not exactly happily going to
switch to your proposal yet :)

Comments?






-- 
Marc-André Lureau

On 29/09/20 19:55, Marc-André Lureau wrote:
> My understanding of what you propose is:
> - ForeignConvert::with_foreign
> - FromForeign::from_foreign (with implied into_native)
> And:
> - ForeignConvert::as_foreign (with the BorrowedPointer/stash-like)
> - ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems
> wrongly designed in your proposal and unnecessary for now)

Might well be, but how is it wrong?  (I'd like to improve).

> I don't have your head, so I find it hard to remember & work with. It> uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_.
> That just blows my mind, sorry :)

Ahah I don't have your head either!  The idea anyway is to reuse
prefixes that are common in Rust code:

* with_: a constructor that uses something to build a type (think
Vec::with_capacity) and therefore takes ownership

* as_: a cheap conversion to something, it's cheap because it reuses the
lifetime (and therefore takes no ownership).  Think Option::as_ref.

* from_/to_: a copying and possibly expensive conversion (that you have
to write the code for).  Because it's copying, it doesn't consume the
argument (for from_) or self (for to_).

* into_: a conversion that consumes the receiver

It may well be over the top.

> Then, I don't understand why ForeignConvert should hold both the "const
> *P -> T" and "&T -> const *P" conversions. Except the common types,
> what's the relation between the two?

Maybe I'm wrong, but why would you need just one?

> Finally, I thought you introduced some differences with the stash
> design, but in fact I can see that ForeignConvert::Storage works just
> the way as ToPtr::Storage. So composition should be similar. Only your
> example code is more repetitive as it doesn't indirectly refer to the
> trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage).

Yes, that's the main difference.  I removed Storage because I didn't
want to force any trait on BorrowedPointer's second type argument.  It
seemed like a generic concept to me.

The other difference is that Stash is a tuple while BorrowedPointer is a
struct and has methods to access it.  Stash seems very ugly to use.

> I am not making any conclusions yet, but I am not exactly happily going
> to switch to your proposal yet :)

Sure, no problem.

Paolo

Hi

On Tue, Sep 29, 2020 at 10:23 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 29/09/20 19:55, Marc-André Lureau wrote:
> > My understanding of what you propose is:
> > - ForeignConvert::with_foreign
> > - FromForeign::from_foreign (with implied into_native)
> > And:
> > - ForeignConvert::as_foreign (with the BorrowedPointer/stash-like)
> > - ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems
> > wrongly designed in your proposal and unnecessary for now)
>
> Might well be, but how is it wrong?  (I'd like to improve).
>

Why BorrowedMutPointer provides both *const P and *mut P ? The *const P
conversion seems overlapping with BorrowedPointer.

Anyway, the &mut T -> *mut P conversion seems fairly rare to me and
error-prone. You usually give up ownership if you let the foreign function
tweak the P.

In any case, we don't need such conversion for QAPI, for now.


> > I don't have your head, so I find it hard to remember & work with. It>
> uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_.
> > That just blows my mind, sorry :)
>
> Ahah I don't have your head either!  The idea anyway is to reuse
> prefixes that are common in Rust code:
>
> * with_: a constructor that uses something to build a type (think
> Vec::with_capacity) and therefore takes ownership
>


ForeignConvert::with_foreign (const *P -> T) doesn't take ownership.

The Rust reference for this kind of conversion is CStr::from_ptr.


> * as_: a cheap conversion to something, it's cheap because it reuses the
> lifetime (and therefore takes no ownership).  Think Option::as_ref.
>

as_ shouldn't create any object, and is thus unsuitable for a general
rs<->sys conversion function (any).

* from_/to_: a copying and possibly expensive conversion (that you have
> to write the code for).  Because it's copying, it doesn't consume the
> argument (for from_) or self (for to_).
>
>
and that's what glib-rs uses (and CStr).



> * into_: a conversion that consumes the receiver
>
>
That's not used by glib afaik, but we should be able to introduce it for
"mut *P -> T", it's not incompatible with FromPtrFull::from_full.

In general, I like the fact that the conversion traits are associated to T,
and not to P (which can remain a bare pointer, without much associated
methods).

It may well be over the top.
>
> > Then, I don't understand why ForeignConvert should hold both the "const
> > *P -> T" and "&T -> const *P" conversions. Except the common types,
> > what's the relation between the two?
>
> Maybe I'm wrong, but why would you need just one?
>

No I mean they could be on different traits. One could be implemented
without the other. Or else I don't understand why the other conversion
functions would not be in that trait too.


> > Finally, I thought you introduced some differences with the stash
> > design, but in fact I can see that ForeignConvert::Storage works just
> > the way as ToPtr::Storage. So composition should be similar. Only your
> > example code is more repetitive as it doesn't indirectly refer to the
> > trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage).
>
> Yes, that's the main difference.  I removed Storage because I didn't
> want to force any trait on BorrowedPointer's second type argument.  It
> seemed like a generic concept to me.
>

To the cost of some duplication. I like the coupling between the traits
better. If you need a similar tuple/struct elsewhere, it's easy to make
your own.

The Storage type can quickly become quite complicated with QAPI, I would
rather avoid having to repeat it, it would create hideous compiler errors
too.


> The other difference is that Stash is a tuple while BorrowedPointer is a
> struct and has methods to access it.  Stash seems very ugly to use.
>

Yes I agree. Not sure why they made it a bare tuple, laziness perhaps :).


-- 
Marc-André Lureau

On 30/09/20 11:15, Marc-André Lureau wrote:
> Why BorrowedMutPointer provides both *const P and *mut P ? The *const P
> conversion seems overlapping with BorrowedPointer.

"&mut T" implements Borrow so it seemed obvious to have as_ptr in
BorrowedMutPointer too.  Though I certainly should implement Borrow in
BorrowedMutPointer.

>     > I don't have your head, so I find it hard to remember & work with.
>     It> uses all possible prefixes: with_, from_, as_, as_mut, to_, and
>     into_.
>     > That just blows my mind, sorry :)
> 
>     Ahah I don't have your head either!  The idea anyway is to reuse
>     prefixes that are common in Rust code:
> 
>     * with_: a constructor that uses something to build a type (think
>     Vec::with_capacity) and therefore takes ownership
> 
> ForeignConvert::with_foreign (const *P -> T) doesn't take ownership.
> 
> The Rust reference for this kind of conversion is CStr::from_ptr.

Ok, I'll take a look.

>     * as_: a cheap conversion to something, it's cheap because it reuses the
>     lifetime (and therefore takes no ownership).  Think Option::as_ref.
> 
> as_ shouldn't create any object, and is thus unsuitable for a general
> rs<->sys conversion function (any).

as_foreign function does not create anything, it reuses the storage to
provide a pointer.  It seems similar to as_slice for example.

>     * from_/to_: a copying and possibly expensive conversion (that you have
>     to write the code for).  Because it's copying, it doesn't consume the
>     argument (for from_) or self (for to_).
> 
> and that's what glib-rs uses (and CStr).

Sort of, I found the none/full suffixes not really idiomatic for Rust.

>     * into_: a conversion that consumes the receiver
> 
> That's not used by glib afaik, but we should be able to introduce it for
> "mut *P -> T", it's not incompatible with FromPtrFull::from_full.

Right.  It's just a different way to write the same thing.  Usually it
is a bit more concise because it allows more type inference.

>     > Then, I don't understand why ForeignConvert should hold both the
>     > "const *P -> T" and "&T -> const *P" conversions. Except the
>     > common types, what's the relation between the two?
> 
>     Maybe I'm wrong, but why would you need just one?
> 
> No I mean they could be on different traits. One could be implemented
> without the other. Or else I don't understand why the other conversion
> functions would not be in that trait too.

The other conversion functions require taking ownership, and I was not
sure if it would always be possible to do so.  For no-ownership-taken
conversions, however, it seemed to me that you'd rarely be unable to
implement one of the two directions.  I might be wrong.

In general though I agree that the changes are mostly cosmetic.

Paolo

On Thu, Sep 10, 2020 at 09:48:50PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Among the QEMU developers, there is a desire to use Rust. (see previous
> thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm
> related projects and other experiments).
> 
> Thanks to our QAPI type system and the associate code generator, it is
> relatively straightforward to create Rust bindings for the generated C
> types (also called sys/ffi binding) and functions. (rust-bindgen could
> probably do a similar job, but it would probably bring other issues).
> This provides an important internal API already.
> 
> Slightly more complicated is to expose a Rust API for those, and provide
> convenient conversions C<->Rust. Taking inspiration from glib-rs
> binding, I implemented a simplified version of the FromGlib/ToGlib
> traits, with simpler ownership model, sufficient for QAPI needs.
> 
> The usage is relatively simple:
> 
> - from_qemu_none(ptr: *const sys::P) -> T
>   Return a Rust type T for a const ffi pointer P.
> 
> - from_qemu_full(ptr: *mut sys::P) -> T
>   Return a Rust type T for a ffi pointer P, taking ownership.
> 
> - T::to_qemu_none() -> Stash<P>
>   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
>   storage data, if any).
> 
> - T::to_qemu_full() -> P
>   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
> 
> With those traits, it's relatively easy to implement the QMP callbacks.
> With enough interest, we could eventually start rewriting QGA in
> Rust, as it is a simple service. See qga/qmp.rs for some examples.
> We could also try to tackle qemu itself.
> 
> Finally, given that the QAPI types are easy to serialize, it was simple
> to use "serde" on them, and provide a D-Bus interface for QMP with zbus.
> (a similar approach could probably be taken for other protocols, that
> could be dynamically loaded... anyone like protobuf better?)
> 
> This PoC modifies qemu-ga to provide the interface on the session bus:
> $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v
> $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> ...
> $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1
> ...
> 
> Note: the generated code doesn't work with the qemu schema, there is a
> couple of fixme/todo left.
> 
> Shameful pain point: meson & cargo don't play nicely together.

Do we actually need/want it to be in the same monolithic repo
as qemu, as opposed to a qemu-qapi-rust repo ?

Trying to weld together different build systems is never that
attractive. The language specific build systems generally are
much simpler if they're self contained. From a distro POV it
can be better if the language bindings are self contained, as
you don't neccessarily want to build the language binding in
the same environment as the main app. For example with modules
in Fedora or RHEL, there can be multiple parallel versions of
a language runtime, and thus language bindings would be built
separately from QEMU.

IIUC, you're generating stuff from the QEMU schemas. One way
we can deal with this is to actually install the QEMU schemas
into /usr/share. Distros would have an "qemu-devel" package
that provided the schemas and the QAPI base tools which
can then be used by separate bindings.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Hi

On Fri, Sep 11, 2020 at 2:47 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Sep 10, 2020 at 09:48:50PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Hi,
> >
> > Among the QEMU developers, there is a desire to use Rust. (see previous
> > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm
> > related projects and other experiments).
> >
> > Thanks to our QAPI type system and the associate code generator, it is
> > relatively straightforward to create Rust bindings for the generated C
> > types (also called sys/ffi binding) and functions. (rust-bindgen could
> > probably do a similar job, but it would probably bring other issues).
> > This provides an important internal API already.
> >
> > Slightly more complicated is to expose a Rust API for those, and provide
> > convenient conversions C<->Rust. Taking inspiration from glib-rs
> > binding, I implemented a simplified version of the FromGlib/ToGlib
> > traits, with simpler ownership model, sufficient for QAPI needs.
> >
> > The usage is relatively simple:
> >
> > - from_qemu_none(ptr: *const sys::P) -> T
> >   Return a Rust type T for a const ffi pointer P.
> >
> > - from_qemu_full(ptr: *mut sys::P) -> T
> >   Return a Rust type T for a ffi pointer P, taking ownership.
> >
> > - T::to_qemu_none() -> Stash<P>
> >   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
> >   storage data, if any).
> >
> > - T::to_qemu_full() -> P
> >   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
> >
> > With those traits, it's relatively easy to implement the QMP callbacks.
> > With enough interest, we could eventually start rewriting QGA in
> > Rust, as it is a simple service. See qga/qmp.rs for some examples.
> > We could also try to tackle qemu itself.
> >
> > Finally, given that the QAPI types are easy to serialize, it was simple
> > to use "serde" on them, and provide a D-Bus interface for QMP with zbus.
> > (a similar approach could probably be taken for other protocols, that
> > could be dynamically loaded... anyone like protobuf better?)
> >
> > This PoC modifies qemu-ga to provide the interface on the session bus:
> > $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v
> > $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> > ...
> > $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> > GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1
> > ...
> >
> > Note: the generated code doesn't work with the qemu schema, there is a
> > couple of fixme/todo left.
> >
> > Shameful pain point: meson & cargo don't play nicely together.
>
> Do we actually need/want it to be in the same monolithic repo
> as qemu, as opposed to a qemu-qapi-rust repo ?
>
> Trying to weld together different build systems is never that
> attractive. The language specific build systems generally are
> much simpler if they're self contained. From a distro POV it
> can be better if the language bindings are self contained, as
> you don't neccessarily want to build the language binding in
> the same environment as the main app. For example with modules
> in Fedora or RHEL, there can be multiple parallel versions of
> a language runtime, and thus language bindings would be built
> separately from QEMU.
>

> IIUC, you're generating stuff from the QEMU schemas. One way
> we can deal with this is to actually install the QEMU schemas
> into /usr/share. Distros would have an "qemu-devel" package
> that provided the schemas and the QAPI base tools which
> can then be used by separate bindings.
>
>
- the schema is configured at build time. The C QAPI types depend on build
conditions. (although I don't take that into account yet, but that
shouldn't be too hard to add)
- one of the goals is to start rewriting some part of QEMU in Rust. Here, I
started with qemu-ga because it's an "easy" target.

Generating binding for QMP schema (the protocol) is slightly easier, since
the JSON messages are loosely typed. Yet, we would probably recommend you
do it from qmp introspect output, at runtime. How close that is from the
original QAPI schema, I can't tell, I never really looked or used it (it's
not human friendly to start with, iirc)

Now, I am confident various people are trying to improve the situation wrt
meson+cargo integration, I'll do some more research.

thanks

On 11/09/20 12:46, Daniel P. Berrangé wrote:
> Do we actually need/want it to be in the same monolithic repo
> as qemu, as opposed to a qemu-qapi-rust repo ?

I think QAPI and qemu-ga should be separate repos altogether.  QAPI
should be included as a submodule in both qemu and qemu-ga.  qemu-ga
instead has absolutely no dependency on QEMU and viceversa, and is a
prime candidate for removing all traces of the configure script and
being a pure meson project.

Paolo

On Fri, Sep 11, 2020 at 01:28:13PM +0200, Paolo Bonzini wrote:
> On 11/09/20 12:46, Daniel P. Berrangé wrote:
> > Do we actually need/want it to be in the same monolithic repo
> > as qemu, as opposed to a qemu-qapi-rust repo ?
> 
> I think QAPI and qemu-ga should be separate repos altogether.  QAPI
> should be included as a submodule in both qemu and qemu-ga.  qemu-ga
> instead has absolutely no dependency on QEMU and viceversa, and is a
> prime candidate for removing all traces of the configure script and
> being a pure meson project.

Yeah, qemu-ga has always been an odd fit, since it is a guest OS
component rather than host OS.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 9/11/20 7:28 AM, Paolo Bonzini wrote:
> On 11/09/20 12:46, Daniel P. Berrangé wrote:
>> Do we actually need/want it to be in the same monolithic repo
>> as qemu, as opposed to a qemu-qapi-rust repo ?
> 
> I think QAPI and qemu-ga should be separate repos altogether.  QAPI
> should be included as a submodule in both qemu and qemu-ga.  qemu-ga
> instead has absolutely no dependency on QEMU and viceversa, and is a
> prime candidate for removing all traces of the configure script and
> being a pure meson project.
> 
> Paolo
> 

I am still actively invested in moving ./scripts/qapi to ./python/qapi 
and turning it into its own installable module as I am doing with 
./python/qemu.

The benefit is that it could be used for the build system in other 
projects; or possibly new backend plugins can be developed and 
maintained separately for it.

(I am working on a JSON-SCHEMA backend.)

((Also, I want to enforce a single flake8/pylint/mypy regime for all of 
our critical Python code. Considered critical: testing infrastructure, 
anything used for the build process. This means everything in 
./python/qemu, the tracetool, and the QAPI parser.))

I have a 20-30 patch series doing the usual mypy/flake8/pylint thing to 
the QAPI parser and it will be able to stand alone in short time.

--js

Hi

On Thu, Sep 10, 2020 at 9:49 PM <marcandre.lureau@redhat.com> wrote:

>
> Shameful pain point: meson & cargo don't play nicely together.
>

I realize you need to configure with --enable-debug if you try this patch,
or it will fail to find the library location. (eh, it's really a PoC!).
Obviously, tested on a Linux/Fedora with Rust/cargo etc installed from
rustup.

-- 
Marc-André Lureau

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> Among the QEMU developers, there is a desire to use Rust. (see previous
> thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm
> related projects and other experiments).
>
> Thanks to our QAPI type system and the associate code generator, it is
> relatively straightforward to create Rust bindings for the generated C
> types (also called sys/ffi binding) and functions. (rust-bindgen could
> probably do a similar job, but it would probably bring other issues).
> This provides an important internal API already.
>
> Slightly more complicated is to expose a Rust API for those, and provide
> convenient conversions C<->Rust. Taking inspiration from glib-rs
> binding, I implemented a simplified version of the FromGlib/ToGlib
> traits, with simpler ownership model, sufficient for QAPI needs.
>
> The usage is relatively simple:
>
> - from_qemu_none(ptr: *const sys::P) -> T
>   Return a Rust type T for a const ffi pointer P.
>
> - from_qemu_full(ptr: *mut sys::P) -> T
>   Return a Rust type T for a ffi pointer P, taking ownership.
>
> - T::to_qemu_none() -> Stash<P>
>   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
>   storage data, if any).
>
> - T::to_qemu_full() -> P
>   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
>
> With those traits, it's relatively easy to implement the QMP callbacks.
> With enough interest, we could eventually start rewriting QGA in
> Rust, as it is a simple service. See qga/qmp.rs for some examples.
> We could also try to tackle qemu itself.

Up to here, you're talking about *internal* interfaces.  Correct?

Your motivation is enabling use of Rust in QEMU.  Correct?

> Finally, given that the QAPI types are easy to serialize, it was simple
> to use "serde" on them, and provide a D-Bus interface for QMP with zbus.
> (a similar approach could probably be taken for other protocols, that
> could be dynamically loaded... anyone like protobuf better?)

QMP is an *external* interface.

It supports compatible evolution: we can make certain kinds of changes
without affecting clients.  These include:

* Adding optional arguments

* Adding results

* Adding values to an enumeration type, branches to a union or
  alternate

* Reordering members of enumerations, structs, unions

* Turning an argument type into an alternate with the old type as branch

We've made use of this extensively.  See also
docs/devel/qapi-code-gen.txt section "Compatibility considerations."

How do such changes affect clients of the proposed D-Bus interface?

> This PoC modifies qemu-ga to provide the interface on the session bus:
> $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v
> $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> ...
> $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1
> ...
>
> Note: the generated code doesn't work with the qemu schema, there is a
> couple of fixme/todo left.
>
> Shameful pain point: meson & cargo don't play nicely together.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

On 21/09/20 11:16, Markus Armbruster wrote:
> QMP is an *external* interface.
> 
> It supports compatible evolution: we can make certain kinds of changes
> without affecting clients.  These include:
> 
> * Adding optional arguments
> 
> * Adding results
> 
> * Adding values to an enumeration type, branches to a union or
>   alternate
> 
> * Reordering members of enumerations, structs, unions
> 
> * Turning an argument type into an alternate with the old type as branch
> 
> We've made use of this extensively.  See also
> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
> 
> How do such changes affect clients of the proposed D-Bus interface?

All this makes me think that Q{MP,OM,API} badly needs rationale
documentation.

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/09/20 11:16, Markus Armbruster wrote:
>> QMP is an *external* interface.
>> 
>> It supports compatible evolution: we can make certain kinds of changes
>> without affecting clients.  These include:
>> 
>> * Adding optional arguments
>> 
>> * Adding results
>> 
>> * Adding values to an enumeration type, branches to a union or
>>   alternate
>> 
>> * Reordering members of enumerations, structs, unions
>> 
>> * Turning an argument type into an alternate with the old type as branch
>> 
>> We've made use of this extensively.  See also
>> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
>> 
>> How do such changes affect clients of the proposed D-Bus interface?
>
> All this makes me think that Q{MP,OM,API} badly needs rationale
> documentation.

docs/devel/qapi-code-gen.txt is fairly thorough (>8000 words), but it
doesn't try to serve as rationale documentation.

Hi

On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Hi,
> >
> > Among the QEMU developers, there is a desire to use Rust. (see previous
> > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm
> > related projects and other experiments).
> >
> > Thanks to our QAPI type system and the associate code generator, it is
> > relatively straightforward to create Rust bindings for the generated C
> > types (also called sys/ffi binding) and functions. (rust-bindgen could
> > probably do a similar job, but it would probably bring other issues).
> > This provides an important internal API already.
> >
> > Slightly more complicated is to expose a Rust API for those, and provide
> > convenient conversions C<->Rust. Taking inspiration from glib-rs
> > binding, I implemented a simplified version of the FromGlib/ToGlib
> > traits, with simpler ownership model, sufficient for QAPI needs.
> >
> > The usage is relatively simple:
> >
> > - from_qemu_none(ptr: *const sys::P) -> T
> >   Return a Rust type T for a const ffi pointer P.
> >
> > - from_qemu_full(ptr: *mut sys::P) -> T
> >   Return a Rust type T for a ffi pointer P, taking ownership.
> >
> > - T::to_qemu_none() -> Stash<P>
> >   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
> >   storage data, if any).
> >
> > - T::to_qemu_full() -> P
> >   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
> >
> > With those traits, it's relatively easy to implement the QMP callbacks.
> > With enough interest, we could eventually start rewriting QGA in
> > Rust, as it is a simple service. See qga/qmp.rs for some examples.
> > We could also try to tackle qemu itself.
>
> Up to here, you're talking about *internal* interfaces.  Correct?
>
> Your motivation is enabling use of Rust in QEMU.  Correct?

That's the first motivation, indeed.


>
> > Finally, given that the QAPI types are easy to serialize, it was simple
> > to use "serde" on them, and provide a D-Bus interface for QMP with zbus.
> > (a similar approach could probably be taken for other protocols, that
> > could be dynamically loaded... anyone like protobuf better?)
>
> QMP is an *external* interface.
>
> It supports compatible evolution: we can make certain kinds of changes
> without affecting clients.  These include:
>
> * Adding optional arguments

This would change the signature of the function, and would need an
interface version bump.

Alternative: pass optional arguments as an extra dictionary. This is a
common idiom in D-Bus (the "a{sv}" type that maps strings to generic
values)

Potentially, use gvariant serialization format, which has maybe type.
But gvariant isn't implemented by most D-Bus libraries (that was the
plan long time ago, but it didn't happen as people lost interest).

> * Adding results

Also change the signature of the function.

However, since messages have boundaries, it is easy to ignore return values.

>
> * Adding values to an enumeration type, branches to a union or
>   alternate
>

As long as the discriminant is represented as a string, it should be fine.

> * Reordering members of enumerations, structs, unions

Again, if the discriminant is a string, it should be the same as with json.

For the members, the usage of dictionaries is required in this case
(else the type signature would change).

> * Turning an argument type into an alternate with the old type as branch

That would also change the function signature.

There isn't much solution I can think of, unless we have an implicit
tagged enum for every argument, which would be quite nasty.

>
> We've made use of this extensively.  See also
> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
>
> How do such changes affect clients of the proposed D-Bus interface?

The introspection XML will always reflect the expected signature. You
should bump your interface version whenever you make incompatible
changes.

If this happens too often, we could also introduce a D-Bus override
mechanism to do manual translations from external interface to
internal.

>
> > This PoC modifies qemu-ga to provide the interface on the session bus:
> > $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v
> > $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> > ...
> > $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> > GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1
> > ...
> >
> > Note: the generated code doesn't work with the qemu schema, there is a
> > couple of fixme/todo left.
> >
> > Shameful pain point: meson & cargo don't play nicely together.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Hi,
>> >
>> > Among the QEMU developers, there is a desire to use Rust. (see previous
>> > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm
>> > related projects and other experiments).
>> >
>> > Thanks to our QAPI type system and the associate code generator, it is
>> > relatively straightforward to create Rust bindings for the generated C
>> > types (also called sys/ffi binding) and functions. (rust-bindgen could
>> > probably do a similar job, but it would probably bring other issues).
>> > This provides an important internal API already.
>> >
>> > Slightly more complicated is to expose a Rust API for those, and provide
>> > convenient conversions C<->Rust. Taking inspiration from glib-rs
>> > binding, I implemented a simplified version of the FromGlib/ToGlib
>> > traits, with simpler ownership model, sufficient for QAPI needs.
>> >
>> > The usage is relatively simple:
>> >
>> > - from_qemu_none(ptr: *const sys::P) -> T
>> >   Return a Rust type T for a const ffi pointer P.
>> >
>> > - from_qemu_full(ptr: *mut sys::P) -> T
>> >   Return a Rust type T for a ffi pointer P, taking ownership.
>> >
>> > - T::to_qemu_none() -> Stash<P>
>> >   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
>> >   storage data, if any).
>> >
>> > - T::to_qemu_full() -> P
>> >   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
>> >
>> > With those traits, it's relatively easy to implement the QMP callbacks.
>> > With enough interest, we could eventually start rewriting QGA in
>> > Rust, as it is a simple service. See qga/qmp.rs for some examples.
>> > We could also try to tackle qemu itself.
>>
>> Up to here, you're talking about *internal* interfaces.  Correct?
>>
>> Your motivation is enabling use of Rust in QEMU.  Correct?
>
> That's the first motivation, indeed.

Sounds useful.

>> > Finally, given that the QAPI types are easy to serialize, it was simple
>> > to use "serde" on them, and provide a D-Bus interface for QMP with zbus.
>> > (a similar approach could probably be taken for other protocols, that
>> > could be dynamically loaded... anyone like protobuf better?)
>>
>> QMP is an *external* interface.
>>
>> It supports compatible evolution: we can make certain kinds of changes
>> without affecting clients.  These include:
>>
>> * Adding optional arguments
>
> This would change the signature of the function, and would need an
> interface version bump.
>
> Alternative: pass optional arguments as an extra dictionary. This is a
> common idiom in D-Bus (the "a{sv}" type that maps strings to generic
> values)
>
> Potentially, use gvariant serialization format, which has maybe type.
> But gvariant isn't implemented by most D-Bus libraries (that was the
> plan long time ago, but it didn't happen as people lost interest).
>
>> * Adding results
>
> Also change the signature of the function.
>
> However, since messages have boundaries, it is easy to ignore return values.

I'm not sure I understand this.

The compatible change I have in mind is adding members to the complex
type returned by a command.

>> * Adding values to an enumeration type, branches to a union or
>>   alternate
>>
>
> As long as the discriminant is represented as a string, it should be fine.
>
>> * Reordering members of enumerations, structs, unions
>
> Again, if the discriminant is a string, it should be the same as with json.
>
> For the members, the usage of dictionaries is required in this case
> (else the type signature would change).
>
>> * Turning an argument type into an alternate with the old type as branch
>
> That would also change the function signature.
>
> There isn't much solution I can think of, unless we have an implicit
> tagged enum for every argument, which would be quite nasty.
>
>>
>> We've made use of this extensively.  See also
>> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
>>
>> How do such changes affect clients of the proposed D-Bus interface?
>
> The introspection XML will always reflect the expected signature. You
> should bump your interface version whenever you make incompatible
> changes.

How do "interface versions" work?  Client's and server's version need to
match, or else no go?

> If this happens too often, we could also introduce a D-Bus override
> mechanism to do manual translations from external interface to
> internal.

Greek to me :)

Hi

On Tue, Sep 22, 2020 at 8:11 PM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> >> * Adding results
> >
> > Also change the signature of the function.
> >
> > However, since messages have boundaries, it is easy to ignore return
> values.
>
> I'm not sure I understand this.
>
> The compatible change I have in mind is adding members to the complex
> type returned by a command.
>


There are certain things you can do in D-Bus, just like we do with JSON.
You could ignore the signature checking, you could ignore some extra
message fields... That's not what people usually expect. D-Bus is a machine
and bindings friendly. It's not meant to be so lazy.



> >> We've made use of this extensively.  See also
> >> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
> >>
> >> How do such changes affect clients of the proposed D-Bus interface?
> >
> > The introspection XML will always reflect the expected signature. You
> > should bump your interface version whenever you make incompatible
> > changes.
>
> How do "interface versions" work?  Client's and server's version need to
> match, or else no go?
>
>
The D-Bus specification doesn't detail versioning much. What is recommended
is to have the version number as part of the interface name (kinda like
soname): http://0pointer.de/blog/projects/versioning-dbus.html (this is
documented in several places iirc)

So a QEMU D-Bus interface could have a name like org.qemu.Qemu51,
org.qemu.Qemu52.. for example, if we can't provide better API stability...

-- 
Marc-André Lureau

On 22/09/20 18:35, Marc-André Lureau wrote:
> The D-Bus specification doesn't detail versioning much. What is
> recommended is to have the version number as part of the interface name
> (kinda like soname):
> http://0pointer.de/blog/projects/versioning-dbus.html (this is
> documented in several places iirc)
> 
> So a QEMU D-Bus interface could have a name like org.qemu.Qemu51,
> org.qemu.Qemu52.. for example, if we can't provide better API stability...

That would be a problem for backports.

It seems to me that the bindings issue is only a problem if we insist on
having positional arguments like we do for C, but if we can avoid
functions with a zillion arguments we could.  For example in Rust, it's
idiomatic to use the builder pattern

    let thread = thread::Builder::new()
        .name("foo".into())
        .stack_size(65536)
        .spawn(run_thread)?;
    thread.join()?;

and I think the same would work in Go or even C++.  It would look like

   qapi::qga::commands::GuestShutdown::new()
       .mode("halt")
       .invoke_on(qapi_channel)?;

with some kind of double dispatch implementation:

   trait QAPIChannel {
      ...
      fn invoke(command: dyn QAPISerialization)
          -> dyn QAPISerialization;
   }

   impl GuestShutdown {
       fn<T: QAPIChannel> invoke_on(t: T) -> () {
           let args = self.as_qapi_serialization();
           t.invoke(args);
           // could "return from_qapi_serialization(result)", likewise
       }
   }

In Python, you can use keyword arguments and there are even keyword-only
arguments ("def f(*, key1, key2)"), like

    qapi.qga.GuestFileOpen(path="/etc/passwd").invoke_on(qapi_channel);

When you do something like this QMP-style APIs are not a problem.
FlatBuffers is another serialization format that supports this kind of
extensibility (https://google.github.io/flatbuffers/ explicitly compares
it to JSON, even).

Paolo

Hi

On Tue, Sep 22, 2020 at 9:08 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 22/09/20 18:35, Marc-André Lureau wrote:
> > The D-Bus specification doesn't detail versioning much. What is
> > recommended is to have the version number as part of the interface name
> > (kinda like soname):
> > http://0pointer.de/blog/projects/versioning-dbus.html (this is
> > documented in several places iirc)
> >
> > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51,
> > org.qemu.Qemu52.. for example, if we can't provide better API
> stability...
>
> That would be a problem for backports.
>
>
Yes..  Backports could expose a subset of the new version interface? Or the
interface can be divided for each Qmp command. (unorthodox)

It seems to me that the bindings issue is only a problem if we insist on
> having positional arguments like we do for C, but if we can avoid
> functions with a zillion arguments we could.  For example in Rust, it's
> idiomatic to use the builder pattern
>
>     let thread = thread::Builder::new()
>         .name("foo".into())
>         .stack_size(65536)
>         .spawn(run_thread)?;
>     thread.join()?;
>
> and I think the same would work in Go or even C++.  It would look like
>
>    qapi::qga::commands::GuestShutdown::new()
>        .mode("halt")
>        .invoke_on(qapi_channel)?;
>
>
Or simply use the same approach as qapi-rs (
https://github.com/arcnmx/qapi-rs) which is  simply generating data
structures based on the schema, and not binding commands to Rust functions
for ex.

qga.execute(&qga::guest_shutdown { mode: Some(GuestShutdownMode::Halt) })

Less idiomatic, but it also works around the optional arguments and
ordering issue.

In both cases, the client interface should be versionized (since some
fields may be added or becoming optional, return value may appear etc), and
only at runtime can you actually verify what is actually supported.

In Python, you can use keyword arguments and there are even keyword-only
> arguments ("def f(*, key1, key2)"), like
>
>     qapi.qga.GuestFileOpen(path="/etc/passwd").invoke_on(qapi_channel);
>
>
Yes, the python binding will have a similar issue. And if we want to add
typing to the mix, representing everything as a dict is not going to help
much. Fortunately, there are other options I believe. But I would rather
aim for the obvious, having non-optional & ordered arguments, and
interface/methods versioning.

When you do something like this QMP-style APIs are not a problem.
> FlatBuffers is another serialization format that supports this kind of
> extensibility (https://google.github.io/flatbuffers/ explicitly compares
> it to JSON, even).
>

Well, I wouldn't say it's not a problem. It makes working with QMP as a
client quite an unpleasant experience overall imho...

-- 
Marc-André Lureau

On 29/09/20 09:45, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 22, 2020 at 9:08 PM Paolo Bonzini <pbonzini@redhat.com
> <mailto:pbonzini@redhat.com>> wrote:
>     > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51,
>     > org.qemu.Qemu52.. for example, if we can't provide better API
>     stability...
> 
>     That would be a problem for backports.
> 
> Yes..  Backports could expose a subset of the new version interface? Or
> the interface can be divided for each Qmp command. (unorthodox)

That seems like a workaround for an IDL that is not the right solution
for the problem.

>        qapi::qga::commands::GuestShutdown::new()
>            .mode("halt")
>            .invoke_on(qapi_channel)?;
> 
> 
> Or simply use the same approach as qapi-rs
> (https://github.com/arcnmx/qapi-rs) which is  simply generating data
> structures based on the schema, and not binding commands to Rust
> functions for ex.
> 
> qga.execute(&qga::guest_shutdown { mode: Some(GuestShutdownMode::Halt) })


That would not be backwards compatible as you would have to set all
optional fields.  Every time the command grows a new optional argument,
all clients would have to specify it; if a command becomes optional,
you'd have to add Some() around it.  So I would say that...

> Less idiomatic, but it also works around the optional arguments and
> ordering issue.

...  the builder pattern is not a workaround: it's the best and most
common Rust idiom to achieve what QAPI expresses as optional fields.
Likewise for keyword-only arguments in Python.

> Yes, the python binding will have a similar issue. And if we want to add
> typing to the mix, representing everything as a dict is not going to
> help much. Fortunately, there are other options I believe. But I would
> rather aim for the obvious, having non-optional & ordered arguments, and
> interface/methods versioning.

You shouldn't just state what you want; you really should take a look at
how the ability to add optional arguments has been used in the past, and
see if an alternative RPC without optional and unordered arguments would
have been as effective.  D-Bus is probably a perfectly good API for
qemu-ga.  The experience with qemu-ga however does not necessarily
extend to QEMU.

The main issue with D-Bus is that it conflates the transport and the IDL
so that you have to resort to passing arguments as key-value pairs.  QMP
does the same, but the IDL is much more flexible and not QEMU-specific,
so we don't pay as high a price.  Remember that while the specific
transport of QMP ("JSON dictionaries over a socket with 'execute' and
'arguments' keys") is QEMU-specific, the concept of using JSON payloads
for RPC is very common.  For example, REST APIs almost invariably use
JSON and the resulting API even "feel" somewhat similar to QMP.

In fact, The first Google result for "openapi backwards compatible
extensions"
(https://github.com/zalando/restful-api-guidelines/blob/master/chapters/compatibility.adoc#107)
might as well be written for QMP:

* [server] Add only optional, never mandatory fields.

* [client] Be tolerant with unknown fields in the payload

* [server] Unknown input fields in payload or URL should not be ignored

* [client] API clients consuming data must not assume that objects are
closed for extension

* [server] When changing your RESTful APIs, do so in a compatible way
and avoid generating additional API versions

* [server] MUST not use URI versioning

and so on.

If you want to "reinvent" QMP, instead of focusing on D-Bus you should
take a look at alternative IDLs and protocols (D-Bus is one but there's
also Protobuf and Flexbuffers), see how QAPI declarations would map to
those protocols, see how you would deal with extensibility, and rank
them according to various criteria.  For example:

* JSON "just works" but needs a custom code generator and imposes some
extra complexity on the clients for the simplest commands

* D-Bus has a good ecosystem and would keep simple commands simpler but
has issues with upgrade paths and is uglier for complex commands

* Protobufs probably would also just work and would have better code
generators, but would require some kind of lint to ensure
backwards-compatibility

* etc.

> Well, I wouldn't say it's not a problem. It makes working with QMP as a
> client quite an unpleasant experience overall imho...

With automatically-generated bindings, the experience writing a QMP
client is as pleasant as the code generator makes it.

Paolo

Hi

On Tue, Sep 29, 2020 at 2:15 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 29/09/20 09:45, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Sep 22, 2020 at 9:08 PM Paolo Bonzini <pbonzini@redhat.com
> > <mailto:pbonzini@redhat.com>> wrote:
> >     > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51,
> >     > org.qemu.Qemu52.. for example, if we can't provide better API
> >     stability...
> >
> >     That would be a problem for backports.
> >
> > Yes..  Backports could expose a subset of the new version interface? Or
> > the interface can be divided for each Qmp command. (unorthodox)
>
> That seems like a workaround for an IDL that is not the right solution
> for the problem.
>
> >        qapi::qga::commands::GuestShutdown::new()
> >            .mode("halt")
> >            .invoke_on(qapi_channel)?;
> >
> >
> > Or simply use the same approach as qapi-rs
> > (https://github.com/arcnmx/qapi-rs) which is  simply generating data
> > structures based on the schema, and not binding commands to Rust
> > functions for ex.
> >
> > qga.execute(&qga::guest_shutdown { mode: Some(GuestShutdownMode::Halt) })
>
>
> That would not be backwards compatible as you would have to set all
> optional fields.  Every time the command grows a new optional argument,
> all clients would have to specify it; if a command becomes optional,
> you'd have to add Some() around it.  So I would say that...
>
>
Not necessarily, with ..default::Default()

> Less idiomatic, but it also works around the optional arguments and
> > ordering issue.
>
> ...  the builder pattern is not a workaround: it's the best and most
> common Rust idiom to achieve what QAPI expresses as optional fields.
> Likewise for keyword-only arguments in Python.
>

Except QAPI makes all fields potentially optional (and unordered), that's
not idiomatic.


> > Yes, the python binding will have a similar issue. And if we want to add
> > typing to the mix, representing everything as a dict is not going to
> > help much. Fortunately, there are other options I believe. But I would
> > rather aim for the obvious, having non-optional & ordered arguments, and
> > interface/methods versioning.
>
> You shouldn't just state what you want; you really should take a look at
> how the ability to add optional arguments has been used in the past, and
> see if an alternative RPC without optional and unordered arguments would
> have been as effective.  D-Bus is probably a perfectly good API for
> qemu-ga.  The experience with qemu-ga however does not necessarily
> extend to QEMU.
>
> The main issue with D-Bus is that it conflates the transport and the IDL
> so that you have to resort to passing arguments as key-value pairs.  QMP
>

D-Bus is machine-level oriented, it's easy to bind to various languages, it
can be pretty efficient too. It's not designed to be a good network RPC.
QMP tries to be a bit of both, but is perhaps not good enough in either.

does the same, but the IDL is much more flexible and not QEMU-specific,
> so we don't pay as high a price.  Remember that while the specific
> transport of QMP ("JSON dictionaries over a socket with 'execute' and
> 'arguments' keys") is QEMU-specific, the concept of using JSON payloads
> for RPC is very common.  For example, REST APIs almost invariably use
> JSON and the resulting API even "feel" somewhat similar to QMP.
>
> In fact, The first Google result for "openapi backwards compatible
> extensions"
> (
> https://github.com/zalando/restful-api-guidelines/blob/master/chapters/compatibility.adoc#107
> )
> might as well be written for QMP:
>
> * [server] Add only optional, never mandatory fields.
>
> * [client] Be tolerant with unknown fields in the payload
>
> * [server] Unknown input fields in payload or URL should not be ignored
>
> * [client] API clients consuming data must not assume that objects are
> closed for extension
>
> * [server] When changing your RESTful APIs, do so in a compatible way
> and avoid generating additional API versions
>
> * [server] MUST not use URI versioning
>
> and so on.
>
> If you want to "reinvent" QMP, instead of focusing on D-Bus you should
> take a look at alternative IDLs and protocols (D-Bus is one but there's
> also Protobuf and Flexbuffers), see how QAPI declarations would map to
> those protocols, see how you would deal with extensibility, and rank
> them according to various criteria.  For example:
>
> * JSON "just works" but needs a custom code generator and imposes some
> extra complexity on the clients for the simplest commands
>
> * D-Bus has a good ecosystem and would keep simple commands simpler but
> has issues with upgrade paths and is uglier for complex commands
>
> * Protobufs probably would also just work and would have better code
> generators, but would require some kind of lint to ensure
> backwards-compatibility
>
>

Again, the issues we are discussing are not specific to binding QMP over
D-Bus. Binding QMP to various languages has similar problems. Perhaps those
problems are minor, perhaps we can find decent solutions, like the one you
suggest to use Rust builder pattern for commands. I think they are not
great, as I tried to explain with the versioning and runtime issues that
you have to take into account anyway at the client level. I would rather
make those problems solved at the server level, that doesn't require any
change to QMP today, just a more careful consideration when making changes
(and probably some tools to help enforce some stability).



-- 
Marc-André Lureau

On 29/09/20 12:34, Marc-André Lureau wrote:
>     That would not be backwards compatible as you would have to set all
>     optional fields.  Every time the command grows a new optional argument,
>     all clients would have to specify it; if a command becomes optional,
>     you'd have to add Some() around it.  So I would say that...
> 
> 
> Not necessarily, with ..default::Default()

That's true, I always forget about .. (though you'd still have to add
Some() for now-optional fields).

>     > Less idiomatic, but it also works around the optional arguments and
>     > ordering issue.
> 
>     ...  the builder pattern is not a workaround: it's the best and most
>     common Rust idiom to achieve what QAPI expresses as optional fields.
>     Likewise for keyword-only arguments in Python.
> 
> Except QAPI makes all fields potentially optional (and unordered),
> that's not idiomatic.

Yes, for some APIs you can always add hand-written, more idiomatic
versions.  Or you could mark them as fixed-arguments in the schema and
let the code generator do that (but then you need to add a compatibility
check).  But that would be an explicit choice, not something required by
the transport.

> D-Bus is machine-level oriented, it's easy to bind to various languages,
> it can be pretty efficient too. It's not designed to be a good network
> RPC. QMP tries to be a bit of both, but is perhaps not good enough in
> either.

No, only tries to be a good network RPC; not a particularly efficient
one, but future-proof.  And it mostly succeeds at that---with one
notable exception: JSON parsers that mess up with numbers bigger than 2^53.

>     If you want to "reinvent" QMP, instead of focusing on D-Bus you should
>     take a look at alternative IDLs and protocols (D-Bus is one but there's
>     also Protobuf and Flexbuffers), see how QAPI declarations would map to
>     those protocols, see how you would deal with extensibility, and rank
>     them according to various criteria.  For example:
> 
>     * JSON "just works" but needs a custom code generator and imposes some
>     extra complexity on the clients for the simplest commands
> 
>     * D-Bus has a good ecosystem and would keep simple commands simpler but
>     has issues with upgrade paths and is uglier for complex commands
> 
>     * Protobufs probably would also just work and would have better code
>     generators, but would require some kind of lint to ensure
>     backwards-compatibility
> 
> Again, the issues we are discussing are not specific to binding QMP over
> D-Bus. Binding QMP to various languages has similar problems.

Marc-André, we are totally in agreement about that!  The problem is that
you have already decided what the solution looks like, and that's what
I'm not sure about because your solution also implies completely
revisiting the schema.

I say there are many candidates (the ones I know are Protobuf and
Flexbuffers) for serialization and many candidates for transport (REST
and gRPC to begin with) in addition to the two {QMP,JSON} and
{DBus,DBus} tuples.  We should at least look at how they do code
generation before deciding that JSON is bad and DBus is good.

> I would rather make those problems solved at the server level, that
> doesn't require any change to QMP today, just a more careful
> consideration when making changes (and probably some tools to help
> enforce some stability).

Problem is, "more careful consideration when making changes" is not a
small thing.  And other RPCs have evolved in a completely different
space (REST APIs for web services) that have chosen the same tradeoffs
as QMP, so why should we not learn from them?

Paolo

Hi

On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 29/09/20 12:34, Marc-André Lureau wrote:
> >     That would not be backwards compatible as you would have to set all
> >     optional fields.  Every time the command grows a new optional
> argument,
> >     all clients would have to specify it; if a command becomes optional,
> >     you'd have to add Some() around it.  So I would say that...
> >
> >
> > Not necessarily, with ..default::Default()
>
> That's true, I always forget about .. (though you'd still have to add
> Some() for now-optional fields).
>
> >     > Less idiomatic, but it also works around the optional arguments and
> >     > ordering issue.
> >
> >     ...  the builder pattern is not a workaround: it's the best and most
> >     common Rust idiom to achieve what QAPI expresses as optional fields.
> >     Likewise for keyword-only arguments in Python.
> >
> > Except QAPI makes all fields potentially optional (and unordered),
> > that's not idiomatic.
>
> Yes, for some APIs you can always add hand-written, more idiomatic
> versions.  Or you could mark them as fixed-arguments in the schema and
> let the code generator do that (but then you need to add a compatibility
> check).  But that would be an explicit choice, not something required by
> the transport.
>

That's my point, if we agree on keeping arguments & members non-optional
and ordered, we are doing 50% of the job for nicer automated bindings.

Second half would probably be involving some version information in the
schema.


> > D-Bus is machine-level oriented, it's easy to bind to various languages,
> > it can be pretty efficient too. It's not designed to be a good network
> > RPC. QMP tries to be a bit of both, but is perhaps not good enough in
> > either.
>
> No, only tries to be a good network RPC; not a particularly efficient
> one, but future-proof.  And it mostly succeeds at that---with one
> notable exception: JSON parsers that mess up with numbers bigger than 2^53.
>
> >     If you want to "reinvent" QMP, instead of focusing on D-Bus you
> should
> >     take a look at alternative IDLs and protocols (D-Bus is one but
> there's
> >     also Protobuf and Flexbuffers), see how QAPI declarations would map
> to
> >     those protocols, see how you would deal with extensibility, and rank
> >     them according to various criteria.  For example:
> >
> >     * JSON "just works" but needs a custom code generator and imposes
> some
> >     extra complexity on the clients for the simplest commands
> >
> >     * D-Bus has a good ecosystem and would keep simple commands simpler
> but
> >     has issues with upgrade paths and is uglier for complex commands
> >
> >     * Protobufs probably would also just work and would have better code
> >     generators, but would require some kind of lint to ensure
> >     backwards-compatibility
> >
> > Again, the issues we are discussing are not specific to binding QMP over
> > D-Bus. Binding QMP to various languages has similar problems.
>
> Marc-André, we are totally in agreement about that!  The problem is that
> you have already decided what the solution looks like, and that's what
> I'm not sure about because your solution also implies completely
> revisiting the schema.
>

Did I? Which schema change are you (or I) implying? Versioning the
interface? It's necessary at the client level, unless everything is
dynamic, after introspection, which makes automated static bindings
impractical.


> I say there are many candidates (the ones I know are Protobuf and
> Flexbuffers) for serialization and many candidates for transport (REST
> and gRPC to begin with) in addition to the two {QMP,JSON} and
> {DBus,DBus} tuples.  We should at least look at how they do code
> generation before deciding that JSON is bad and DBus is good.
>

Contrary to what you believe I am not focusing so much on DBus here :) It
took about 200 loc to bind it, effortlessly (compared to sys<->rs
conversion). All it does is to expose the same API we have in the generated
C somehow (similar static types & functions - not all as a{sv} opaque
dictionaries).

It's easy for QEMU to generate a good static binding for C, because the
version always matches. For a client, you wouldn't be able to write a
similar idiomatic API in C, Rust, Python or Go, unfortunately.

Iow, I am not trying to sell DBus, I would like to make it easier to bind
QMP in general. (although I do believe that DBus is a better protocol than
QMP for local IPC, yes. And gRPC is probably better for remoting)

> I would rather make those problems solved at the server level, that
> > doesn't require any change to QMP today, just a more careful
> > consideration when making changes (and probably some tools to help
> > enforce some stability).
>
> Problem is, "more careful consideration when making changes" is not a
> small thing.  And other RPCs have evolved in a completely different
> space (REST APIs for web services) that have chosen the same tradeoffs
> as QMP, so why should we not learn from them?
>
>
I don't buy that generalization. A very recent protocol in this space, that
aims to be a good low-level RPC on Linux (for containers, cloud etc) is
varlink. (In many ways, we could compare it to QMP, but it lacks some
important features, like events)

varlink does non-optional members and versioning the same way I propose
here, for what I could tell. Although they use JSON, and have similar
transport "benefits", this basic rule allow them to have very decent
automated binding in various languages, without resorting to unorthodox
solutions, ex:
https://github.com/varlink/rust/blob/master/examples/example/src/main.rs

-- 
Marc-André Lureau

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
[...]
>> Marc-André, we are totally in agreement about that!  The problem is that
>> you have already decided what the solution looks like, and that's what
>> I'm not sure about because your solution also implies completely
>> revisiting the schema.
>>
>
> Did I? Which schema change are you (or I) implying? Versioning the
> interface? It's necessary at the client level, unless everything is
> dynamic, after introspection, which makes automated static bindings
> impractical.

I disagree with "necessary".

A client can use a specific version of QMP, and still talk to a server
with a different version, because we designed that capability into QMP.

You absolutely can create bindings for a specific version of QMP for the
client if you want.  Just make sure the client as a whole obeys the
rules of the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt.

>> I say there are many candidates (the ones I know are Protobuf and
>> Flexbuffers) for serialization and many candidates for transport (REST
>> and gRPC to begin with) in addition to the two {QMP,JSON} and
>> {DBus,DBus} tuples.  We should at least look at how they do code
>> generation before deciding that JSON is bad and DBus is good.
>>
>
> Contrary to what you believe I am not focusing so much on DBus here :) It
> took about 200 loc to bind it, effortlessly (compared to sys<->rs
> conversion). All it does is to expose the same API we have in the generated
> C somehow (similar static types & functions - not all as a{sv} opaque
> dictionaries).

Two points.

1. Opaque dictionaries are far from the only way to do keyword arguments
in a language that lacks them.

2. The API we generate for C is not exactly wonderful.

Behold this beauty:

    void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, bool has_base_node, const char *base_node, bool has_base, const char *base, bool has_top_node, const char *top_node, bool has_top, const char *top, bool has_backing_file, const char *backing_file, bool has_speed, int64_t speed, bool has_on_error, BlockdevOnError on_error, bool has_filter_node_name, const char *filter_node_name, bool has_auto_finalize, bool auto_finalize, bool has_auto_dismiss, bool auto_dismiss, Error **errp);

It's gotten so bad we added a second way to do the C API:

    void qmp_drive_backup(DriveBackup *arg, Error **errp);

Turns out

    DriveBackup arg = {
        ... initialize the optionals you need ...
    }
    qmp_drive_backup(&arg, &err);

is a lot easier on the eyes than passing 29 positional arguments.

This could be viewed as a work-around for C's lack of positional
parameters.

Even more fun:

    void qmp_blockdev_add(BlockdevOptions *arg, Error **errp);

BlockdevOptions is a tagged union.

This could be viewed as a work-around for C's lack of function
overloading.

> It's easy for QEMU to generate a good static binding for C, because the
> version always matches. For a client, you wouldn't be able to write a
> similar idiomatic API in C, Rust, Python or Go, unfortunately.

I disagree.  You won't be able to write good bindings using just
positional parameters.  Not even if you add restrictions on how we can
evolve QMP.  And no, I do not consider the C bindings we create for QEMU
itself "good".  They're the best we could do, and good enough.

When you do bindings for another language, do bindings for that
language, not C bindings in that language.

Regardless of bindings, the client as a whole should obey the rules of
the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt.  If these
rules have become counter-productive, then it's time to replace QMP
wholesale.

Do not attempt to force a square peg into a round hole.  If we must have
square pegs, design a square hole, and retire the round hole.

> Iow, I am not trying to sell DBus, I would like to make it easier to bind
> QMP in general. (although I do believe that DBus is a better protocol than
> QMP for local IPC, yes. And gRPC is probably better for remoting)
>
>> I would rather make those problems solved at the server level, that
>> > doesn't require any change to QMP today, just a more careful
>> > consideration when making changes (and probably some tools to help
>> > enforce some stability).
>>
>> Problem is, "more careful consideration when making changes" is not a
>> small thing.  And other RPCs have evolved in a completely different
>> space (REST APIs for web services) that have chosen the same tradeoffs
>> as QMP, so why should we not learn from them?
>>
>>
> I don't buy that generalization. A very recent protocol in this space, that
> aims to be a good low-level RPC on Linux (for containers, cloud etc) is
> varlink. (In many ways, we could compare it to QMP, but it lacks some
> important features, like events)
>
> varlink does non-optional members and versioning the same way I propose
> here, for what I could tell. Although they use JSON, and have similar
> transport "benefits", this basic rule allow them to have very decent
> automated binding in various languages, without resorting to unorthodox
> solutions, ex:
> https://github.com/varlink/rust/blob/master/examples/example/src/main.rs

Paolo pointed out successful protocols that make tradeoffs similar to
QMP to support the idea that these tradeoffs can make sense and are
workable.

Pointing out other, dissimilar protocols is not a convincing
counter-argument :)

Hi

On Wed, Sep 30, 2020 at 11:34 AM Markus Armbruster <armbru@redhat.com>
wrote:

> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> >
> > On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com>
> wrote:
> [...]
> >> Marc-André, we are totally in agreement about that!  The problem is that
> >> you have already decided what the solution looks like, and that's what
> >> I'm not sure about because your solution also implies completely
> >> revisiting the schema.
> >>
> >
> > Did I? Which schema change are you (or I) implying? Versioning the
> > interface? It's necessary at the client level, unless everything is
> > dynamic, after introspection, which makes automated static bindings
> > impractical.
>
> I disagree with "necessary".
>
> A client can use a specific version of QMP, and still talk to a server
> with a different version, because we designed that capability into QMP.
>

 "A client can use a specific version of QMP" == versioning on the client
side

>
> You absolutely can create bindings for a specific version of QMP for the
> client if you want.  Just make sure the client as a whole obeys the
> rules of the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt.
>
> >> I say there are many candidates (the ones I know are Protobuf and
> >> Flexbuffers) for serialization and many candidates for transport (REST
> >> and gRPC to begin with) in addition to the two {QMP,JSON} and
> >> {DBus,DBus} tuples.  We should at least look at how they do code
> >> generation before deciding that JSON is bad and DBus is good.
> >>
> >
> > Contrary to what you believe I am not focusing so much on DBus here :) It
> > took about 200 loc to bind it, effortlessly (compared to sys<->rs
> > conversion). All it does is to expose the same API we have in the
> generated
> > C somehow (similar static types & functions - not all as a{sv} opaque
> > dictionaries).
>
> Two points.
>
> 1. Opaque dictionaries are far from the only way to do keyword arguments
> in a language that lacks them.
>

Oh one can always be creative. The point is trying to stay idiomatic in the
target language.


>
> 2. The API we generate for C is not exactly wonderful.
>
> Behold this beauty:
>
>     void qmp_block_commit(bool has_job_id, const char *job_id, const char
> *device, bool has_base_node, const char *base_node, bool has_base, const
> char *base, bool has_top_node, const char *top_node, bool has_top, const
> char *top, bool has_backing_file, const char *backing_file, bool has_speed,
> int64_t speed, bool has_on_error, BlockdevOnError on_error, bool
> has_filter_node_name, const char *filter_node_name, bool has_auto_finalize,
> bool auto_finalize, bool has_auto_dismiss, bool auto_dismiss, Error **errp);
>
> It's gotten so bad we added a second way to do the C API:
>
>     void qmp_drive_backup(DriveBackup *arg, Error **errp);
>
> Turns out
>
>     DriveBackup arg = {
>         ... initialize the optionals you need ...
>     }
>     qmp_drive_backup(&arg, &err);
>
> is a lot easier on the eyes than passing 29 positional arguments.
>
>
So is writing the function arguments with indentation. Then I don't see
much difference between a long list of arguments in a struct and that
function. The main difference is that you make it easy to pass those
arguments down. But often, you want to pass a subset, you don't want to
pass the whole context as it may lead to bad design / bugs.

This could be viewed as a work-around for C's lack of positional
> parameters.
>
>
Or a badly designed QMP command.

Even more fun:
>
>     void qmp_blockdev_add(BlockdevOptions *arg, Error **errp);
>
> BlockdevOptions is a tagged union.
>
> This could be viewed as a work-around for C's lack of function
> overloading.
>
>
Or a badly designed QMP command ?

> It's easy for QEMU to generate a good static binding for C, because the
> > version always matches. For a client, you wouldn't be able to write a
> > similar idiomatic API in C, Rust, Python or Go, unfortunately.
>
> I disagree.  You won't be able to write good bindings using just
> positional parameters.  Not even if you add restrictions on how we can
> evolve QMP.  And no, I do not consider the C bindings we create for QEMU
> itself "good".  They're the best we could do, and good enough.
>
>
Sure they could be better, they are still quite idiomatic for C.

When you do bindings for another language, do bindings for that
> language, not C bindings in that language.
>
>
Yes

Regardless of bindings, the client as a whole should obey the rules of
> the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt.  If these
> rules have become counter-productive, then it's time to replace QMP
> wholesale.
>
> Do not attempt to force a square peg into a round hole.  If we must have
> square pegs, design a square hole, and retire the round hole.
>
>
Hmm? I am trying to make the hole a bit more regular...

> Iow, I am not trying to sell DBus, I would like to make it easier to bind
> > QMP in general. (although I do believe that DBus is a better protocol
> than
> > QMP for local IPC, yes. And gRPC is probably better for remoting)
> >
> >> I would rather make those problems solved at the server level, that
> >> > doesn't require any change to QMP today, just a more careful
> >> > consideration when making changes (and probably some tools to help
> >> > enforce some stability).
> >>
> >> Problem is, "more careful consideration when making changes" is not a
> >> small thing.  And other RPCs have evolved in a completely different
> >> space (REST APIs for web services) that have chosen the same tradeoffs
> >> as QMP, so why should we not learn from them?
> >>
> >>
> > I don't buy that generalization. A very recent protocol in this space,
> that
> > aims to be a good low-level RPC on Linux (for containers, cloud etc) is
> > varlink. (In many ways, we could compare it to QMP, but it lacks some
> > important features, like events)
> >
> > varlink does non-optional members and versioning the same way I propose
> > here, for what I could tell. Although they use JSON, and have similar
> > transport "benefits", this basic rule allow them to have very decent
> > automated binding in various languages, without resorting to unorthodox
> > solutions, ex:
> > https://github.com/varlink/rust/blob/master/examples/example/src/main.rs
>
> Paolo pointed out successful protocols that make tradeoffs similar to
> QMP to support the idea that these tradeoffs can make sense and are
> workable.
>
> Pointing out other, dissimilar protocols is not a convincing
> counter-argument :)
>

It's relevant. Did you study varlink a bit? It's so close to QMP, you will
find it hard to point out real dissimilarities.


-- 
Marc-André Lureau

On 30/09/20 09:51, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Sep 30, 2020 at 11:34 AM Markus Armbruster <armbru@redhat.com
> <mailto:armbru@redhat.com>> wrote:
> 
>     Marc-André Lureau <marcandre.lureau@gmail.com
>     <mailto:marcandre.lureau@gmail.com>> writes:
> 
>     > Hi
>     >
>     > On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com
>     <mailto:pbonzini@redhat.com>> wrote:
>     [...]
>     >> Marc-André, we are totally in agreement about that!  The problem
>     is that
>     >> you have already decided what the solution looks like, and that's
>     what
>     >> I'm not sure about because your solution also implies completely
>     >> revisiting the schema.
>     >>
>     >
>     > Did I? Which schema change are you (or I) implying? Versioning the
>     > interface? It's necessary at the client level, unless everything is
>     > dynamic, after introspection, which makes automated static bindings
>     > impractical.
> 
>     I disagree with "necessary".
> 
>     A client can use a specific version of QMP, and still talk to a server
>     with a different version, because we designed that capability into QMP.
> 
>  "A client can use a specific version of QMP" == versioning on the
> client side

No: "a client can use code generated for a specific version of QMP" does
not imply versioning on the client side.  It does imply that, before
using *certain* features, the client must verify that they are
implemented in the server.

>     1. Opaque dictionaries are far from the only way to do keyword arguments
>     in a language that lacks them.
> 
> Oh one can always be creative. The point is trying to stay idiomatic in
> the target language.

And builders in Rust, or keyword arguments in Python, are perfectly
idiomatic.

> Or a badly designed QMP command.
> Or a badly designed QMP command ?

Great.  Do you have ideas around how to design good QMP commands?  It
doesn't have to be patches for code, just documentation.

>     > varlink does non-optional members and versioning the same way I propose
>     > here, for what I could tell. Although they use JSON, and have similar
>     > transport "benefits", this basic rule allow them to have very decent
>     > automated binding in various languages, without resorting to
>     unorthodox
>     > solutions, ex:
>     >
>     https://github.com/varlink/rust/blob/master/examples/example/src/main.rs
> 
>     Paolo pointed out successful protocols that make tradeoffs similar to
>     QMP to support the idea that these tradeoffs can make sense and are
>     workable.
> 
>     Pointing out other, dissimilar protocols is not a convincing
>     counter-argument :)
> 
> It's relevant. Did you study varlink a bit? It's so close to QMP, you
> will find it hard to point out real dissimilarities.

I played a bit with varlink, and it seems to me that varlink does
actually support QMP-/OpenAPI-like extensibility.  Missing nullable
fields are treated as null, and on the client side it generally does not
give an error if it receives unknown fields (just like QMP). [1]  Also
just like QMP there are limits to the server's liberality, for example
sending extra fields to the server is an error.

In fact I could have skipped the experiments and read the Varlink
documentation: :)

  Varlink interfaces do not have a version number, they only have a
  feature set described in detail by the interface definition, which is
  part of the wire protocol.

  If your interface has users, you should not break the interface, only
  extend it, never remove or incompatibly change things which might be
  already in use.

  [...]

  Varlink does not use positional parameters or fixed-size objects in
  its interface definition or on the wire, all parameters are identified
  by their name and can be extended later.

  Extending existing structures should be done via optional fields
  (nullable type, maybe). The result of the methods, when passed
  parameters without the optional fields should be the same as in older
  versions. Method parameters can be extended the same way. The expected
  behavior for omitted fields/parameters should be documented. Removing
  fields, types, errors or methods are not backward compatible and
  should be avoided.

which is pretty much what QMP does.  So even though some users of
varlink might have chosen to rein themselves in on the extensibility
allowed by Varlink, that's a self-imposed limitation.  It may be due to
the desire to interoperate with varlink language bindings that use
positional parameters, but that's again a limitation of the bindings and
not the protocol.

So I don't see varlink as a reason to change the existing practice for
QMP, more as a second example of someone else doing the same as QMP.

Again, I'm not saying that DBus's choice with respect to versioning and
backwards/forwards-compatibility is _wrong_.  I do not like Protobuf's
numbered fields for that matter either, but it's not wrong either.  I am
saying that the choices in QMP are one of many different tradeoffs, and
that there's no point in saying that they get in the way of writing
language bindings or even just in the way of writing good/idiomatic
language bindings, because they don't.

Paolo




[1] Try this:

$ git clone git://github.com/bonzini/python
$ git checkout nullable-ping
$ python3 -m varlink.tests.test_orgexamplemore --varlink="unix:/tmp/test" &

And then:

$ varlink info unix:/tmp/test org.example.more
...
method Ping(ping: ?string) -> (pong: ?string)
...
$ python -m varlink.cli call unix:/tmp/test/org.example.more.Ping '{}'
{
  "pang": "xxx",
  "pong": null
}

You can see that the argument was implicitly treated as null, and there
were no complaints about the extra returned field.

I didn't try returning extra fields with the Rust bindings, but I did
try optional arguments and they work fine.  A "recent" client with
optional argument can talk to an "old" server with non-optional
argument, and it will only fail if the client actually takes advantage
of the newer interface.  As long as the argument is there, everything
works just fine.

On Tue, Sep 22, 2020 at 05:09:56PM +0200, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > Hi
> >
> > On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> We've made use of this extensively.  See also
> >> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
> >>
> >> How do such changes affect clients of the proposed D-Bus interface?
> >
> > The introspection XML will always reflect the expected signature. You
> > should bump your interface version whenever you make incompatible
> > changes.
> 
> How do "interface versions" work?  Client's and server's version need to
> match, or else no go?

Yes, both client and server need to support the same "interface" in order
to talk to each other. If a service makes an incompatible API change, then
then change the interface name. It is common to include a number in the
interface name.

Of course some changes can be made in a backwards compatible manner so
don't require changing the interface. eg adding new methods, adding
new signals (aka async events), adding new properties.

Adding/changing/removing parameters  in an existing method is what
causes an interface incompatability.

You can potentially mitigate the inability to change parameters by
modelling parameters in a variety of ways.

Many of the tricks common in traditional C libraries for future
proofing API designs will apply equally well to DBus APIs.

For example, instead of modelling everything as a distinct positional
parameter, you can model them as optional named parameters. These would
be exposed as an array(map(string-variant)) - basically think of it as a
hash table. This is a half-way house between very strongly typed and very
weakly typed worlds.  Libvirt takes this approach with some of our C APIs
that we expect to grow parameters over time.

A variant on this is to expose some data in a custom document format
as a single parameter. For example if there was an API requesting a
description of a block backend properties, instead of having a DBus
API where all the block backend props were explicitly modelled, just
have a single string parameter that returns a JSON/XML doc. You can
extend that doc at will without impacting the DBus API. Again libvirt
takes this approach with our XML doc schema.

A final useful technique is to have a generic "int flags" parameter as
a way to express new functional behaviour by defining extra flags.


The challenge with all these ideas is figuring out whether there's
a viable way to map them into how we describe the current QMP protocol.
I don't think there's a especially good answer to do a 100% automated
mapping from QMP to DBus, while keeping extensibility and also maximising
strong type checking benefits. There's a tradeoff between extensibility
and strong typing that needs some intelligent thought on a case by case
basis IMHO.

For an automated mapping, either we go fully strong typed and accept
that we'll be breaking DBus interface compatibility continually, or
we go full weakly typed and accept that clients won't get much type
validation benefits at build time.

It is the inherant problem with using something weakly typed as the
master specification and trying to translate it to something strongly
typed.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

[...]
> The challenge with all these ideas is figuring out whether there's
> a viable way to map them into how we describe the current QMP protocol.
> I don't think there's a especially good answer to do a 100% automated
> mapping from QMP to DBus, while keeping extensibility and also maximising
> strong type checking benefits. There's a tradeoff between extensibility
> and strong typing that needs some intelligent thought on a case by case
> basis IMHO.
>
> For an automated mapping, either we go fully strong typed and accept
> that we'll be breaking DBus interface compatibility continually, or
> we go full weakly typed and accept that clients won't get much type
> validation benefits at build time.
>
> It is the inherant problem with using something weakly typed as the
> master specification and trying to translate it to something strongly
> typed.

I avoid "weakly typed" and "strongly typed", because it means different
things to different people.  "Statically typed" and "dynamically typed"
are clear concepts.

Interfaces defined with QAPI are statically typed.

Serialization / deserialization via QMP works even when the interfaces
on both ends differ in certain limited ways.  It supports more
differences than an RPC transport whose idea of "procedure" has both
feet in the grave of ALGOL 60 could.

At either end of the wire, we need to bridge the gap between a
programming language and the wire format.

At the server end (QEMU), we use the QAPI code generator to bridge from
C, and for the one specific version of the QAPI-defined interface this
build of QEMU provides.

Clients we've left to fend for themselves.  Perhaps it's been a matter
of not understanding client requirements well enough, and chickening
out.

A client could do the same as QEMU does: bridge one specific version of
the QAPI-defined interface.  Could even be done with a suitably extended
QAPI code generator.  A client could also bridge multiple interface
versions, if that us useful.

Regardless of the number of versions bridged, the client could still
talk to servers that use another version, thanks to the QMP transport.

When the server (i.e. QEMU) is newer, any new stuff it may provide is
inaccessible (d'oh).  Also, old stuff may be gone (rare, involves going
through the deprecation process).  Graceful degradation when the client
lags behind too much.

When the server is older than the client, any new stuff the client
actually uses won't work.  The client can use introspection to adjust
itself to older servers.  Graceful degradation again.

Nothing so far precludes a statically typed interface for client code!

The problem isn't "weakly typed" QMP (whatever that may mean), it's that
chasing the evolving QAPI-defined interface with manually written code
is a lot of work.

Libvirt has been doing that work, but you know, "what have the Romans
ever done for us?"

Marc-André's D-Bus experiment explores another way to bridge the gap.
This bridge is generated, not hand-written.  Because the QMP transport
also gets replaced by one that lacks QMP's capability to "mediate"
between versions, client and server versions become tightly coupled.

This isn't due to static typing.  It's due to use of a tool that makes
different tradeoffs than we made with QAPI/QMP.

I'm not fundamentally opposed to adding some QAPI/not-QMP external
interface, but I'd prefer not to end up with the union of disadvantages
and the intersection of advantages.

If QMP no longer matches our requirements, we should replace it
wholesale.

Hi

On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Hi,
> >
> > Among the QEMU developers, there is a desire to use Rust. (see previous
> > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm
> > related projects and other experiments).
> >
> > Thanks to our QAPI type system and the associate code generator, it is
> > relatively straightforward to create Rust bindings for the generated C
> > types (also called sys/ffi binding) and functions. (rust-bindgen could
> > probably do a similar job, but it would probably bring other issues).
> > This provides an important internal API already.
> >
> > Slightly more complicated is to expose a Rust API for those, and provide
> > convenient conversions C<->Rust. Taking inspiration from glib-rs
> > binding, I implemented a simplified version of the FromGlib/ToGlib
> > traits, with simpler ownership model, sufficient for QAPI needs.
> >
> > The usage is relatively simple:
> >
> > - from_qemu_none(ptr: *const sys::P) -> T
> >   Return a Rust type T for a const ffi pointer P.
> >
> > - from_qemu_full(ptr: *mut sys::P) -> T
> >   Return a Rust type T for a ffi pointer P, taking ownership.
> >
> > - T::to_qemu_none() -> Stash<P>
> >   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
> >   storage data, if any).
> >
> > - T::to_qemu_full() -> P
> >   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
> >
> > With those traits, it's relatively easy to implement the QMP callbacks.
> > With enough interest, we could eventually start rewriting QGA in
> > Rust, as it is a simple service. See qga/qmp.rs for some examples.
> > We could also try to tackle qemu itself.
>
> Up to here, you're talking about *internal* interfaces.  Correct?
>
> Your motivation is enabling use of Rust in QEMU.  Correct?
>
> > Finally, given that the QAPI types are easy to serialize, it was simple
> > to use "serde" on them, and provide a D-Bus interface for QMP with zbus.
> > (a similar approach could probably be taken for other protocols, that
> > could be dynamically loaded... anyone like protobuf better?)
>
> QMP is an *external* interface.
>
> It supports compatible evolution: we can make certain kinds of changes
> without affecting clients.  These include:
>
> * Adding optional arguments
>
> * Adding results
>
> * Adding values to an enumeration type, branches to a union or
>   alternate
>
> * Reordering members of enumerations, structs, unions
>
> * Turning an argument type into an alternate with the old type as branch
>
> We've made use of this extensively.  See also
> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
>
> How do such changes affect clients of the proposed D-Bus interface?
>

It's not just about the D-Bus interface though.

QMP being JSON, being lazily typed: everytime we make such changes, we
inflict some pain to all the QMP bindings that want to have a
statically checked & native version of the interface. Iow, we should
think twice before doing any of this.

> > This PoC modifies qemu-ga to provide the interface on the session bus:
> > $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v
> > $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> > ...
> > $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi
> > GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1
> > ...
> >
> > Note: the generated code doesn't work with the qemu schema, there is a
> > couple of fixme/todo left.
> >
> > Shameful pain point: meson & cargo don't play nicely together.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
[...]
>> > Finally, given that the QAPI types are easy to serialize, it was simple
>> > to use "serde" on them, and provide a D-Bus interface for QMP with zbus.
>> > (a similar approach could probably be taken for other protocols, that
>> > could be dynamically loaded... anyone like protobuf better?)
>>
>> QMP is an *external* interface.
>>
>> It supports compatible evolution: we can make certain kinds of changes
>> without affecting clients.  These include:
>>
>> * Adding optional arguments
>>
>> * Adding results
>>
>> * Adding values to an enumeration type, branches to a union or
>>   alternate
>>
>> * Reordering members of enumerations, structs, unions
>>
>> * Turning an argument type into an alternate with the old type as branch
>>
>> We've made use of this extensively.  See also
>> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
>>
>> How do such changes affect clients of the proposed D-Bus interface?
>>
>
> It's not just about the D-Bus interface though.
>
> QMP being JSON, being lazily typed: everytime we make such changes, we
> inflict some pain to all the QMP bindings that want to have a
> statically checked & native version of the interface. Iow, we should
> think twice before doing any of this.

Having to think twice before doing something we need to do all the time
would slow us down.  I don't think this is going to fly.

QMP is designed to avoid tight coupling of server (i.e. QEMU) and
client.  In particular, we don't want "you have to upgrade both in
lockstep".

A well-behaved client works fine even when it's written for a somewhat
older or newer QMP than the server provides.  "Somewhat" because we
deprecate and eventually remove stuff.  Graceful degradation when the
gap gets too large.

There's a gap between the "lose" wire format, and a "tight" statically
typed internal interface.  The gap exists in QEMU, and we bridge it.
Clients can do the same.  Libvirt does: it provides a statically typed
version of the interface without undue coupling.

Replacing the "lose" wire format by something "tighter" like D-Bus
shrinks the gap.  Good.  It also tightens the coupling.  Bad.

[...]

Hi

On Tue, Sep 22, 2020 at 7:39 PM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi
> >
> > On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >>
> >> marcandre.lureau@redhat.com writes:
> [...]
> >> > Finally, given that the QAPI types are easy to serialize, it was
> simple
> >> > to use "serde" on them, and provide a D-Bus interface for QMP with
> zbus.
> >> > (a similar approach could probably be taken for other protocols, that
> >> > could be dynamically loaded... anyone like protobuf better?)
> >>
> >> QMP is an *external* interface.
> >>
> >> It supports compatible evolution: we can make certain kinds of changes
> >> without affecting clients.  These include:
> >>
> >> * Adding optional arguments
> >>
> >> * Adding results
> >>
> >> * Adding values to an enumeration type, branches to a union or
> >>   alternate
> >>
> >> * Reordering members of enumerations, structs, unions
> >>
> >> * Turning an argument type into an alternate with the old type as branch
> >>
> >> We've made use of this extensively.  See also
> >> docs/devel/qapi-code-gen.txt section "Compatibility considerations."
> >>
> >> How do such changes affect clients of the proposed D-Bus interface?
> >>
> >
> > It's not just about the D-Bus interface though.
> >
> > QMP being JSON, being lazily typed: everytime we make such changes, we
> > inflict some pain to all the QMP bindings that want to have a
> > statically checked & native version of the interface. Iow, we should
> > think twice before doing any of this.
>
> Having to think twice before doing something we need to do all the time
> would slow us down.  I don't think this is going to fly.
>
> QMP is designed to avoid tight coupling of server (i.e. QEMU) and
> client.  In particular, we don't want "you have to upgrade both in
> lockstep".
>
> A well-behaved client works fine even when it's written for a somewhat
> older or newer QMP than the server provides.  "Somewhat" because we
> deprecate and eventually remove stuff.  Graceful degradation when the
> gap gets too large.
>
> There's a gap between the "lose" wire format, and a "tight" statically
> typed internal interface.  The gap exists in QEMU, and we bridge it.
> Clients can do the same.  Libvirt does: it provides a statically typed
> version of the interface without undue coupling.
>
> Replacing the "lose" wire format by something "tighter" like D-Bus
> shrinks the gap.  Good.  It also tightens the coupling.  Bad.
>
> [...]
>
>
>

At least, this little D-Bus experiment puts some light on one of the
current QMP weakness: it's hard to bind QMP.

There are good reasons to prefer strongly typed languages. Whenever QMP is
statically bound there, and such changes are made, it is pushing the
versionning issues to others. It's probably one of the reasons why people
are stuck binding QMP manually: doing it automatically would not be
practical, as it would regularly break the interface & build. You have to
version the schema & interface yourself.

So we end up with multiple bindings, manually bound with mistakes etc.

What does this freedom really gives us in exchange? We don't want to commit
to a stable API? It's not rocket science, everybody else does it with
interface version numbers. What makes QEMU/QMP so different?

As for this D-Bus binding, if we don't commit to some better QMP stability
guarantees, we could simply bump the version of the D-Bus interfaces for
each Qemu release (without compatibility with older versions). It's not a
great idea, but it's the best to do then.


-- 
Marc-André Lureau

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

[...]
> What does this freedom really gives us in exchange? We don't want to commit
> to a stable API? It's not rocket science, everybody else does it with
> interface version numbers. What makes QEMU/QMP so different?

It's not rocket science, and we're so used to it that we don't even
notice anymore how awful it is.

When you compile to native code, exact interface match is required for
efficiency.

This used to be pretty much a non-issue: when you compile and link
statically, the only interface remaining at run time is system calls.

Dynamic linking threw us into DLL hell.  Yes, we figured out how to
version symbols, when to bump sonames, and how prepare for and make
binary compatible interface changes.  It's still awful.  People deploy
in containers just to get out of this awful game.  But remember: there's
a *reason*, namely efficiency.

Once you go beyond a single process, you need interprocess
communication.  We use procedure calls for intraprocess communication,
so remote procedure calls are an obvious solution for interprocess
communication.

Where many RPC systems have gone wrong, in my opinion, is bringing along
the awfulness of exact interface matches, with much less of a reason,
but even more awfulness: you now get to also wrestle with multiple
versions of servers fighting over ports and such.

Yes, containers, I know.  They help a lot with keeping such messes under
control.  But some messes are necessary, while others are not.

I respectfully disagree with the notion that "everybody else does it
with interface version numbers".  There's a ton of IPC protocols out
there that do not require exact interface matches.

[...]