-
-
Notifications
You must be signed in to change notification settings - Fork 127
Implement safe io #871
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
Implement safe io #871
Conversation
1200fdd
to
281adac
Compare
Added commit descriptions where there were api questions. |
2b2aeee
to
b8c5eb6
Compare
|
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.
Seems good to me otherwise
16e67be
to
86cfed9
Compare
Rebased. There were quite many conflicts so a review would be appreciated. |
Is this ready for another review and potentially merging? |
Yes and maybe |
glib/src/functions.rs
Outdated
stderr_fd: V, | ||
stdin_fd: Option<impl AsFd>, | ||
stdout_fd: Option<impl AsFd>, | ||
stderr_fd: Option<impl AsFd>, |
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.
Option<impl ...>
is usually a bad idea. Just try calling this function with a None
:)
I think here and in the other APIs using an Option<impl ...>
currently you'll have to add a builder pattern instead
@@ -238,7 +241,7 @@ pub fn compute_checksum_for_string( | |||
|
|||
#[cfg(unix)] | |||
#[doc(alias = "g_unix_open_pipe")] | |||
pub fn unix_open_pipe(flags: i32) -> Result<(RawFd, RawFd), Error> { | |||
pub unsafe fn unix_open_pipe(flags: i32) -> Result<(RawFd, RawFd), Error> { |
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.
Should probably return OwnedFd
?
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 that compatible with the flags
argument that allows setting O_CLOEXEC/FD_CLOEXEC?
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 think so
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.
As a prior art rustix::fd::open
also returns OwnedFd
and allows passing in the CLOEXEC flag.
@@ -132,7 +135,7 @@ pub fn spawn_async_with_fds<P: AsRef<std::path::Path>, T: AsRawFd, U: AsRawFd, V | |||
#[cfg(not(windows))] | |||
#[cfg_attr(docsrs, doc(cfg(not(windows))))] | |||
#[doc(alias = "g_spawn_async_with_pipes")] | |||
pub fn spawn_async_with_pipes< | |||
pub unsafe fn spawn_async_with_pipes< |
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.
Why not make this use the new traits?
@@ -872,14 +872,14 @@ where | |||
/// The default main loop almost always is the main loop of the main thread. | |||
/// Thus, the closure is called on the main thread. | |||
#[doc(alias = "g_unix_fd_add_full")] | |||
pub fn unix_fd_add<F>(fd: RawFd, condition: IOCondition, func: F) -> SourceId |
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 think these functions all need to be unsafe
actually. impl AsFd
is correct but it's up to the caller to ensure that the fd is valid long enough
Perhaps we could split this into smaller chunks. |
Go for it, or maybe @nagisa wants to take this over to bring it over the finish line? |
@A6GibKm would you mind rebasing this one? i would like to switch to 2024 edition before the next breaking release |
How does this relate to 2024 edition? But regardless, it would be nice to finish this :) |
Rebased. |
Sure, go ahead :) |
I am closing in favor of #1745. If someone wants to pick up wip: unix_fd_list: Use custom iterator for steal_fds, feel free to do so. |
Can you create an issue for tracking the missing parts? |
Needs reviewing
Stuff thats missing