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.

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

Support cs_disasm_iter #115

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

dnsserver
Copy link

What do you think about this API?

@tmfink
Copy link
Member

tmfink commented Sep 21, 2021

This is a big improvement over the last PR. 😄

Overall, I like the approach of having separate type to "own" the cs_insn allocation. I also like how DisasmIter tracks the "updates" from repeated calls to cs_disasm_iter().

I have some comments that I will make in-line.

capstone-rs/examples/recursive.rs Outdated Show resolved Hide resolved
capstone-rs/examples/recursive.rs Outdated Show resolved Hide resolved
capstone-rs/examples/recursive.rs Show resolved Hide resolved
capstone-rs/examples/recursive.rs Outdated Show resolved Hide resolved
capstone-rs/examples/recursive.rs Outdated Show resolved Hide resolved
capstone-rs/src/capstone.rs Outdated Show resolved Hide resolved
/// Used to start the iterative disassembly
///
/// usage shown in examples/recursive.rs
pub fn disasm_iter(&mut self, code: &[u8], offset: usize, addr: u64) -> CsResult<Insn> {
Copy link
Member

Choose a reason for hiding this comment

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

we don't need to take an offset; the user can just adjust the code slice as they want:

let new_code = &code[3..];

Copy link
Author

Choose a reason for hiding this comment

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

It will get a bit tricky to handle the iteration within the loop without an offset ( cur_insn = disasm.disasm_iter_continue(&buf); ). Would it be too much to ask to keep the offset ?

capstone-rs/src/capstone.rs Outdated Show resolved Hide resolved
@tmfink
Copy link
Member

tmfink commented Sep 22, 2021

I just pushed a fix (#116) for the GitHub action failure; please rebase to the current master branch to get the fix.

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #115 (1e57323) into master (854de72) will not change coverage.
The diff coverage is n/a.

❗ Current head 1e57323 differs from pull request most recent head 06dbfb6. Consider uploading reports for the commit 06dbfb6 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #115   +/-   ##
=======================================
  Coverage   94.90%   94.90%           
=======================================
  Files          22       22           
  Lines        2670     2670           
=======================================
  Hits         2534     2534           
  Misses        136      136           
Impacted Files Coverage Δ
capstone-rs/src/capstone.rs 93.52% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 854de72...06dbfb6. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants