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.

Sign up for GitHub

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

Jump to bottom

Implement safe io #871

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Implement safe io #871

wants to merge 11 commits into from

Conversation

A6GibKm
Copy link
Contributor

@A6GibKm A6GibKm commented Dec 28, 2022 •
edited
Loading

Needs reviewing

Stuff thats missing

  • unix_open_pipe (marked as unsafe for the moment)
  • spawn_async_with_pipes (marked as unsafe for the moment)
  • spawn_async_with_fds
  • g_subprocess_launcher_take_fd
  • g_subprocess_launcher_take_stderr_fd
  • g_subprocess_launcher_take_stderr_fd
  • g_subprocess_launcher_take_stdin_fd
  • g_subprocess_launcher_take_stdout_fd
  • into_raw_unix_fd
  • into_raw_unix_fd_local
  • g_unix_fd_add_full

@A6GibKm A6GibKm marked this pull request as draft December 28, 2022 09:44
sdroege
sdroege reviewed Dec 28, 2022
View reviewed changes
gio/Cargo.toml Outdated Show resolved Hide resolved
@A6GibKm A6GibKm force-pushed the safe-io branch 2 times, most recently from 1200fdd to 281adac Compare December 28, 2022 09:56
@A6GibKm
Copy link
Contributor Author

A6GibKm commented Dec 28, 2022

Added commit descriptions where there were api questions.

@A6GibKm A6GibKm force-pushed the safe-io branch 4 times, most recently from 2b2aeee to b8c5eb6 Compare December 28, 2022 10:37
@A6GibKm
Copy link
Contributor Author

A6GibKm commented Dec 28, 2022 •
edited
Loading

stuff that takes impl AsFd should take &impl AsFd. Done.

@sdroege sdroege added this to the 0.17.0 milestone Jan 3, 2023
sdroege
sdroege reviewed Jan 4, 2023
View reviewed changes
gio/src/unix_fd_list.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Jan 4, 2023
View reviewed changes
glib/src/functions.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Jan 4, 2023
View reviewed changes
Copy link
Member

@sdroege sdroege left a 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

@sdroege sdroege modified the milestones: 0.17.0, 0.18.0 Jan 21, 2023
@A6GibKm A6GibKm force-pushed the safe-io branch 2 times, most recently from 16e67be to 86cfed9 Compare January 27, 2023 14:46
@A6GibKm A6GibKm marked this pull request as ready for review July 2, 2023 07:54
@A6GibKm A6GibKm marked this pull request as draft July 2, 2023 08:00
@A6GibKm
Copy link
Contributor Author

A6GibKm commented Jul 2, 2023

Rebased. There were quite many conflicts so a review would be appreciated.

@sdroege
Copy link
Member

sdroege commented Jul 3, 2023

Is this ready for another review and potentially merging?

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Jul 3, 2023

Is this ready for another review and potentially merging?

Yes and maybe

sdroege
sdroege reviewed Aug 7, 2023
View reviewed changes
gio/src/desktop_app_info.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Aug 7, 2023
View reviewed changes
gio/src/unix_fd_list.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Aug 7, 2023
View reviewed changes
gio/src/unix_fd_list.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
gio/src/socket.rs
if error.is_null() {
Ok(from_glib_full(ret))
} else {
Err(from_glib_full(error))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On success, the returned GSocket takes ownership of fd. On failure, the caller must close fd themselves.

So you need to close() the fd here

@sdroege sdroege marked this pull request as ready for review June 8, 2024 08:44
sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
gio/src/subprocess_launcher.rs Outdated
@@ -1,7 +1,7 @@
// Take a look at the license at the top of the repository in the LICENSE file.

#[cfg(any(unix, all(docsrs, unix)))]
use std::os::unix::io::IntoRawFd;
use std::os::unix::io::{IntoRawFd, OwnedFd};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below: "unused import: ffi" on Windows so maybe just move that part into a #[cfg(unix)] while we're at it

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
gio/src/unix_fd_list.rs Outdated
type IntoIter = FdIterator;

fn into_iter(mut self) -> Self::IntoIter {
// Can I .take() the ptr and set it to null here?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but but you should set the len in the FdArray to 0 so you don't drop the fds twice

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
gio/src/unix_fd_list.rs
}

impl FdArray {
pub fn as_slice(&self) -> &[BorrowedFd<'_>] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you didn't implement Deref<Target = [OwnedFd]>, it would be nice if you implemented a non-destructive iter() too. Personally, I would just implement Deref :)

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
gio/src/unix_fd_list.rs
let current = self.ptr.as_ptr();
if self.len > 1 {
let next = unsafe { self.ptr.as_ptr().add(1) };
self.ptr = ptr::NonNull::new(next).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work. You'll have to keep track of the original pointer so it can be freed on Drop (of the FdIterator)

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
gio/src/unix_fd_list.rs
}
if self.len > 1 {
let next = unsafe { self.ptr.as_ptr().add(1) };
self.ptr = ptr::NonNull::new(next).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work, you'll also have to free the original pointer

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
gio/src/unix_fd_list.rs
len: usize,
}

impl Iterator for FdIterator {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to also implement size_hint() and DoubleEndedIterator and ExactSizeIterator and FusedIterator here

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
glib/src/functions.rs
stderr_fd: V,
stdin_fd: Option<impl AsFd>,
stdout_fd: Option<impl AsFd>,
stderr_fd: Option<impl AsFd>,
Copy link
Member

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

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
glib/src/functions.rs
@@ -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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably return OwnedFd?

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
glib/src/functions.rs
@@ -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<
Copy link
Member

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?

sdroege
sdroege reviewed Jun 8, 2024
View reviewed changes
glib/src/source.rs
@@ -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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdroege sdroege sdroege left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.20
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@A6GibKm @sdroege @bilelmoussaoui