NetBSD: add sysctl backend for std::env::current_exe #46373

jakllsch · 2017-11-29T17:47:13Z

Use the CTL_KERN.KERN_PROC_ARGS.-1.KERN_PROC_PATHNAME sysctl in
preference over the /proc/curproc/exe symlink.

Additionally, perform more validation of aformentioned symlink.
Particularly on pre-8.x NetBSD this symlink will point to '/' when
accurate information is unavailable.

rust-highfive · 2017-11-29T17:47:18Z

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

kennytm · 2017-11-29T17:51:14Z

src/libstd/sys/unix/os.rs

+        }
+        Err(io::Error::new(io::ErrorKind::Other, "/proc/curproc/exe doesn't point to regular file."))
+    }
+    sysctl().or(procfs())


Use sysctl().or_else(procfs) here, otherwise procfs() will always be called.

kennytm · 2017-11-29T17:52:09Z

src/libstd/sys/unix/os.rs

+        }
+    }
+    fn procfs() -> io::Result<PathBuf> {
+        let curproc_exe = PathBuf::from("/proc/curproc/exe");


Why a PathBuf instead of Path?

For no reason other than to match the linux,android,emscripten current_exe(). I had path::Path::new() in a previous version of this. I'll restore it.

kennytm · 2017-11-29T17:57:13Z

src/libstd/sys/unix/os.rs

+            cvt(libc::sysctl(mib.as_ptr(), mib.len() as ::libc::c_uint,
+                             path.as_ptr() as *mut libc::c_void, &mut path_len,
+                             ptr::null(), 0))?;
+            path.set_len(path_len - 1); // chop off NUL


Do you need to check path_len == 0 just like the FreeBSD implementation above? Or is it possible to merge the implementations of the BSDs (only the mib differ as far as I can see).

Possibly could even check for path_len <= 1 for additional safety. There are a few very subtle differences between the FreeBSD & DragonFly block and what I implemented, but I imagine they could be merged if thought best. While they are nearly identical, I have this purist view that as there is no shared lineage in the kernel-side implementation of these sysctls, they may as well stay seperate here.

kennytm · 2017-11-29T18:03:42Z

src/libstd/sys/unix/os.rs

+        if curproc_exe.is_file() {
+            return ::fs::read_link(curproc_exe);
+        }
+        Err(io::Error::new(io::ErrorKind::Other, "/proc/curproc/exe doesn't point to regular file."))


This line is too long, causing CI to fail :(

[00:04:06] tidy error: /checkout/src/libstd/sys/unix/os.rs:246: line longer than 100 chars

I'll wrap that line.

jakllsch · 2017-11-29T22:06:34Z

CI might have missed this:

error[E0277]: the trait bound core::result::Result<path::PathBuf, io::error::Error>: core::ops::FnOnce<(io::error::Error,)> is not satisfied
--> /local/jakllsch/rust/rust/src/libstd/sys/unix/os.rs:253:14
|
253 | sysctl().or_else(procfs())
| ^^^^^^^ the trait core::ops::FnOnce<(io::error::Error,)> is not implemented for core::result::Result<path::PathBuf, io::error::Error>

kennytm · 2017-11-29T22:24:15Z

The CI for PR only runs on Linux, so it will definitely miss any change made to NetBSD.

kennytm · 2017-11-30T15:07:08Z

src/libstd/sys/unix/os.rs

+        Err(io::Error::new(io::ErrorKind::Other,
+                           "/proc/curproc/exe doesn't point to regular file."))
+    }
+    sysctl().or_else(procfs())


sysctl().or_else(procfs) without the last ().

I had tried that and got:

error[E0593]: function is expected to take 1 argument, but it takes 0 arguments --> /local/jakllsch/rust/rust/src/libstd/sys/unix/os.rs:253:14 | 253 | sysctl().or_else(procfs) | ^^^^^^^ expected function that takes 1 argument error: aborting due to previous error

I'd really like to keep the prototypes/signatures of sysctl() and procfs() identical..

@jakllsch Oh ok. Call through a closure sysctl().or_else(|_| procfs()) then.

kennytm · 2017-11-30T18:03:01Z

@jakllsch Are we supposed to wait for you to squash the commits or can these be merged as-is?

Use the CTL_KERN.KERN_PROC_ARGS.-1.KERN_PROC_PATHNAME sysctl in preference over the /proc/curproc/exe symlink. Additionally, perform more validation of aformentioned symlink. Particularly on pre-8.x NetBSD this symlink will point to '/' when accurate information is unavailable.

jakllsch · 2017-11-30T21:32:50Z

@kennytm I've squashed it.

kennytm · 2017-11-30T21:42:44Z

Thanks for the contribution!

@bors r+

bors · 2017-11-30T21:42:45Z

📌 Commit ccef969 has been approved by kennytm

… r=kennytm NetBSD: add sysctl backend for std::env::current_exe Use the CTL_KERN.KERN_PROC_ARGS.-1.KERN_PROC_PATHNAME sysctl in preference over the /proc/curproc/exe symlink. Additionally, perform more validation of aformentioned symlink. Particularly on pre-8.x NetBSD this symlink will point to '/' when accurate information is unavailable.

Rollup of 13 pull requests - Successful merges: #45880, #46280, #46373, #46376, #46385, #46386, #46387, #46392, #46400, #46401, #46405, #46412, #46421 - Failed merges:

rust-highfive assigned kennytm Nov 29, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2017

kennytm reviewed Nov 29, 2017

View reviewed changes

kennytm requested changes Nov 30, 2017

View reviewed changes

kennytm approved these changes Nov 30, 2017

View reviewed changes

jakllsch force-pushed the netbsd-kern_proc_pathname branch from 8de0cde to ccef969 Compare November 30, 2017 18:18

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2017

kennytm mentioned this pull request Dec 1, 2017

Rollup of 13 pull requests #46430

Merged

bors added a commit that referenced this pull request Dec 1, 2017

Auto merge of #46430 - kennytm:rollup, r=kennytm

7051754

Rollup of 13 pull requests - Successful merges: #45880, #46280, #46373, #46376, #46385, #46386, #46387, #46392, #46400, #46401, #46405, #46412, #46421 - Failed merges:

bors merged commit ccef969 into rust-lang:master Dec 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetBSD: add sysctl backend for std::env::current_exe #46373

NetBSD: add sysctl backend for std::env::current_exe #46373

jakllsch commented Nov 29, 2017

rust-highfive commented Nov 29, 2017

kennytm Nov 29, 2017

jakllsch Nov 29, 2017

kennytm Nov 29, 2017

jakllsch Nov 29, 2017

kennytm Nov 29, 2017

jakllsch Nov 29, 2017

kennytm Nov 29, 2017

jakllsch Nov 29, 2017

jakllsch commented Nov 29, 2017

kennytm commented Nov 29, 2017

kennytm Nov 30, 2017

jakllsch Nov 30, 2017

kennytm Nov 30, 2017

kennytm commented Nov 30, 2017

jakllsch commented Nov 30, 2017

kennytm commented Nov 30, 2017

bors commented Nov 30, 2017

NetBSD: add sysctl backend for std::env::current_exe #46373

NetBSD: add sysctl backend for std::env::current_exe #46373

Conversation

jakllsch commented Nov 29, 2017

rust-highfive commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakllsch commented Nov 29, 2017

kennytm commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm commented Nov 30, 2017

jakllsch commented Nov 30, 2017

kennytm commented Nov 30, 2017

bors commented Nov 30, 2017