-
-
Notifications
You must be signed in to change notification settings - Fork 31
Regen with nullable callbacks #230
Regen with nullable callbacks #230
Conversation
The ones with the fds are unsafe like this. Will review in detail tomorrow
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
src/auto/desktop_app_info.rs
Outdated
pid_callback: Option<&mut dyn (FnMut(&DesktopAppInfo, glib::Pid))>, | ||
stdin_fd: i32, | ||
stdout_fd: i32, | ||
stderr_fd: i32, |
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.
This should be generic over FromRawFd
for the fds on UNIX and FromRawHandle
on Windows. Like we do elsewhere.
Passing arbitrary numbers around as fds is unsafe.
Looks good otherwise |
b77d12b
to
7aa83c7
Compare
7aa83c7
to
1e10059
Compare
1e10059
to
2b16006
Compare
uris: &[&str], | ||
launch_context: Option<&P>, | ||
spawn_flags: glib::SpawnFlags, | ||
user_setup: Option<Box_<dyn FnOnce() + 'static>>, |
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.
Not sure this is correct or safe. The function is called once per process, so if you have multiple URIs... :) OTOH this is called right after fork()
from what I understand. Unclear if FnOnce
is correct in this case.
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.
What do you think?
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.
Also unclear if Rust is supposed to be fork-safe, or if the closure needs to be marked unsafe anyway
src/desktop_app_info.rs
Outdated
pid_callback: Option<&mut dyn (FnMut(&DesktopAppInfo, glib::Pid))>, | ||
stdin_fd: FD, | ||
stdout_fd: FD, | ||
stderr_fd: FD, |
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.
This should become a generic function with FD: IntoRawFd
, or be marked as unsafe
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 IntoRawFd
and not AsRawFd
?
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.
You give ownership of the fds to the function or not?
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 wouldn't, that's why I pcked As*
instead of Into*
.
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.
It doesn't matter what you would do :P What does the C function do?
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.
It appears that the FDs need to be closed outside of the function: https://github.com/GNOME/gnome-shell/blob/master/src/shell-app.c#L1322
Therefore, we don't give ownership.
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.
Then you're right :)
a714d79
to
6ec1700
Compare
6ec1700
to
ef1e34f
Compare
This is obsolete now? |
I guess? |
Wait actually it wasn't: it added new objects. Well, let's first merge your regen PR, it'll make less noise. |
Requires gtk-rs/gir#815.