Rename the public ptrace_* functions. #692

marmistrz · 2017-07-20T08:33:13Z

Unlike in C, we have namespacing in Rust. Renaming the functions allows us to avoid a use nix::sys::ptrace::* in favor of use nix::sys::ptrace and then calling, for example, ptrace::traceme()

I'm wondering if we should rename the private functions too...

Unlike in C, we have namespacing in Rust. Renaming the functions allows us to avoid a `use nix::sys::ptrace::*` in favor of `use nix::sys::ptrace` and then calling, for example, `ptrace::traceme()`

Susurrus · 2017-07-20T13:49:04Z

If you think the private functions could have better names, I'm all for it. You'll also need to re-name the tests (if you run cargo test before pushing you'll generally save yourself some time with this PRs).

Also, could you add this usage to the module documentation at the top? It'd be nice if we explicitly told people that's how they should do this versus just letting them figure it out. In theory they should know cause it's how a lot of Rust works, but it might be worth mentioning. What do you think? Too basic?

marmistrz · 2017-07-20T14:01:04Z

I think I'll add the documentation in #666

Susurrus · 2017-07-20T14:19:02Z

test/sys/test_ptrace.rs

@@ -1,6 +1,7 @@
 use nix::Error;
 use nix::errno::*;
 use nix::unistd::*;
+use nix::sys::ptrace;
 use nix::sys::ptrace::*;


This change doesn't seem so useful if you still need to glob import a ton of things. Can we get rid of those here or does that make things really cluttered?

marmistrz · 2017-07-20T14:24:56Z

I can't find a better name for the private functions and I'd leave them as they are. A function called other looks weird.

Susurrus · 2017-07-20T15:45:59Z

test/sys/test_ptrace.rs

    assert!(err == Error::Sys(Errno::EPERM) || err == Error::Sys(Errno::ENOSYS));
 }

 // Just make sure ptrace_setoptions can be called at all, for now.
 #[test]
 fn test_ptrace_setoptions() {
+    use nix::sys::ptrace::ptrace::*; // for PTRACE_O_TRACESYSGOOD


Why aren't you just importing the single constant here then? No need to bulk import. Same for PTRACE_ATTACH above.

Man, we really need to get PtraceRequest, PtraceEvent, and PtraceOptions into their own enums/bitflags. Having a ptrace module within a ptrace module is just horrible ergonomics.

Oh, I probably was too tired back then ;) Sorry, I'll fix that tomorrow.

And I totally agree about the enums

marmistrz · 2017-07-24T07:03:35Z

Anything else needed to merge this?

Susurrus · 2017-07-24T16:41:00Z

Yes, this needs a CHANGELOG entry (you should wait for #706 to land and rebase on that).

This refactors the examples to more directly address the exact use cases for the `ioctl!` macro that `nix` provides. Additionally other macros that should not be used by end users are no longer discussed.

These are low-level functions that shouldn't be exposed

This also means that we need to properly mask off bits to the valid range of ioctl numbers.

These were exported for some weird reason and then left in for documentation. Also some parts of certain modules used them and others used the libc:: prefix. This was removed to improve the docs and also code consistency

There two different write semantics used with ioctls: one involves passing a pointer the other involves passing an int. Previously the ioctl! macro did not distinguish between these cases and left it up to the user to set the proper datatype. This previous version was not type safe and prone to errors. The solution here is to split the "write" variant into a "write_ptr" and "write_int" variant that makes the semantics more explicit and improves type safety by specifying arguments better.

Instead of relying on the macro user to calculate the length in bytes do that within the macro itself

ptsname(3) returns a pointer to a global variable, so it isn't thread-safe. Protect it with a mutex.

On semi-recent Rust versions (I think 1.8+) this now works properly.

It's just a (redundant) link to the repo, not a real homepage. According to the [API guidelines](https://github.com/brson/rust-api-guidelines#cargotoml-includes-all-common-metadata-c-metadata), this shouldn't be set in this case.

The recommended way to trace syscalls with ptrace is to set the PTRACE_O_TRACESYSGOOD option, to distinguish syscall stops from receiving an actual SIGTRAP. In C, this would cause WSTOPSIG to return SIGTRAP | 0x80, but nix wants to parse that as an actual signal. Add another wait status type for syscall stops (in the language of the ptrace(2) manpage, "PTRACE_EVENT stops" and "Syscall-stops" are different things), and mask out bit 0x80 from signals before trying to parse it. Closes nix-rust#550

Some tests were invoking non-async-signal-safe functions from the child process after a `fork`. Since they might be invoked in parallel, this could lead to problems.

Note that ptrace isn't documented as signal-safe, but it's supposed to just be a light syscall wrapper, so it should be fine.

When tests are run in QEMU the job just times out

These are assumed to be QEMU issues, as they also fail on mips.

marmistrz · 2017-07-25T07:14:09Z

Is it expected that rebasing on current master generated a big list of commits mentioned in the PR? (see the whole list after your comment)

…rename

Susurrus · 2017-07-25T15:38:11Z

test/sys/test_wait.rs

@@ -51,7 +51,8 @@ mod ptrace {
    use libc::_exit;

    fn ptrace_child() -> ! {
-        let _ = ptrace(PTRACE_TRACEME, Pid::from_raw(0), ptr::null_mut(), ptr::null_mut());
+        // TODO ptrace::traceme will be added in #666


There's no need for these comments.

Susurrus · 2017-07-25T15:38:21Z

test/sys/test_wait.rs

        assert_eq!(waitpid(child, None), Ok(WaitStatus::PtraceSyscall(child)));
        // Then get the ptrace event for the process exiting
-        assert!(ptrace(PTRACE_CONT, child, ptr::null_mut(), ptr::null_mut()).is_ok());
+        // TODO ptrace::cont will be added in #666


Susurrus · 2017-07-25T15:38:25Z

test/sys/test_wait.rs


        // First, stop on the next system call, which will be exit()
-        assert!(ptrace(PTRACE_SYSCALL, child, ptr::null_mut(), ptr::null_mut()).is_ok());
+        // TODO ptrace::syscall will be added in #666


Susurrus · 2017-07-25T15:40:23Z

src/sys/ptrace.rs

@@ -108,7 +108,7 @@ fn ptrace_other(request: ptrace::PtraceRequest, pid: Pid, addr: *mut c_void, dat
 }

 /// Set options, as with `ptrace(PTRACE_SETOPTIONS,...)`.
-pub fn ptrace_setoptions(pid: Pid, options: ptrace::PtraceOptions) -> Result<()> {
+pub fn setoptions(pid: Pid, options: ptrace::PtraceOptions) -> Result<()> {


There's no module-level documentation for the ptrace module, but it'd be good to add one showing an example usage that demonstrates the namespacing we recomment. We don't need it per-say, but it'd help users use this crate very easily. It'll also help us see where the imports aren't very ergonomic so we can improve them later.

I'll add the documentation when I'm done with all the ptrace changes, ok?

Besides, I really think about splitting up #666 into a couple smaller PRs because I'm starting to lose control of the tests.

marmistrz · 2017-07-25T15:54:52Z

Removed the comments.

Susurrus · 2017-07-25T16:28:45Z

bors r+

692: Rename the public ptrace_* functions. r=Susurrus Unlike in C, we have namespacing in Rust. Renaming the functions allows us to avoid a `use nix::sys::ptrace::*` in favor of `use nix::sys::ptrace` and then calling, for example, `ptrace::traceme()` I'm wondering if we should rename the private functions too...

bors · 2017-07-25T18:35:43Z

Build succeeded

Rename the public ptrace_* functions.

b4f1749

Unlike in C, we have namespacing in Rust. Renaming the functions allows us to avoid a `use nix::sys::ptrace::*` in favor of `use nix::sys::ptrace` and then calling, for example, `ptrace::traceme()`

Fix tests

3192df4

Susurrus reviewed Jul 20, 2017

View reviewed changes

Tidy up imports in test_ptrace.rs

d5c4d0f

Susurrus reviewed Jul 20, 2017

View reviewed changes

Get rid of the bulk imports

18b2bc1

marmistrz force-pushed the ptrace-rename branch from 02d443d to 18b2bc1 Compare July 21, 2017 08:06

Susurrus and others added 18 commits July 25, 2017 09:09

Remove old workaround

8928a7d

Re-add bad variant of ioctl!

1b9a779

Add a "bad none" variant to the ioctl macro

a5b01c0

Hide internal macros/types within ioctl

2fe5c2b

Remove unnecessary path aliasing

233a678

Revise ioctl module documentation

55a7d4b

This refactors the examples to more directly address the exact use cases for the `ioctl!` macro that `nix` provides. Additionally other macros that should not be used by end users are no longer discussed.

Remove ioc_* functions

caaffb8

These are low-level functions that shouldn't be exposed

Use the proper ioctl number type depending on target

82e0139

This also means that we need to properly mask off bits to the valid range of ioctl numbers.

Remove c_int and c_void from root

e4a1851

These were exported for some weird reason and then left in for documentation. Also some parts of certain modules used them and others used the libc:: prefix. This was removed to improve the docs and also code consistency

Remove unnecessary constants

88dc19b

Unify argument names to generated ioctl functions

7b07847

Add 'bad' prefixes for read, write_*, and readwrite ioctls

571386b

Refactor ioctl! for buffers

d63c616

Instead of relying on the macro user to calculate the length in bytes do that within the macro itself

Update changelog

93b2929

Fix thread safety issues in pty and termios tests

085f47c

ptsname(3) returns a pointer to a global variable, so it isn't thread-safe. Protect it with a mutex.

fix some tests for Android

1c9e0ca

Remove workaround for pub extern crate

2a5b86d

On semi-recent Rust versions (I think 1.8+) this now works properly.

jonas-schievink and others added 18 commits July 25, 2017 09:09

Remove homepage from Cargo.toml

b6ad298

It's just a (redundant) link to the repo, not a real homepage. According to the [API guidelines](https://github.com/brson/rust-api-guidelines#cargotoml-includes-all-common-metadata-c-metadata), this shouldn't be set in this case.

Add tests of actual ioctl usage

845453b

Document WaitStatus and its variants

2288202

use std::env::temp_dir() to retrieve the temp directory in test_mkstemp

907bb98

Allow doc attributes in ioctl macro

a162ea2

Document invariants of fork and fix tests

745a4ab

Some tests were invoking non-async-signal-safe functions from the child process after a `fork`. Since they might be invoked in parallel, this could lead to problems.

Remove unneeded local

4cefd53

Replace remaining process::exit with libc::_exit

536787e

Note that ptrace isn't documented as signal-safe, but it's supposed to just be a light syscall wrapper, so it should be fine.

Add CHANGELOG entry for PR nix-rust#661

283fb1c

Release 0.9.0

f3167db

Bump version to 0.10.0-pre

021e851

Move Tier 3s to Tier 2

8beaea2

Disable tests on s390x

46191fd

When tests are run in QEMU the job just times out

Disable failing tests on mips64

a717003

These are assumed to be QEMU issues, as they also fail on mips.

Disable testing because of musl #include issues

ce60aa5

Add a changelog entry

1844378

Merge branch 'master' into ptrace-rename

6583fd1

marmistrz added 2 commits July 25, 2017 09:19

Fix the tests compiling

9cf8248

Merge branch 'ptrace-rename' of github.com:marmistrz/nix into ptrace-…

6ce39a6

…rename

Susurrus suggested changes Jul 25, 2017

View reviewed changes

Remove the TODO comments, add unwrap() for ptrace.

bf93180

Susurrus approved these changes Jul 25, 2017

View reviewed changes

bors bot merged commit bf93180 into nix-rust:master Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename the public ptrace_* functions. #692

Rename the public ptrace_* functions. #692

marmistrz commented Jul 20, 2017

Susurrus commented Jul 20, 2017

marmistrz commented Jul 20, 2017

Susurrus Jul 20, 2017

marmistrz commented Jul 20, 2017

Susurrus Jul 20, 2017

marmistrz Jul 20, 2017 •

edited

Loading

marmistrz Jul 21, 2017

marmistrz commented Jul 24, 2017

Susurrus commented Jul 24, 2017

marmistrz commented Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus Jul 25, 2017

marmistrz Jul 25, 2017

marmistrz commented Jul 25, 2017

Susurrus commented Jul 25, 2017

bors bot commented Jul 25, 2017

Rename the public ptrace_* functions. #692

Rename the public ptrace_* functions. #692

Conversation

marmistrz commented Jul 20, 2017

Susurrus commented Jul 20, 2017

marmistrz commented Jul 20, 2017

Choose a reason for hiding this comment

marmistrz commented Jul 20, 2017

Choose a reason for hiding this comment

marmistrz Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmistrz commented Jul 24, 2017

Susurrus commented Jul 24, 2017

marmistrz commented Jul 25, 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

marmistrz commented Jul 25, 2017

Susurrus commented Jul 25, 2017

bors bot commented Jul 25, 2017

Build succeeded

marmistrz Jul 20, 2017 •

edited

Loading