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

fix(userspace/engine): free formatters, if any #1447

Merged
merged 2 commits into from
Oct 27, 2020
Merged

Conversation

leogr
Copy link
Member

@leogr leogr commented Oct 15, 2020

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

Previously, formatters were freed by LUA code when re-opening outputs.
Since now, outputs are not controlling anymore the falco_formats class (see #1412), we just free formatters only if were already initialized.

That is needed when the engine restarts (see #1446).

By doing so, we also ensure that the correct inspector instance is set to the formatter cache.

Which issue(s) this PR fixes:

Fixes #1446

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(userspace/engine): free formatters, if any

Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Please take a look at my comment :)

userspace/engine/formats.cpp Outdated Show resolved Hide resolved
@leogr
Copy link
Member Author

leogr commented Oct 16, 2020

/milestone 0.27.0

@poiana poiana added this to the 0.27.0 milestone Oct 16, 2020
Previously, formatters were freed by LUA code when re-opening outputs.
Since now, outputs are not controlling anymore the falco_formats class (see #1412), we just free formatters only if were already initialized.

That is needed when the engine restarts (see #1446).

By doing so, we also ensure that correct inspector instance is set to the formatter cache.

Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
@poiana poiana added size/S and removed size/XS labels Oct 16, 2020
@leogr leogr changed the title wip: fix(userspace/engine): free formatters, if any fix(userspace/engine): free formatters, if any Oct 16, 2020
@leogr
Copy link
Member Author

leogr commented Oct 16, 2020

Ready for review.

Thanks, @leodido for the suggestions.

Co-Authored-By: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
@poiana
Copy link

poiana commented Oct 19, 2020

LGTM label has been added.

Git tree hash: fa5bb6bede04d5d38bd6cd0dd4d7b23035ec8275

@fntlnz
Copy link
Contributor

fntlnz commented Oct 19, 2020

We can probably use make_shared from cpp+20 here/ I know that we are on C++11 so we might just use the polyfills from abseil cpp that we already have for gRPC .

abseil: https://github.com/abseil/abseil-cpp
grpc importing abseil: https://github.com/falcosecurity/falco/blob/master/cmake/modules/gRPC.cmake#L119

@fntlnz
Copy link
Contributor

fntlnz commented Oct 26, 2020

Putting an hold since this is blocking the merge queue. Feel free to remove once it's approved by two maintainers.

/hold

@leogr
Copy link
Member Author

leogr commented Oct 27, 2020

/cc @leodido

@poiana poiana requested a review from leodido October 27, 2020 10:47
@poiana
Copy link

poiana commented Oct 27, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, leodido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@leogr
Copy link
Member Author

leogr commented Oct 27, 2020

/hold cancel

🥳

@poiana poiana merged commit c8703b8 into master Oct 27, 2020
@poiana poiana deleted the fix/free-formatters branch October 27, 2020 14:12
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.

segmentation fault after SIGHUP
4 participants