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

Added AddressType type to ptrace::linux + peek/poke user fix #958

Merged
merged 2 commits into from
Oct 30, 2018
Merged

Added AddressType type to ptrace::linux + peek/poke user fix #958

merged 2 commits into from
Oct 30, 2018

Conversation

xd009642
Copy link
Contributor

This was added to the BSD ptrace API and probably should have been added to the linux API to make it easier to write code for both platforms without the user having to reexport the types to their own crate.

I went to use the latest nix myself and found that having this added would improve usability for nix users.

Could potentially add a type for what they return (BSDs: c_int, linux: c_long). Data input might be trickier as linux is *mut c_void and BSD just takes data as c_int.

@xd009642 xd009642 changed the title Added AddressType type to ptrace::linux Added AddressType type to ptrace::linux + peek/poke user fix Oct 22, 2018
@xd009642
Copy link
Contributor Author

I also realised I accidentally conflated (PEEK|POKE)_USER with the peeks and pokes for text and data which are equivalent, whereas user refers to the user struct. This is technically a regression although the functionality is deprecated so don't know if you guys want it reverted or not?

@xd009642
Copy link
Contributor Author

@asomers do you have time to have a look at this?

@asomers
Copy link
Member

asomers commented Oct 24, 2018

So the PTRACE_PEEKUSER mistake crept in in PR #949 ? Then you should fix it. It isn't really a regression from the users' point of view, because there hasn't been a release since then. I think this PR is fine, but you do need to update the existing CHANGELOG entry for 949 with the additional PR link.

@xd009642
Copy link
Contributor Author

Yeah there's no tests for it so I didn't realise I'd made a mistake. Unfortunately it's pretty hard to test as it relies on knowledge of the user struct and what's happening in the test executable - unless you can think of a better way?

@asomers
Copy link
Member

asomers commented Oct 24, 2018

No. I'm not even clear on what PTRACE_PEEK_USER does. Is it redundant with PTRACE_GETREGS?

@xd009642
Copy link
Contributor Author

I think it might be, PTRACE_PEEK_USER reads a single word from a given offset into the user struct, from looking at the man page it seems PTRACE_GETREGS reads the entire user struct not just one word.

@xd009642
Added AddressType type
c285b95 
This was added to the BSD ptrace API and probably should have been added to tyhe linux API to make it easier to write code for both platforms without the user having to reexport the types to their own crate.
asomers
asomers requested changes Oct 24, 2018
View reviewed changes
CHANGELOG.md Outdated
@@ -30,6 +30,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
([#949](https://github.com/nix-rust/nix/pull/949))
- Added a `acct` wrapper module for enabling and disabling process accounting
([#952](https://github.com/nix-rust/nix/pull/952))
([#949](https://github.com/nix-rust/nix/pull/949)) ([#958](https://github.com/nix-rust/nix/pull/958))
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you got a merge error when you rebased.

asomers
asomers reviewed Oct 26, 2018
View reviewed changes
CHANGELOG.md Outdated
- Added a `acct` wrapper module for enabling and disabling process accounting
([#952](https://github.com/nix-rust/nix/pull/952))
([#949](https://github.com/nix-rust/nix/pull/949))
Copy link
Member

Choose a reason for hiding this comment

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

CHANGELOG still isn't right.

@xd009642
Copy link
Contributor Author

Fixed (I hope)

asomers
asomers approved these changes Oct 30, 2018
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Oct 30, 2018
@bors @xd009642
Merge #958
bd05d72 
958: Added AddressType type to ptrace::linux + peek/poke user fix r=asomers a=xd009642

This was added to the BSD ptrace API and probably should have been added to the linux API to make it easier to write code for both platforms without the user having to reexport the types to their own crate.

I went to use the latest nix myself and found that having this added would improve usability for nix users.

Could potentially add a type for what they return (BSDs: `c_int`, linux: `c_long`). Data input might be trickier as linux is `*mut c_void` and BSD just takes data as `c_int`.

Co-authored-by: xd009642 <danielmckenna93@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 30, 2018

Build succeeded

@bors bors bot merged commit 12578de into nix-rust:master Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xd009642 @asomers