dbus2 / zbus-old Goto Github PK

In GitLab by @zeenix on May 23, 2020, 19:14

In GitLab by @zeenix on May 23, 2020, 19:14

The following discussion from !73 should be addressed:

• @zeenix started a discussion: (+8 comments)

  • Same concern as for the experimental proxy api about allocated strings and in this case, wouldn't most uses of this be to readily use (parts of) the introspection tree, rather than keep the tree around for lifetimes to be a pain for the user?

We really should support this but it'll need upstream work, unless we want to rely more on nix crate:

• rust-lang/rust#42048
• async-rs/async-std#623

Run cargo-audit as part of CI.

In GitLab by @zeenix on May 23, 2020, 13:35

In GitLab by @elmarco on May 7, 2020, 19:57

I think the naming can be confusing (it confused me, I just realized we are talking about signature!).

In GitLab by @elmarco on May 7, 2020, 19:57

I think the naming can be confusing (it confused me, I just realized we are talking about signature!).

In GitLab by @elmarco on May 7, 2020, 15:22

The following is bugging the serializer:

    #[test]
    fn struct_with_hashmap_of_value() {
        use crate as zvariant;
        use serde::Serialize;

        let mut hm = HashMap::new();
        hm.insert("key".into(), Value::from(55));

        #[derive(Type, Serialize)]
        struct Foo {
            hmap: HashMap<String, Value<'static>>,
        }

        let foo = Foo { hmap: hm };

        let ctxt = Context::<LE>::new_dbus(0);
        to_bytes(ctxt, &foo).unwrap();
    }

thread 'tests::struct_with_hashmap_of_value' panicked at 'called Result::unwrap()on anErrvalue: Message("invalid type: charactera, expected s, g, oorv")', zvariant/src/lib.rs:738:9

When the struct has another field inside, the error is bit different. That might be another related issue.

The following discussion from !60 should be addressed:

• @zeenix started a discussion:

  Hmm.. we should make these fields private and provide getters. From user's POV unique_name must always be correctly set after Connection is created.

In GitLab by @elmarco on May 15, 2020, 01:03

Before I do some cleanups and propose the proxy API, I could take some feedback. Please focus on externally visible API, all the inner stuff we can evolve later. Also, I focus exclusively on owned version for now, because it is more convenient to use and that's what I need first. I think we can derive a borrowed / callback-based version in a later iteration. It is also missing signals, which will have to be registered through some kind of dispatcher, or other API which are to be defined.

Does the overall Proxy API look ok? This is mostly an internal API for the derive macro to use, but it will be public, and may be used directly if prefered (not a good idea I think).

https://gitlab.freedesktop.org/elmarco/zbus/-/blob/zbus-wip/zbus/examples/server/proxy.rs

pub type ProxyResult<T> = std::result::Result<T, ProxyError>;

impl<'c> Proxy<'c> {
    pub fn new(
        conn: &'c Connection,
        destination: &str,
        path: &str,
        interface: &str,
    ) -> ProxyResult<Self> 

    pub fn introspect(&self) -> ProxyResult<String> 

    pub fn try_get<T>(&self, property_name: &str) -> ProxyResult<T>
    where
        T: TryFrom<OwnedValue>

    pub fn try_set<'a, T: 'a>(&self, property_name: &str, value: T) -> ProxyResult<()>
    where
        T: Into<Value<'a>>

    pub fn call<B, R>(&self, method_name: &str, body: &B) -> ProxyResult<R>
    where
        B: serde::ser::Serialize + zvariant::Type,
        R: serde::de::DeserializeOwned + zvariant::Type
}

• should that be on top-level namespace, or ::proxy or else?
• should it use a seperate Error/Result or reuse Connection's?

For the macro/derive part, I have looked at various DBus APIs (fdo, systemd and polkit + my own experience). A good example usage I propose is: https://gitlab.freedesktop.org/elmarco/zbus/-/blob/zbus-wip/zbus/examples/server/polkit1.rs#L299

#[DBusProxy(
    interface = "org.freedesktop.PolicyKit1.Authority",
    default_service = "org.freedesktop.PolicyKit1",
    default_path = "/org/freedesktop/PolicyKit1/Authority"
)]
trait Authority {
    fn CheckAuthorization(
        subject: &Subject,
        action_id: &str,
        details: std::collections::HashMap<&str, &str>,
        flags: CheckAuthorizationFlags,
        cancellation_id: &str,
    ) -> AuthorizationResult;

    #[DBusProxy(property)]
    fn BackendFeatures() -> AuthorityFeatures;

    /// some other example for getter/setters.
    #[DBusProxy(property)]
    fn KExecWatchdogUSec() -> u64;
    /// A property method that has input args is a setter. By default, it will strip the Set from the name for the property name
    #[DBusProxy(property)]
    fn SetKExecWatchdogUSec(value: u64);

}

The actual derived impl looks something like:

impl Authority {
    // constructors:
    fn new(c: &Connection) -> Self;
    fn new_for(c: &Connection, dest: &str, path &str) -> Self;

    // method call, &self is appended, ProxyResult wraps the returned value 
    fn CheckAuthorization(
        &self,
        subject: &Subject,
        action_id: &str,
        details: std::collections::HashMap<&str, &str>,
        flags: CheckAuthorizationFlags,
        cancellation_id: &str,
    ) -> ProxyResult<AuthorizationResult> {
        self.call("CheckAuthorization", &(subject, action_id,...))
    }

    // same in snake_case
    fn check_authorization(
        &self,
        subject: &Subject,
        action_id: &str,
        details: std::collections::HashMap<&str, &str>,
        flags: CheckAuthorizationFlags,
        cancellation_id: &str,
    ) -> ProxyResult<AuthorizationResult>;

    fn BackendFeatures(&self) -> ProxyResult<AuthorityFeatures> {
       self.try_get("BackendFeatures")
    }
    fn backend_features(&self) -> ProxyResult<AuthorityFeatures>

    fn SetKExecWatchdogUSec(value: u64) -> ProxyResult<()> {
        self.try_set("KExecWatchdogUSec", &value)
    }
    fn set_kexec_watchdog_usec(value: u64) -> ProxyResult<()>; // actually, my snake-case function is buggy atm :)

In the trait,

• should it have the &self explicitly
• similarly, should it have the ProxyResult explicetly too?
  (both would be quite verbose and repetitive, so I decided not to have them by default, and only reflect DBus API)
• again, to better reflect the DBus API, I prefered to use the casing used over DBus by default when declaring the trait, and let the macro derive the rest. Should we instead have snake_case method converted to CamelCase? (with exceptions being edited with additional attributes?)

The traits can be automatically generated from introspection XML, only higher level types (structs, flags) are done manually, since the schema doesn't expose such details.

Perhaps a trait is not the most appropriate to declare the DBus API, but then what should it be? Some adhoc rust-derived description language?

Any other comment?

We really should support this but it'll need upstream work, unless we want to rely more on nix crate:

• rust-lang/rust#42048
• async-rs/async-std#623

We already provide conversion from tuples to Structures and from Structure to Value but it'd be great if we provide a direct conversion from/to tuples from/to Value as well.

In GitLab by @elmarco on Apr 29, 2020, 17:16

The following struct is serialized as "(s...)":

#[derive(Type)]
struct TestStruct {
    name: String,
    ...
}

I think it could be useful if zvariant/derive would have the option to serialize it as "s..." instead. Or is it already possible?

In GitLab by @programmerjake on Nov 27, 2019, 01:58

serde has the benefit of a well thought out API with support for arbitrary protocols, as well as being the defacto standard for Rust. Even if you don't end up using it, emulating the serialization/de-serialization traits would probably be quite helpful. See serde_cbor for a simple example binary data format.

We should provide the following (empty) traits to facilitate trait bounds:

• Serialize: serde::ser::Serialize + Type
• Deserialize trait: serde::ser::Deserialize + Type

Current implementation creates bytes representation of the entire value to be serialized. I think we can avoid that by create a separate serializer just for size and just calculate the size in there.

The following discussion from !73 should be addressed:

• @zeenix started a discussion: (+8 comments)

  • Same concern as for the experimental proxy api about allocated strings and in this case, wouldn't most uses of this be to readily use (parts of) the introspection tree, rather than keep the tree around for lifetimes to be a pain for the user?

In GitLab by @SDavidoff on Feb 8, 2020, 19:18

The layout of D-bus messages is very friendly towards zero-copy decoding: since everything is already properly aligned, you can access the data in memory directly instead of allocating and copying it to the outside.

This can be done without major changes to the API since this is only relevant for strings and arrays, not primitive types. All you'd need to add is converting zvariant::Array into a slice instead of a Vec and zvariant::String into an &str.

For &str there is already a function to perform the checks and convert: https://doc.rust-lang.org/std/str/fn.from_utf8.html

All array types except bool are covered by try_cast() methods from bytemuck crate: https://docs.rs/bytemuck/1.2.0/bytemuck/
&[bool] has additional validity invariants (only values of 0 or 1 are allowed) and will require a traversal for validation, and a manual unsafe cast.

The only tricky part is that you'd need to align the entirety of zvariant heap allocation to the largest possible alignment requirement (8 bytes for u64) so you might have to add up to 7 padding bytes before the zvariant itself even begins.

In GitLab by @elmarco on May 7, 2020, 15:22

The following is bugging the serializer:

    #[test]
    fn struct_with_hashmap_of_value() {
        use crate as zvariant;
        use serde::Serialize;

        let mut hm = HashMap::new();
        hm.insert("key".into(), Value::from(55));

        #[derive(Type, Serialize)]
        struct Foo {
            hmap: HashMap<String, Value<'static>>,
        }

        let foo = Foo { hmap: hm };

        let ctxt = Context::<LE>::new_dbus(0);
        to_bytes(ctxt, &foo).unwrap();
    }

thread 'tests::struct_with_hashmap_of_value' panicked at 'called Result::unwrap()on anErrvalue: Message("invalid type: charactera, expected s, g, oorv")', zvariant/src/lib.rs:738:9

When the struct has another field inside, the error is bit different. That might be another related issue.

In GitLab by @SDavidoff on Feb 8, 2020, 18:13

The dependency on byteorder crate can be easily avoided, since all the relevant functions are already provided by the standard library (including handling endianness).

Example of stdlib replacement for read_ methods can be found here: https://github.com/cbreeden/fxhash/pull/13/files

Writes can be done through to_ne_bytes: https://doc.rust-lang.org/stable/std/primitive.u32.html#method.to_ne_bytes

The change should be quite trivial, but sadly I am under legal restrictions and cannot contribute to this repository.

Run cargo-audit as part of CI.

The current Decode::decode implementations decodes all the elements upfront, which is very inefficient. It's not a problem in most cases in the context of D-Bus but GVariant was designed to avoid exactly this kind of inefficiency since it's also meant to contain large sets of data.

We probably want an additional API that can deserialize only specific range from an encoded array. It's OK if we can only make it work for GVariant format.

When the length is provided, we don't need to seek back to write the length of the sequence and can calculate and write it upfront. It's unfortunate that the length is not guaranteed so we could have removed the Seek requirement on the passed writer.

As we've always planned. The spec is here. For primitive types it's exactly the same so it shouldn't be very hard. For container types and strings, the most important difference is that GVariant keeps metadata at the end, not beginning.

We should test against glib's API to ensure compatibility and maybe also compare the performance diff, if any. Some very WIP work exists in this branch: https://gitlab.freedesktop.org/zeenix/zbus/-/tree/wip/gvariant

Checklist:

• Ensure we always encode and decode in the right byte-order.
• Deserializer code for container types must deserialize from child value from their respective slices only. This likely means separate Deserializer instance(s) for child values.
• Deserializer assumes byte slice contains only the type currently being decoded, for basic types, this is only relevant to string (including signature & object path) deserialization.
• Support for Maybe type, which should map nicely to Option<T: Type>
• (OPTIONAL) It would be great if we could also add integration tests against Ostree.

Current implementation creates bytes representation of the entire value to be serialized. I think we can avoid that by create a separate serializer just for size and just calculate the size in there.

In GitLab by @tim-seoss on Apr 14, 2020, 16:24

Hello,

I know nearly nothing about dbus, but I'd like to write a small tool in Rust to pull out this sort of data from ModemManager:

dbus-send --system --print-reply --dest=org.freedesktop.ModemManager1 /org/freedesktop/ModemManager1/Modem/0 org.freedesktop.DBus.Properties.Get string:org.freedesktop.ModemManager1.Modem.Signal string:Lte
method return time=1586873244.543342 sender=:1.7818 -> destination=:1.7921 serial=10053 reply_serial=2
   variant       array [
         dict entry(
            string "rssi"
            variant                double -46
         )
         dict entry(
            string "rsrp"
            variant                double -76
         )
         dict entry(
            string "rsrq"
            variant                double -11
         )
         dict entry(
            string "snr"
            variant                double 8.4
         )
      ]

... so thought I would look at zbus to accomplish this. I noticed that only the session bus is currently supported, so thought I'd hack on it a bit to see if I could get system connections working. As a PoC I changed the connection socket path to /var/run/dbus/system_bus_socket and tried to print out the result of: GetMachineId.

Whilst the debug output seems to start OK (debug output shows that Hello succeeds, and the system bus name is printed out), the call_method to GetMachineID (which succeeds against the session bus) returns an Err(Variant(IncorrectType)) on the system bus.

As a relative Rust and Dbus newbie, should I carry on hacking on this, and if-so, any tips?

In GitLab by @programmerjake on Nov 27, 2019, 01:58

serde has the benefit of a well thought out API with support for arbitrary protocols, as well as being the defacto standard for Rust. Even if you don't end up using it, emulating the serialization/de-serialization traits would probably be quite helpful. See serde_cbor for a simple example binary data format.

like we do in zvariant.

In GitLab by @SDavidoff on Feb 8, 2020, 19:18

The layout of D-bus messages is very friendly towards zero-copy decoding: since everything is already properly aligned, you can access the data in memory directly instead of allocating and copying it to the outside.

This can be done without major changes to the API since this is only relevant for strings and arrays, not primitive types. All you'd need to add is converting zvariant::Array into a slice instead of a Vec and zvariant::String into an &str.

For &str there is already a function to perform the checks and convert: https://doc.rust-lang.org/std/str/fn.from_utf8.html

All array types except bool are covered by try_cast() methods from bytemuck crate: https://docs.rs/bytemuck/1.2.0/bytemuck/
&[bool] has additional validity invariants (only values of 0 or 1 are allowed) and will require a traversal for validation, and a manual unsafe cast.

The only tricky part is that you'd need to align the entirety of zvariant heap allocation to the largest possible alignment requirement (8 bytes for u64) so you might have to add up to 7 padding bytes before the zvariant itself even begins.

In GitLab by @elmarco on May 15, 2020, 01:03

Before I do some cleanups and propose the proxy API, I could take some feedback. Please focus on externally visible API, all the inner stuff we can evolve later. Also, I focus exclusively on owned version for now, because it is more convenient to use and that's what I need first. I think we can derive a borrowed / callback-based version in a later iteration. It is also missing signals, which will have to be registered through some kind of dispatcher, or other API which are to be defined.

Does the overall Proxy API look ok? This is mostly an internal API for the derive macro to use, but it will be public, and may be used directly if prefered (not a good idea I think).

https://gitlab.freedesktop.org/elmarco/zbus/-/blob/zbus-wip/zbus/examples/server/proxy.rs

pub type ProxyResult<T> = std::result::Result<T, ProxyError>;

impl<'c> Proxy<'c> {
    pub fn new(
        conn: &'c Connection,
        destination: &str,
        path: &str,
        interface: &str,
    ) -> ProxyResult<Self> 

    pub fn introspect(&self) -> ProxyResult<String> 

    pub fn try_get<T>(&self, property_name: &str) -> ProxyResult<T>
    where
        T: TryFrom<OwnedValue>

    pub fn try_set<'a, T: 'a>(&self, property_name: &str, value: T) -> ProxyResult<()>
    where
        T: Into<Value<'a>>

    pub fn call<B, R>(&self, method_name: &str, body: &B) -> ProxyResult<R>
    where
        B: serde::ser::Serialize + zvariant::Type,
        R: serde::de::DeserializeOwned + zvariant::Type
}

• should that be on top-level namespace, or ::proxy or else?
• should it use a seperate Error/Result or reuse Connection's?

For the macro/derive part, I have looked at various DBus APIs (fdo, systemd and polkit + my own experience). A good example usage I propose is: https://gitlab.freedesktop.org/elmarco/zbus/-/blob/zbus-wip/zbus/examples/server/polkit1.rs#L299

#[DBusProxy(
    interface = "org.freedesktop.PolicyKit1.Authority",
    default_service = "org.freedesktop.PolicyKit1",
    default_path = "/org/freedesktop/PolicyKit1/Authority"
)]
trait Authority {
    fn CheckAuthorization(
        subject: &Subject,
        action_id: &str,
        details: std::collections::HashMap<&str, &str>,
        flags: CheckAuthorizationFlags,
        cancellation_id: &str,
    ) -> AuthorizationResult;

    #[DBusProxy(property)]
    fn BackendFeatures() -> AuthorityFeatures;

    /// some other example for getter/setters.
    #[DBusProxy(property)]
    fn KExecWatchdogUSec() -> u64;
    /// A property method that has input args is a setter. By default, it will strip the Set from the name for the property name
    #[DBusProxy(property)]
    fn SetKExecWatchdogUSec(value: u64);

}

The actual derived impl looks something like:

impl Authority {
    // constructors:
    fn new(c: &Connection) -> Self;
    fn new_for(c: &Connection, dest: &str, path &str) -> Self;

    // method call, &self is appended, ProxyResult wraps the returned value 
    fn CheckAuthorization(
        &self,
        subject: &Subject,
        action_id: &str,
        details: std::collections::HashMap<&str, &str>,
        flags: CheckAuthorizationFlags,
        cancellation_id: &str,
    ) -> ProxyResult<AuthorizationResult> {
        self.call("CheckAuthorization", &(subject, action_id,...))
    }

    // same in snake_case
    fn check_authorization(
        &self,
        subject: &Subject,
        action_id: &str,
        details: std::collections::HashMap<&str, &str>,
        flags: CheckAuthorizationFlags,
        cancellation_id: &str,
    ) -> ProxyResult<AuthorizationResult>;

    fn BackendFeatures(&self) -> ProxyResult<AuthorityFeatures> {
       self.try_get("BackendFeatures")
    }
    fn backend_features(&self) -> ProxyResult<AuthorityFeatures>

    fn SetKExecWatchdogUSec(value: u64) -> ProxyResult<()> {
        self.try_set("KExecWatchdogUSec", &value)
    }
    fn set_kexec_watchdog_usec(value: u64) -> ProxyResult<()>; // actually, my snake-case function is buggy atm :)

In the trait,

• should it have the &self explicitly
• similarly, should it have the ProxyResult explicetly too?
  (both would be quite verbose and repetitive, so I decided not to have them by default, and only reflect DBus API)
• again, to better reflect the DBus API, I prefered to use the casing used over DBus by default when declaring the trait, and let the macro derive the rest. Should we instead have snake_case method converted to CamelCase? (with exceptions being edited with additional attributes?)

The traits can be automatically generated from introspection XML, only higher level types (structs, flags) are done manually, since the schema doesn't expose such details.

Perhaps a trait is not the most appropriate to declare the DBus API, but then what should it be? Some adhoc rust-derived description language?

Any other comment?

In GitLab by @SDavidoff on Feb 8, 2020, 18:13

The dependency on byteorder crate can be easily avoided, since all the relevant functions are already provided by the standard library (including handling endianness).

Example of stdlib replacement for read_ methods can be found here: https://github.com/cbreeden/fxhash/pull/13/files

Writes can be done through to_ne_bytes: https://doc.rust-lang.org/stable/std/primitive.u32.html#method.to_ne_bytes

The change should be quite trivial, but sadly I am under legal restrictions and cannot contribute to this repository.

The following discussion from !60 should be addressed:

• @zeenix started a discussion:

  Hmm.. we should make these fields private and provide getters. From user's POV unique_name must always be correctly set after Connection is created.

In GitLab by @elmarco on Apr 29, 2020, 17:16

The following struct is serialized as "(s...)":

#[derive(Type)]
struct TestStruct {
    name: String,
    ...
}

I think it could be useful if zvariant/derive would have the option to serialize it as "s..." instead. Or is it already possible?

Would be nice if that's possible. No 'no-alloc' though and we'll need to use alloc crate. See how it's done in serde.

Don't forget to update the docs, when this is added.

In GitLab by @tim-seoss on Apr 14, 2020, 16:24

Hello,

I know nearly nothing about dbus, but I'd like to write a small tool in Rust to pull out this sort of data from ModemManager:

dbus-send --system --print-reply --dest=org.freedesktop.ModemManager1 /org/freedesktop/ModemManager1/Modem/0 org.freedesktop.DBus.Properties.Get string:org.freedesktop.ModemManager1.Modem.Signal string:Lte
method return time=1586873244.543342 sender=:1.7818 -> destination=:1.7921 serial=10053 reply_serial=2
   variant       array [
         dict entry(
            string "rssi"
            variant                double -46
         )
         dict entry(
            string "rsrp"
            variant                double -76
         )
         dict entry(
            string "rsrq"
            variant                double -11
         )
         dict entry(
            string "snr"
            variant                double 8.4
         )
      ]

... so thought I would look at zbus to accomplish this. I noticed that only the session bus is currently supported, so thought I'd hack on it a bit to see if I could get system connections working. As a PoC I changed the connection socket path to /var/run/dbus/system_bus_socket and tried to print out the result of: GetMachineId.

Whilst the debug output seems to start OK (debug output shows that Hello succeeds, and the system bus name is printed out), the call_method to GetMachineID (which succeeds against the session bus) returns an Err(Variant(IncorrectType)) on the system bus.

As a relative Rust and Dbus newbie, should I carry on hacking on this, and if-so, any tips?

We should provide the following (empty) traits to facilitate trait bounds:

• Serialize: serde::ser::Serialize + Type
• Deserialize trait: serde::ser::Deserialize + Type

As we've always planned. The spec is here. For primitive types it's exactly the same so it shouldn't be very hard. For container types and strings, the most important difference is that GVariant keeps metadata at the end, not beginning.

We should test against glib's API to ensure compatibility and maybe also compare the performance diff, if any. Some very WIP work exists in this branch: https://gitlab.freedesktop.org/zeenix/zbus/-/tree/wip/gvariant

Checklist:

• Ensure we always encode and decode in the right byte-order.
• Deserializer code for container types must deserialize from child value from their respective slices only. This likely means separate Deserializer instance(s) for child values.
• Deserializer assumes byte slice contains only the type currently being decoded, for basic types, this is only relevant to string (including signature & object path) deserialization.
• Support for Maybe type, which should map nicely to Option<T: Type>
• (OPTIONAL) It would be great if we could also add integration tests against Ostree.

In GitLab by @zeenix on Dec 31, 2019, 12:26

We should decode messages based on the byte order on the first header field, rather than error out on non-native byte order.

like we do in zvariant.

At the moment, it'll save us the following steps in each stage run:

apt-get --allow-unauthenticated update -yq
apt-get install -o dir::cache::archives="$APT_CACHE_DIR" -y dbus

Maybe use gitlab's template support?

At the moment, it'll save us the following steps in each stage run:

apt-get --allow-unauthenticated update -yq
apt-get install -o dir::cache::archives="$APT_CACHE_DIR" -y dbus

Maybe use gitlab's template support?

In GitLab by @zeenix on May 23, 2020, 13:35

We should decode messages based on the byte order on the first header field, rather than error out on non-native byte order.

Would be nice if that's possible. No 'no-alloc' though and we'll need to use alloc crate. See how it's done in serde.

Don't forget to update the docs, when this is added.

The current Decode::decode implementations decodes all the elements upfront, which is very inefficient. It's not a problem in most cases in the context of D-Bus but GVariant was designed to avoid exactly this kind of inefficiency since it's also meant to contain large sets of data.

We probably want an additional API that can deserialize only specific range from an encoded array. It's OK if we can only make it work for GVariant format.

In GitLab by @zeenix on Dec 31, 2019, 12:26

We already provide conversion from tuples to Structures and from Structure to Value but it'd be great if we provide a direct conversion from/to tuples from/to Value as well.

When the length is provided, we don't need to seek back to write the length of the sequence and can calculate and write it upfront. It's unfortunate that the length is not guaranteed so we could have removed the Seek requirement on the passed writer.