💥 zv: Simplify ser/de API that ensures FD ownership is handled correctly #497
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We can hopefully soon ditch our own `OwnedFd` type and have our `Fd` by a wrapper around `std::os::fd::OwnedFd` but until then, we'll need this to allow for a step-by-step transition to std's type.
We'll be making use of this in the following commits to clean up the zvariant API.
Instead return * the new `Encoded` type (which contains FDs on unix platforms) from to_bytes*. * the FDs vector from to_writer*. This also means that we switch from RawFd to OwnedFd type. Caveat: We have to dup the file descriptors but since Serialize::serialize takes a reference to self, there is no way to get ownership right w/o dup. However, dup is cheap so it's not a big deal IMO. Having said that, I do have an idea on how to avoid the dup, but mostly likely it will require additiion of a lifetime parameter to the `Message` type.
This makes for a much better API for deserialization than top-level functions, which we can ditch in a following commit.
A new type the contains the body bytes and signature. We'll use this in a following commit to be returned from `Message::body`.
Deref impls were a misuse anyway (dbus2#475) and we're about to introduce changes that will require us to keep another type inside these structs, instead of Message. Typically users will not need access to underlying message anyway but if they do, they can/should use the lower-level SignalStream API. Moreover, we also add a `From<YourSignalStruct> for Arc<Message>` implementation for some edge cases.
This also drops all body-related API from Message.
zeenix
force-pushed
the
simplify-zv-api
branch
from
e5729e0 to
6203d54
Compare
Use `serialized::Data` API instead. These top-level functions shall be dropped in a following commit.
Users now use `serialized::Data` API instead.
Provide access to the underlying serialized::Data.
This is no longer needed.
These were never used by us. The high-level ser functions that users use are independent of these types.
Nobody needs to use them directly and I seriously doubt anyone does.
Make use of generics and `AsFd` trait to avoid unnecessary allocation of FD list.
Seems one of our indirect dependencies, toml_datatime have bumped to this and since Rust 1.67 was released more than 8 months ago, it's not that big a deal.
zeenix
force-pushed
the
simplify-zv-api
branch
from
6203d54 to
fe81eb0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We mainly introduce new data types that hold serialized data along with associated file descriptors. This ensures that the FDs live as long as the serialized data that refers to them.
Caveats:
dupfor every FD that's serialized.zvariant::to_write*doesn't ensure FD lifetimes are associated with the writer passed. If we can't solve this, these methods should be marked asunsafeIMO.I've an idea on how to solve both of these but that will be a separate PR as this branch is already big enough.
These changes also lay the ground work for solving #286 in a nice way.