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

Define process.parent via the new field reuse mechanism #868

Merged
merged 2 commits into from
Jun 15, 2020

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Jun 11, 2020

The process.parent fields had initially been defined by duplicating each field explicitly, since the field reuse mechanism didn't support field reuse within the same field set at the time.

Introducing process.parent.* by duplicating fields has predictably led to a few small mistakes, despite us squinting very hard and trying to avoid this :-)

  • At least one process field was modified without having the same modification applied to its process.parent twin.
  • Also, the field set pe was only reused under process, but not process.parent. This was not intentional.

This PR removes the field duplication by using the improved reuse mechanism. This means both problems described above are also resolved by this PR.

@webmat webmat self-assigned this Jun 11, 2020
@webmat
Copy link
Contributor Author

webmat commented Jun 11, 2020

@rw-access When introducing PE, we had discussed that it belonged both under process and process.parent. But we forgot to do so explicitly, at the time. This PR fixes this. You're still good with this?

@rw-access
Copy link
Contributor

Awesome. Before I ✔️, this adds code_signature to the parent too, right?

@webmat
Copy link
Contributor Author

webmat commented Jun 11, 2020

@rw-access Yes, both code_signature and hash were correctly reused under process.parent. We did so explicitly by adding an additional reuse entry (see this example). The issue is that we had forgotten to do that for pe.

With the revamped reuse mechanism, we no longer use to define reuse at process.parent explicitly. When we reuse something else under process, it will be added to both process and process.parent automatically. So this PR actually removes these reuse entries towards the parent.

Copy link
Contributor

@rw-access rw-access left a comment

Choose a reason for hiding this comment

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

🚢

@rw-access
Copy link
Contributor

rw-access commented Jun 11, 2020

Also, I noticed this typo for process.args

      description: >
        Array of process arguments, starting with the absolute path to the executable.

For the first arg, I think it should be passed-through as is, so I think we can remove that note so that it's just this:

      description: >
        Array of process arguments.

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@webmat webmat merged commit 6de399f into elastic:master Jun 15, 2020
@smerzlyakov
Copy link

Looks not so good to use process.parent.* bacause of it do not fir to all scheme. We do not have parent base field.
Why not to use parent.process.* ? Looks better, because of all sub-fileds are in process core field.

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.

4 participants