-
Notifications
You must be signed in to change notification settings - Fork 74
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
Typesafe IPC #331
Typesafe IPC #331
Conversation
No. You may break as many things as you want :) |
I noticed couple |
This should conclude first part if IPC refactor. In next PR I will reorganize daemon and client a bit to facilitate easier adjustments. |
Wayland is already being used by some people in FreeBSD. They've even opened issues in this project. Furthermore, in general, Wayland isn't Linux-specific. There was a time where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me so long. This is a large PR and I am kind of busy right now.
In any case, I am really liking how this is turning out. I just have a tiny nitpick regarding how you do imports.
use std::env; | ||
use std::marker::PhantomData; | ||
use std::sync::OnceLock; | ||
use std::time::Duration; | ||
|
||
use rustix::fd::OwnedFd; | ||
use rustix::io::Errno; | ||
use rustix::net; | ||
|
||
use super::ErrnoExt; | ||
use super::IpcError; | ||
use super::IpcErrorKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you are doing imports like this? I don't particularly mind, but it isn't consistent with the rest of the codebase. I supposed we could normalize them later...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply prefer this style. When I change imports, the resulting diff is on one line (with entire path). Also, I think it is easier to parse (entire path in one spot, I don't have to jump around to see who declared something like fs
module) and edit in general (dd
in vim).
Alright. And as for |
They are there to avoid copies. By memory mapping them we have only one shared memory buffer between the client and the daemon. Also, we won't be using serde, at all. Serde does a bunch of unnecessary copies around, and increases our dependencies significantly due to everything we have to pull. It's really good for large applications where writing everything manually would be a pain, or simply when you don't want to bother. But I went to great lengths to make IPC as efficient as I could. Our serialization and de-serialization will remain all manual. I believe the easiest way of making the code more readable would be by using something like a builder pattern. You wrap |
Sorry, that's not really what I meant. By serde I meant reading and writing messages to the shared buffer (without a protocol). But when receiving and deserializing data, I noticed tho two aforementioned types being built. Is there a reason not to create EDIT: |
Ah, I get it now, my bad. I don't 100% remember what the problem was, but I believe it was that, when we send in an animation, we will have the image bytes and the animation bytes all mapped, and I wanted to unmap just the image bytes and keep the animation bytes. In any case, if you think you can simplify the logic by removing them, go ahead and give it a try! |
This is first (and probably not last) PR for IPC refactor. For now this is a draft because I am unsure if PRs of this size are either to big or too inconsequential.
Apart from sending and receiving itself, I will take a look at some semi-duplicated code (looking at you,
RequestSend
andRequestRecv
), though this will not be included in this PR.Is there any form of compatibility guarantee between swww and swww-daemon if version bump ocuurs?