Find solution for DBusError that works for clients and interfaces

[zeenix]

In GitLab by @rosslannen on Aug 25, 2021, 22:46

The following discussion from !368 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.

[zeenix]

@rosslannen Hi, quite a bit has changed wrt to this in main branch and also we don't panic on interface methods returning zbus::Error anymore. Could you kindly check if this issue is still relevant?

[zeenix]

In GitLab by @rosslannen on Sep 28, 2021, 18:09

@zeenix I like the direction that the changes are headed. It is a definite improvement over when this issue was first raised.

However, there are still a couple of strange things I see still while looking at it. The new DBusError trait is great, but it only has the one method - however, the derive macro still looks like it generates methods on the struct that are not part of the trait (name(), description(), and reply()). I would see this as pretty unexpected behavior, especially when all derives in std - from what I can remember - just derive the trait, and nothing else. This is also pretty consistent with other derive macros, such as serde or thiserror. I think those methods should either be moved into the DBusError trait if they are important to it, or removed from the DBusError derive. It looks like there are some other traits that are automatically derived that I am not sure should be - at least not automatically, with no other input. In general, I prefer something like the approach that thiserror takes: just the derive generates just the trait, but then other stuff can be added using the #[error(...)] attribute or #[from] attribute within the enum to generate extra stuff.

The current solution also doesn't solve one of my original issues with the trait, as it looks like it still requires a ZBus(zbus::Error) variant on the enum in order to be used, even if it is only used on the server side.

Something that could work for this would be to have a #[dbus_error(...)] attribute that goes along with the derive that could be added to control things like generating a Display impl or creating the From<zbus::Error> impl (or maybe even other From<OtherError> impls). And then to use the error in client use cases, it would just have a trait bound of DBusError + From<zbus::Error> and the server side would just require DBusError. That way it doesn't force the user to create that ZBus variant on the enum for server use only.

I don't have a lot of experience writing macros, but I could take a try at creating an implementation this week. Stuff at work has lightened up a bit, and I have some time available.

[zeenix]

@rosslannen thanks for explaining!

the new DBusError trait is great, but it only has the one method - however, the derive macro still looks like it generates methods on the struct that are not part of the trait (name(), description(), and reply()).

Oh, somehow I missed that part. It should have been done, yes. However, this part would be easiest to fix.

It looks like there are some other traits that are automatically derived that I am not sure should be - at least not automatically, with no other input.

I think the idea here was that you'd use this macro to easily create an error type to use on D-Bus, rather than making your error type work with D-Bus. However, I see the value in the latter.

Something that could work for this would be to have a #[dbus_error(...)] attribute that goes along with the derive that could be added to control things like generating a Display impl or creating the From<zbus::Error> impl (or maybe even other From<OtherError> impls). And then to use the error in client use cases, it would just have a trait bound of DBusError + From<zbus::Error> and the server side would just require DBusError. That way it doesn't force the user to create that ZBus variant on the enum for server use only.

Hm.. yeah that could work. However, since you say that you've a bit of a learn curve on writing macros, I'd suggest splitting this off (into a separate issue/MR) and first the get easier bits done. :)

Stuff at work has lightened up a bit, and I have some time available.

Awesome! :)

[zeenix]

mentioned in commit d842f8f23c02434bfba73800da0418c65fc56e07