Replace some direct uses of libc with wrappers #10090
Conversation
|
Lovely. This tries to build fspacectl on FreeBSD, which fails for reasons not entirely clear to me. I've found nix-rust/nix#2122, which seems related, but isn't in a nix release yet. So we would have to wait for a release or adopt our own wrappers. To be honest this decreases my trust in nix. |
|
Alright, added our own wrappers. I do not believe that fiddling around with getting this nix feature to build on FreeBSD is in any way worth it given the wrappers are a single line each, and we get to avoid the newtypes which in our use would just get in the way. This doesn't mean we couldn't switch to nix for other things and then also switch this. |
faho
changed the title
This removes some spurious unsafe blocks and makes usage a bit nicer
fish-rust/src/builtins/test.rs
Outdated
| @@ -84,8 +84,7 @@ mod test_expressions { | |||
| // Return true if the number is a tty(). | |||
| fn isatty(&self, streams: &mut IoStreams) -> bool { | |||
| fn istty(fd: libc::c_int) -> bool { | |||
| // Safety: isatty cannot crash. | |||
| unsafe { libc::isatty(fd) > 0 } | |||
| crate::nix::isatty(fd) | |||
There was a problem hiding this comment.
I've been using the libc:: qualifier for C functions, to give a hint to folks who don't know the function.
But no qualifier for constants like SIGHLD because those are usually used in a libc:: function call already.
Not sure what's the future. Here I think it's fine to use unqualified isatty(fd) always
fish-rust/src/builtins/test.rs
Outdated
| @@ -84,8 +84,7 @@ mod test_expressions { | |||
| // Return true if the number is a tty(). | |||
| fn isatty(&self, streams: &mut IoStreams) -> bool { | |||
| fn istty(fd: libc::c_int) -> bool { | |||
| // Safety: isatty cannot crash. | |||
| unsafe { libc::isatty(fd) > 0 } | |||
| crate::nix::isatty(fd) | |||
There was a problem hiding this comment.
this function can be inlined now
Description
EDIT: Switched to our own wrappers, nix has problems compiling on FreeBSD with the "fs" feature.
This replaces our uses of libc's geteuid, getegid, getpid and isatty with the corresponding functions from the nix crate.
The main advantage is that it removes a bunch of awkward
unsafeblocks. In turn nix has made some type choices like wrap pid_t in a Pid newtype that we then need to cast into an i32 again so we can use it.This isn't the only solution. Alternatively we could:
Fundamentally, I would like for
unsafeto be a rare thing that's easy to audit. In this case libc gets in the way because it defaults to marking things as unsafe, which includes things like getpid where I do not understand what the unsafety is supposed to be.So I would like to wrap these things away, and these four functions were an easy start.
There are some other rust "style" things that I would like to change, including our uses of .unwrap(), which scares me (it's effectively an "assert"!), and I would like to remove a bunch of them.