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

Feature/virtual mem flags parse #3606

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlonZivony
Copy link
Contributor

1. Explain what the PR does

feat(events): parse access_remote_vm args

Parse vm and gup flags arguments of access_remote_vm.

2. Explain how to test it

3. Other comments

An event for accessing the memroy of a process externally (can be the
same process) by the mem file of the process in procfs.

Co-authored-by: OriGlassman <[email protected]>
@AlonZivony
Copy link
Contributor Author

@rafaeldtinoco can you tell me your opinion on the last commit?

Parse vm and gup flags arguments of access_remote_vm.
@AlonZivony AlonZivony force-pushed the feature/virtual-mem-flags-parse branch from a6be9ac to deb2f1d Compare October 24, 2023 15:51
@AlonZivony AlonZivony marked this pull request as ready for review November 13, 2023 14:09
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

The first commit is being already rebased/reviewed in another PR. The last commit is about the flags parsing and I think we should do the version distincting inside the helper package with a generic function that opts for one or another (or something similar).

@AlonZivony
Copy link
Contributor Author

The first commit is being already rebased/reviewed in another PR. The last commit is about the flags parsing and I think we should do the version distincting inside the helper package with a generic function that opts for one or another (or something similar).

The problem with this approach is that you don't know how to user want to use the API.
If this library will be used on the flag from a different machine than the one it was traced in, then the function will return the wrong flag.
So I think the current API is good, but we can add a function of ParseGUPFlagsCurrentOS with the logic I have here.

@rafaeldtinoco
Copy link
Contributor

If this library will be used on the flag from a different machine than the one it was traced in, then the function will return the wrong flag.

You're probably thinking about analyze mode. In that case, we're also missing OSinfo information from the running environment, no ? Thinking about it, like said previously, I believe we should add metadata to the output file (that would include information about running environment). And more on that, if that is true than the function from the helper should accept "current running version" argument and decide by itself.

Either way, I think the decision to parse legacy or not should be delegated to the helper itself. ParseCurrent or ParseSpecific(version) or anything like it, up to you.

@geyslan
Copy link
Member

geyslan commented Feb 21, 2024

We have this update #3875

Please rebase your PR against main to make use of the new workflow setup.

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

Successfully merging this pull request may close these issues.

3 participants