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

Use standard library OwnedFd #403

Closed
ids1024 opened this issue Dec 16, 2022 · 10 comments
Closed

Use standard library OwnedFd #403

ids1024 opened this issue Dec 16, 2022 · 10 comments

Comments

@ids1024
Copy link
Contributor

ids1024 commented Dec 16, 2022

Currently dbus defines an OwnedFd type. Since a type with that name and purpose exists in std now, it would be best to use the standard type (the io-lifetimes crate can be used to support the same thing on older compilers, if needed).

This would be a breaking API change since the type is defined differently.

Looking into this, there are a few complications:

  • dbus relies on the type implementing PartialOrd, while the standard OwnedFd doesn't. This could be addressed by a manual implementation of PartialOrd in types that contain a OwnedFd, but that's awkward.
  • dbus also relies on Clone. While the standard library doesn't offer this and provides OwnedFd::try_clone since the operation may fail. Similar considerations for manual implementations.
  • dbus provides a placeholder OwnedFd on non-Unix platforms. Maybe instead the UnixFd enum variant should only exist on Unix? Not sure if there are other complications.

Or without API break, or having to deal with these issue, implementing AsFd and From would offer better interoperability with the standard types, at least.

@diwic
Copy link
Owner

diwic commented Dec 20, 2022

I think a first step could be to implement possible traits for std::os::fd::OwnedFd just like we do for std::fs::File.

Could you clarify what part of dbus relies on the type implementing PartialOrd and Clone? I checked Arg, RefArg, Append and Get and neither of those required PartialOrd and Clone.

@ids1024
Copy link
Contributor Author

ids1024 commented Dec 20, 2022

MessageItem has a UnixFd(OwnedFd) variant, and implements PartialOrd and Clone. Looks like removing PartialOrd from that as well as MessageItemDict and MessageItemArray compiles. But removing Clone leads to errors.

@diwic
Copy link
Owner

diwic commented Dec 21, 2022

Ok, so MessageItem is really legacy and I don't care that much about it. Is MessageItem the primary place you want it in? Arg/Append/Get can take multiple types. RefArg has a single representation w r t what you get out when you read something, but can also be implemented for many types.

@ids1024
Copy link
Contributor Author

ids1024 commented Dec 21, 2022

Ah, I didn't see that was a legacy thing. I was just looking at what code in the repository breaks without the PartialOrd and Clone implementations on OwnedFd.

@ids1024
Copy link
Contributor Author

ids1024 commented Dec 21, 2022

So yeah, if implementing those traits for the standard library type is sufficient to use that with non-legacy APIs, that should be doable enough without any breaking API changes.

@diwic
Copy link
Owner

diwic commented Dec 22, 2022

Hmm, so even if the API changes are not breaking, it does not seem easy to implement this without either bumping rust version or add additional dependencies...right?

@ids1024
Copy link
Contributor Author

ids1024 commented Dec 22, 2022

If it's just a a matter of implementing a few traits, io-lifetimes could be added as an optional dependency.

@ids1024
Copy link
Contributor Author

ids1024 commented Dec 23, 2022

#407 adds such an optional dependency.

@diwic
Copy link
Owner

diwic commented Dec 30, 2022

See also #410

@diwic
Copy link
Owner

diwic commented Jan 6, 2023

Ok, so I decided to remove the io-lifetimes feature and add a stdfd feature instead. I believe this fixes your issue. See #418

@diwic diwic closed this as completed Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants