Find solution for DBusError that works for clients and interfaces

The following discussion from !368 (merged) should be addressed:

@rosslannen started a discussion: (+11 comments)

This does eliminate the usage of zbus::Error as a return type from interface methods, as it does not have a reply() method.

Not sure I follow this part. fdo::Error doesn't have that method either. 🤔

According to the documentation, and the macro expansion of the zbus::fdo module (cargo expand --manifest-path zbus/Cargo.toml fdo):

        pub fn reply(
            &self,
            c: &::zbus::Connection,
            call: &::zbus::Message,
        ) -> ::std::result::Result<u32, ::zbus::Error> {
            let name = self.name();
            match self {
                Self::Failed(f0) => c.reply_error(call, name, &(f0)),
                Self::NoMemory(f0) => c.reply_error(call, name, &(f0)),
                Self::ServiceUnknown(f0) => c.reply_error(call, name, &(f0)),
                Self::NameHasNoOwner(f0) => c.reply_error(call, name, &(f0)),
                Self::NoReply(f0) => c.reply_error(call, name, &(f0)),
                Self::IOError(f0) => c.reply_error(call, name, &(f0)),
                Self::BadAddress(f0) => c.reply_error(call, name, &(f0)),
                Self::NotSupported(f0) => c.reply_error(call, name, &(f0)),
                Self::LimitsExceeded(f0) => c.reply_error(call, name, &(f0)),
                Self::AccessDenied(f0) => c.reply_error(call, name, &(f0)),
                Self::AuthFailed(f0) => c.reply_error(call, name, &(f0)),
                Self::NoServer(f0) => c.reply_error(call, name, &(f0)),
                Self::Timeout(f0) => c.reply_error(call, name, &(f0)),
                Self::NoNetwork(f0) => c.reply_error(call, name, &(f0)),
                Self::AddressInUse(f0) => c.reply_error(call, name, &(f0)),
                Self::Disconnected(f0) => c.reply_error(call, name, &(f0)),
                Self::InvalidArgs(f0) => c.reply_error(call, name, &(f0)),
                Self::FileNotFound(f0) => c.reply_error(call, name, &(f0)),
                Self::FileExists(f0) => c.reply_error(call, name, &(f0)),
                Self::UnknownMethod(f0) => c.reply_error(call, name, &(f0)),
                Self::UnknownObject(f0) => c.reply_error(call, name, &(f0)),
                Self::UnknownInterface(f0) => c.reply_error(call, name, &(f0)),
                Self::UnknownProperty(f0) => c.reply_error(call, name, &(f0)),
                Self::PropertyReadOnly(f0) => c.reply_error(call, name, &(f0)),
                Self::TimedOut(f0) => c.reply_error(call, name, &(f0)),
                Self::MatchRuleNotFound(f0) => c.reply_error(call, name, &(f0)),
                Self::MatchRuleInvalid(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnExecFailed(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnForkFailed(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnChildExited(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnChildSignaled(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnFailed(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnFailedToSetup(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnConfigInvalid(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnServiceNotValid(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnServiceNotFound(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnPermissionsInvalid(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnFileInvalid(f0) => c.reply_error(call, name, &(f0)),
                Self::SpawnNoMemory(f0) => c.reply_error(call, name, &(f0)),
                Self::UnixProcessIdUnknown(f0) => c.reply_error(call, name, &(f0)),
                Self::InvalidSignature(f0) => c.reply_error(call, name, &(f0)),
                Self::InvalidFileContent(f0) => c.reply_error(call, name, &(f0)),
                Self::SELinuxSecurityContextUnknown(f0) => c.reply_error(call, name, &(f0)),
                Self::AdtAuditDataUnknown(f0) => c.reply_error(call, name, &(f0)),
                Self::ObjectPathInUse(f0) => c.reply_error(call, name, &(f0)),
                Self::InconsistentMessage(f0) => c.reply_error(call, name, &(f0)),
                Self::InteractiveAuthorizationRequired(f0) => c.reply_error(call, name, &(f0)),
                Self::NotContainer(f0) => c.reply_error(call, name, &(f0)),
                Self::ZBus(_) => ::std::rt::begin_panic("Can not reply with ZBus error type"),
            }
        }

zbus::fdo::Error does have a reply() method. It looks like it comes from the #[derive(DBusError)].

However, before this change returning an Err(zbus::Error::...) from an interface method resulted in a panic, which I think is unexpected behavior. This removes that from happening.

Just for the record, this wording makes the problem sound much worse that it is. Panics in macros are very normal and more like errors since you see them at the compile-time.

I apologize for the mis-wording of this, but as you can see above it is actually a runtime panic in the code generated by a macro, not a panic in the macro generated code itself. The minimum reproduction of the unexpected behavior I am taking about is below:

use std::convert::TryInto;

use zbus::{
    dbus_interface,
    fdo::{DBusProxy, RequestNameFlags},
    Connection, ObjectServer,
};

struct Greeter;

#[dbus_interface(name = "org.zbus.Greeter")]
impl Greeter {
    /// Method that returns `zbus::Result<T>`.
    fn zbus_method(&self) -> zbus::Result<u32> {
        Err(zbus::Error::Unsupported)
    }

    /// Method that returns `zbus::fdo::Result<u32>`.
    fn fdo_method(&self) -> zbus::fdo::Result<u32> {
        Err(zbus::fdo::Error::Failed("failure".to_string()))
    }
}

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let connection = Connection::session()?;

    let dbus = DBusProxy::new(&connection)?;
    dbus.request_name(
        "org.zbus.Greeter".try_into().unwrap(),
        RequestNameFlags::ReplaceExisting.into(),
    )?;

    let mut object_server = ObjectServer::new(&connection);

    let greeter = Greeter;

    object_server.at("/org/zbus/Greeter", greeter)?;

    loop {
        if let Err(err) = object_server.try_handle_next() {
            eprintln!("{}", err);
        }
    }
}

Calling fdo_method() from a dbus-client (I'm using D-Feet) returns back org.freedesktop.DBus.Error.Failed: failure over the D-Bus, as expected. However, calling zbus_method() returns org.freedesktop.DBus.Error.NoReply: Message recipient disconnected from message bus without replying (4), and the application panics with: Can not reply with ZBus error type. I would consider this unexpected behavior - one compiling error type returns an error to the client, and the other compiling error crashes the server.

The only remaining downside to this entire solution is that all errors created with #[derive(DBusError)] must have the ZBus(zbus::Error) variant on them, and the generated reply() method will panic if that variant is encountered. However, for server errors I don't think this should be a requirement. Maybe if there was a way to make that specific variant optional for the #[derive(DBusError)] and only required for client returned errors, or make a different derive for server errors. That is up for discussion, but probably out of scope of this merge request.

@rosslannen Many thanks for providing a merge-request btw!

You are welcome. It is an awesome library. Used it a ton recently, would love to keep contributing.

Edited Aug 26, 2021 by Zeeshan Ali Khan

Admin message

Find solution for DBusError that works for clients and interfaces