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

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Implement safe io #871

wants to merge 10 commits into from

Conversation

A6GibKm
Copy link
Contributor

@A6GibKm A6GibKm commented Dec 28, 2022 •
edited

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

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
unsafe {
let mut length = mem::MaybeUninit::uninit();
let ret = FromGlibContainer::from_glib_full_num(
let ret: Vec<RawFd> = FromGlibContainer::from_glib_full_num(
Copy link
Member

Choose a reason for hiding this comment

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

At least this one here should be a glib::Slice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but the return type is still Vec<BorrowedFd<'_>>, which is kinda odd.

Copy link
Member

Choose a reason for hiding this comment

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

It says steal here so they're probably not borrowed but owned?

Also why don't you use glib::Slice here to avoid the copy to a Vec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to do the map RawFd->BorrowedFd without ownership issues at the time.

Copy link
Member

Choose a reason for hiding this comment

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

OwnedFd has the same layout as an actual fd so you could just cast things.

I think the correct thing to do here (because glib::Slice requires all the traits for the contained type) is to define a new struct FdArray(...) that stores a glib::Slice<i32> and can Deref into BorrowedFd and IntoIterator<Item = OwnedFd>, and on Drop frees things accordingly.

@bilelmoussaoui bilelmoussaoui modified the milestones: 0.18.0, 0.19.0 Mar 11, 2024
@bilelmoussaoui bilelmoussaoui modified the milestones: 0.19.0, 0.20 May 31, 2024
@A6GibKm
Copy link
Contributor Author

A6GibKm commented May 31, 2024

rebased. I don't quite remember the state of this. It should be fine, iirc.

sdroege
sdroege reviewed Jun 2, 2024
View reviewed changes
gio/src/unix_fd_list.rs Outdated Show resolved Hide resolved
@A6GibKm A6GibKm force-pushed the safe-io branch 6 times, most recently from 5135cd7 to 4b9a92b Compare June 2, 2024 10:07
@A6GibKm
Copy link
Contributor Author

A6GibKm commented Jun 2, 2024

I removed the extra functions, since yeah in all cases you can unsafely use them with BorrowedFd::borrow_raw and OwnedFd::form_raw_fd.

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