Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE REQUEST] Derive D-Bus supported Variants #1594

Open
sophie-h opened this issue Dec 9, 2024 · 15 comments
Open

[FEATURE REQUEST] Derive D-Bus supported Variants #1594

sophie-h opened this issue Dec 9, 2024 · 15 comments
Labels
enhancement New feature or request

Comments

@sophie-h
Copy link
Contributor

sophie-h commented Dec 9, 2024

I think the main issue here is with maybe types m…. This could be solved by using a separate DbusOption type that is represented by a…. But I would prefer if there was an option for the derive macro to do this with std Options instead.

Another thing is that many D-Bus APIs use a{sv} a lot to be extendible. Therefore, it would be nice if this representation could be derived from struct S { x: Option<A>, y: Option<B> }. I think something similar is already supported for enums.

@sophie-h sophie-h added the enhancement New feature or request label Dec 9, 2024
@sdroege
Copy link
Member

sdroege commented Dec 9, 2024

I think the main issue here is with maybe types m…. This could be solved by using a separate DbusOption type that is represented by a…. But I would prefer if there was an option for the derive macro to do this with std Options instead.

The problem here is that macros don't have type information, so the best you can do is to hope that what is called Option is actually the one from std. The alternative would be to use a DBusOption type.

So GVariant has maybe types but DBus doesn't, and in DBus they're expressed as an empty array?

Another thing is that many D-Bus APIs use a{sv} a lot to be extendible. Therefore, it would be nice if this representation could be derived from struct S { x: Option<A>, y: Option<B> }. I think something similar is already supported for enums.

You mean struct S { k: String, v: Option<Variant> }? That's basically glib::VariantEntry / glib::VariantDict if I'm not missing something.

@sophie-h
Copy link
Contributor Author

sophie-h commented Dec 9, 2024

The problem here is that macros don't have type information, so the best you can do is to hope that what is called Option is actually the one from std. The alternative would be to use a DBusOption type.

Meh, true. For now, using a custom option is certainly the way to go.

In theory, all the maybes could probably be replaced after the to_variant(). But that would involve traversing the all the containers etc and trying Any::downcast on all of them.

So GVariant has maybe types but DBus doesn't, and in DBus they're expressed as an empty array?

Exactly. The empty array or array with one element is the most common workaround.

You mean struct S { k: String, v: Option<Variant> }? That's basically glib::VariantEntry / glib::VariantDict if I'm not missing something.

No, I think I mean something different.

If you have a

S {
    x: Some(A),
    y: None,
    z: Some(C)
}

You encode that as a dict with { "x": Value(B), "z": Value(C) }, if that makes sense.

@sdroege
Copy link
Member

sdroege commented Dec 9, 2024

No, I think I mean something different.

Oh I see. Yes you could add that to the derive attribute as another way to derive the serialized format of a struct. Just like there are multiple options already for enums.

@sophie-h
Copy link
Contributor Author

sophie-h commented Dec 9, 2024

Here is a usage example: https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.Print.html

Almost all portals take an a{sv} for being able to add optional parameters later without having to change the D-Bus API.

@sophie-h
Copy link
Contributor Author

sophie-h commented Dec 9, 2024

I think we will need somewhat of a duplication for everything D-Bus. Mainly because of file descriptors. The "handle" Variant type is an index in the GUnixFdList which exists outside the Variants.

pub trait ToDBusVariant {
    fn to_dbus_variant(&self, fd_list: &UnixFDList) -> Result<glib::Variant, glib::Error>;
}

pub trait FromDBusVariant: Sized {
    fn from_dbus_variant(variant: &glib::Variant, fd_list: &UnixFDList) -> Option<Self>;
}

impl ToDBusVariant for OwnedFd {
    fn to_dbus_variant(&self, fd_list: &UnixFDList) -> Result<glib::Variant, glib::Error> {
        let new_index = fd_list.length();
        fd_list.append(self.as_raw_fd())?;

        unsafe { Ok(from_glib_none(glib::ffi::g_variant_new_handle(new_index))) }
    }
}

impl FromDBusVariant for OwnedFd {
    fn from_dbus_variant(variant: &glib::Variant, fd_list: &UnixFDList) -> Option<Self> {
        let variant = variant.to_glib_none().0;
        let index = unsafe { glib::ffi::g_variant_get_handle(variant) };
        let fd = fd_list.get(index).ok()?;
        unsafe { Some(OwnedFd::from_raw_fd(fd)) }
    }
}

That's the idea that I currently have. Types like bool, u32 etc. are easily covered.

A bigger problem is that we would need a glib::DBusVariant derive macro that internally uses *DBusVariant function internally everywhere.

@sophie-h
Copy link
Contributor Author

sophie-h commented Dec 9, 2024

Maybe more like

pub trait ToDBusVariant {
    fn to_dbus_variant(&self) -> Result<(glib::Variant, UnixFDList), glib::Error> {
        let fd_list = UnixFDList::new();
        let variant = self.to_dbus_sub_variant(&fd_list)?;
        Ok((variant, fd_list))
    }

    fn to_dbus_sub_variant(&self, fd_list: &UnixFDList) -> Result<glib::Variant, glib::Error>;
}

@sdroege
Copy link
Member

sdroege commented Dec 10, 2024

How does this work in C? Can you show an example that uses fds there, and also how that would nowadays look like in Rust without making use of new traits?

@sophie-h
Copy link
Contributor Author

It think it's not possible with safe rust atm.

The main issue that the current Value derives can't cover is that the list of fds is sent independently from the method arguments. The method arguments only contain the index from the list of fds that are passed elsewhere. Therefore, a method that wants to retrieve an OwnedFd from a Value, also needs to have the list of fds, since Value only contains the index.

Here are some oversimplified examples that assume that one FD value is sent/received.

Calling a method

let fd = std::fs::File::open("test").unwrap().as_fd().try_clone_to_owned().unwrap();

let fd_list = gio::UnixFDList::new();
fd_list.append(fd.as_raw_fd().unwrap();

// First entry in the fd_list, therefore index 0
let args = unsafe { from_glib_none(glib::ffi::g_variant_new_handle(0) };

let result = self.proxy.call_with_unix_fd_list_future(
    "FunctionName",
    Some(&args),
    gio::DBusCallFlags::NONE,
    1000,
    Some(&fd_list)
).await;

Handling a method call

let fd_list = invocation.message().unix_fd_list().unwrap();

let variant = parameters.to_glib_none().0;
let index = unsafe { glib::ffi::g_variant_get_handle(variant) };
let fd = fd_list.get(index).unwrap();

/* calculate reply to return "value" containing the fds in "fd_list" */

invocation.return_value_with_unix_fd_list(Some(value), Some(fd_list));

@sophie-h
Copy link
Contributor Author

Here is the C example that's linked in the docs https://gitlab.gnome.org/GNOME/glib/-/blob/HEAD/gio/tests/gdbus-example-unix-fd-client.c

@sdroege
Copy link
Member

sdroege commented Dec 11, 2024

Ah I see. The problem is that the fd list is completely separate from the variants, and the variants only contain an index into that list. I'm not sure how your trait idea would solve that in general (e.g. you could have multiple parameters or return values that are fds so you somehow need to combine the whole argument list into variants and fds instead of per argument).

Maybe another approach would be to have some macro on top for doing methods calls that a) creates the variants and the fd list and makes sure the indices match up, b) does the reverse for the return value? And similar in the other direction for handling method calls inside an object (which probably also means the current trait for that is insufficient).

@sophie-h
Copy link
Contributor Author

I'm not sure how your trait idea would solve that in general (e.g. you could have multiple parameters or return values that are fds so you somehow need to combine the whole argument list into variants and fds instead of per argument).

You call to_dbus_variant on the highest level (for example when you need to pass something as method argument). This starts with a fresh UnixFDList. Every to_dbus_sub_variant() that works with containers, passes that list to the to_dbus_sub_variant() calls for the children. If somewhere in the tree this hits an OwnedFd you get this code from above

        let new_index = fd_list.length();
        fd_list.append(self.as_raw_fd())?;

        unsafe { Ok(from_glib_none(glib::ffi::g_variant_new_handle(new_index))) }

which adds to the UnixFDList and creates a handle Value with the correct index. At the top, to_dbus_variant() returns the UnixFDList which contains all FDs in the correct order.

Maybe another approach would be to have some macro on top for doing methods calls that a) creates the variants and the fd list and makes sure the indices match up, b) does the reverse for the return value? And similar in the other direction for handling method calls inside an object (which probably also means the current trait for that is insufficient).

a) is already done by ToDBusVariant and b) is done by FromDBusVariant (you pass the UnixFDList you got from the API). I also have the macros that do that automatically for method args and return values on client and server side.

I will clean all of the proof of concept stuff up such that it's closer to something mergeable. But I might also try out what I have first to see if it actually work in glycin.

@sdroege
Copy link
Member

sdroege commented Dec 11, 2024

Ok, let's see how that goes. To avoid having to redo all the u32 etc implementations you could probably add a blanket implementation around ToVariant etc?

@sophie-h
Copy link
Contributor Author

To avoid having to redo all the u32 etc implementations you could probably add a blanket implementation around ToVariant etc?

yep, I already have

macro_rules! impl_dbus_variant {
    ($t:ty) => {
        impl StaticDBusVariantType for $t {
            fn static_dbus_variant_type() -> std::borrow::Cow<'static, glib::VariantTy> {
                Self::static_variant_type()
            }
        }

        impl ToDBusVariant for $t {
            fn to_dbus_sub_variant(
                &self,
                _fd_list: &UnixFDList,
            ) -> Result<glib::Variant, glib::Error> {
                Ok(self.to_variant())
            }
        }

        impl FromDBusVariant for $t {
            fn from_dbus_variant(variant: &glib::Variant, _fd_list: &UnixFDList) -> Option<Self> {
                Self::from_variant(variant)
            }
        }
    };
}

impl_dbus_variant!(bool);
impl_dbus_variant!(u32);
impl_dbus_variant!(String);
impl_dbus_variant!(());

@sdroege
Copy link
Member

sdroege commented Dec 11, 2024

I meant

impl<T: ToVariant> ToDBusVariant for T {
    fn to_dbus_sub_variant(&self, _fd_list: &UnixFDList) -> Result<glib::Variant, glib::Error> {
        Ok(self.to_variant())
    }
}

@sophie-h
Copy link
Contributor Author

That kinda works, but doesn't have a lot of benefits. But it stops us from implementing a separate DBusVariant implementation for Option<T>, giving us DBusVariant derives that actually don't work with D-Bus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants