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

crit: add features to retrieve memory pages #133

Merged
merged 2 commits into from
Jun 16, 2023

Conversation

behouba
Copy link
Contributor

@behouba behouba commented Jun 2, 2023

The goal of this PR is to add features for collecting memory pages content.
For the now I have added GetMemPages function to retrieve the memory pages of a process. Alongside it, helper functions genMemChunk and getPage have been implemented. The core logic is derived from https://github.com/checkpoint-restore/criu/blob/criu-dev/coredump/criu_coredump/coredump.py.

It's still a work in progress, but I would like to submit it for feedback.

PTAL: @adrianreber, @rst0git

Thank you!

@behouba behouba marked this pull request as draft June 2, 2023 16:00
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch coverage: 78.57% and project coverage change: +1.91 🎉

Comparison is base (748b90b) 48.33% compared to head (4c337d0) 50.25%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
+ Coverage   48.33%   50.25%   +1.91%     
==========================================
  Files          23       24       +1     
  Lines        2435     2589     +154     
==========================================
+ Hits         1177     1301     +124     
- Misses       1084     1104      +20     
- Partials      174      184      +10     
Impacted Files Coverage Δ
test/crit/main.go 52.20% <33.33%> (-9.34%) ⬇️
crit/mempages.go 97.24% <97.24%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adrianreber
Copy link
Member

Thanks for the PR. I am a bit unsure about the design. Returning the content of the pages as a return parameter does not sound right. For large memory areas I would expect that using pointers would be better and would hopefully require less data movement.

Nice to see tests included in the PR. You could try to also pass a couple of wrong parameters to test the error paths as well.

@behouba
Copy link
Contributor Author

behouba commented Jun 5, 2023

I have made a few changes to the previous implementation:

  • Added a MemoryReader struct to encapsulate common and reusable data.
  • Modified the GetMemPages function to accept addresses (start and end) for flexibility, enabling the retrieval of specific portions of memory (e.g., mm_arg, mm_env, pagemap addresses etc.).
  • The GetMemPages function now returns a *bytes.Buffer instead of []byte.

Do you think this is a good direction for ?

crit/mempages.go Outdated Show resolved Hide resolved
crit/mempages_test.go Outdated Show resolved Hide resolved
@behouba behouba force-pushed the read-memory-pages branch 4 times, most recently from 8bef749 to 555be57 Compare June 10, 2023 11:07
@behouba behouba marked this pull request as ready for review June 10, 2023 11:08
crit/mempages.go Outdated Show resolved Hide resolved
crit/mempages.go Outdated Show resolved Hide resolved
crit/mempages.go Outdated Show resolved Hide resolved
Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

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

LGTM

This commit adds the MemoryReader struct with methods to retrieve
the memory pages associated with a process. This feature enhances
the functionality of crit as a package by providing access processes
memory pages content.

Signed-off-by: Kouame Behouba Manasse <behouba@gmail.com>
@behouba behouba changed the title crit: add functions to retrieve memory page crit: add features to retrieve memory pages Jun 15, 2023
This commit introduces a new function `readMemoryPages` that reads
the process arguments and environment variables from memory pages
and compares them with the `environ` and `cmdline` files of the process.

Signed-off-by: Kouame Behouba Manasse <behouba@gmail.com>
@adrianreber
Copy link
Member

Looks very good. Thanks.

@adrianreber adrianreber merged commit 4c639ba into checkpoint-restore:master Jun 16, 2023
14 checks passed
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

4 participants