refactor(libsinsp): use smart pointer for m_resolver in sinsp_dns_manager #1558

incertum · 2023-12-07T00:02:22Z

What type of PR is this?

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

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

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

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

Follow up #1526 (comment).
It seems that we need to keep using a pointer, but a smart pointer now as you can't otherwise truly check if std::thread is empty.

CC @FedeDP @federico-sysdig @LucaGuerra

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

incertum · 2023-12-07T01:16:36Z

CI Build / build-shared-libs-linux-amd64 🧐 (pull_request)

appears to have some tbb linkage issues.

federico-sysdig

It seems that we need to keep using a pointer, but a smart pointer now as you can't otherwise truly check if std::thread is empty.

I thought so, after a quick read of the code, that's too bad.
I'm sorry to learn about the tbb linkage issues, I can't understand how they could be related to this change, which LGTM.

federico-sysdig · 2023-12-07T08:17:47Z

userspace/libsinsp/test/dns_manager.ut.cpp

+#include <gtest/gtest.h>
+#include "sinsp_with_test_input.h"
+#include <dns_manager.h>


The rules for ordering the header files vary a lot from organization to organization. However, there is some consensus in the industry that the headers "closest" to the current compilation unit should go first, then more distant ones such as 3rd party libs and, finally, standard header files. The main reason is that including a system/library header file before the local ones might hide the fact that they miss a critical header for their internal definition, only to be sadly discovered by someone, at some later point in time, who uses these headers in a different context.
According to this idea GTest header should go last in this case.
It's your call whether you feel to embrace this.
As an aside, I think the ordering of headers should be put in the project's coding guidelines.

yes we need the coding guidelines as ... well style is all over the place.

incertum · 2023-12-08T07:49:37Z

No idea about the CI, anyone having a clue why CI Build / build-shared-libs-linux-amd64 🧐 (pull_request) is failing?

Edit: That pipeline seems to be wonky across many PRs.

…ager Signed-off-by: Melissa Kilby <[email protected]>

Signed-off-by: Melissa Kilby <[email protected]>

Co-authored-by: Federico Di Pierro <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>

FedeDP

/approve

poiana · 2023-12-12T08:02:51Z

LGTM label has been added.

Git tree hash: 83baf17911bcd54fa34534fae83d2312d3a76e23

gnosek

👍 some very minor questions

gnosek · 2023-12-12T08:05:28Z

userspace/libsinsp/test/dns_manager.ut.cpp

+*/
+
+#if defined(HAS_CAPTURE) && !defined(CYGWING_AGENT) && !defined(_WIN32) && !defined(__EMSCRIPTEN__)
+#include "sinsp_with_test_input.h"


Do we need this include?

the test is not using it, so TEST_F(sinsp_with_test_input below is not really necessary.

When we remove this include I think we are ready to go, WDYT?

✔️ from me

gnosek · 2023-12-12T08:06:22Z

userspace/libsinsp/test/dns_manager.ut.cpp

+TEST_F(sinsp_with_test_input, simple_dns_manager_invocation) 
+{
+    // Simple dummy test to assert that sinsp_dns_manager is invocated correctly
+    // and not leaking memory


Do we run the test suite under valgrind in CI? Do we actually detect the (lack of) memory leak?

We run ASan in all libs tests, which will detect memory leaks.

gnosek · 2023-12-12T08:06:35Z

userspace/libsinsp/dns_manager.cpp

@@ -195,7 +195,7 @@ void sinsp_dns_manager::cleanup()
 	{
 		m_exit_signal.set_value();
 		m_resolver->join();
-		m_resolver = NULL;
+		m_resolver.reset();


Ouch. Good catch!

gnosek · 2023-12-12T08:11:42Z

It seems that we need to keep using a pointer, but a smart pointer now as you can't otherwise truly check if std::thread is empty.

I thought so, after a quick read of the code, that's too bad.

std::optional might be an option (hee hee), not sure if it's worth the hassle though.

Signed-off-by: Melissa Kilby <[email protected]>

FedeDP

/approve

poiana · 2023-12-12T16:18:56Z

LGTM label has been added.

Git tree hash: 2e477a958ad4add52209294e5110b773e5260e3f

incertum · 2023-12-12T16:20:33Z

It seems that we need to keep using a pointer, but a smart pointer now as you can't otherwise truly check if std::thread is empty.

I thought so, after a quick read of the code, that's too bad.

std::optional might be an option (hee hee), not sure if it's worth the hassle though.

@gnosek Oh I would prefer not to mess too much with the dns manager as I never looked at it really 🤣
It likely should be refactored one of these days.

LucaGuerra

LGTM

poiana · 2023-12-12T16:56:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, LucaGuerra

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:

~~OWNERS~~ [FedeDP,LucaGuerra,incertum]

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

poiana added kind/bug Something isn't working release-note-none dco-signoff: yes kind/cleanup area/libsinsp size/M labels Dec 7, 2023

poiana requested review from gnosek and hbrueckner December 7, 2023 00:02

poiana added the approved label Dec 7, 2023

federico-sysdig reviewed Dec 7, 2023

View reviewed changes

incertum force-pushed the cleanup-dns-manager branch 4 times, most recently from 05fc130 to 0cb9311 Compare December 8, 2023 07:39

Andreagit97 added this to the 0.14.0 milestone Dec 11, 2023

incertum added 2 commits December 11, 2023 15:35

refactor(libsinsp): use smart pointer for m_resolver in sinsp_dns_man…

bbaed25

…ager Signed-off-by: Melissa Kilby <[email protected]>

new(libsinsp/test): simple test for sinsp_dns_manager

9c63937

Signed-off-by: Melissa Kilby <[email protected]>

incertum force-pushed the cleanup-dns-manager branch from 0cb9311 to 9c63937 Compare December 11, 2023 15:36

fix(cmake): add tbb to unit-test-libsinsp

542e518

Co-authored-by: Federico Di Pierro <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>

FedeDP previously approved these changes Dec 12, 2023

View reviewed changes

poiana assigned FedeDP Dec 12, 2023

poiana added the lgtm label Dec 12, 2023

gnosek reviewed Dec 12, 2023

View reviewed changes

cleanup(libsinsp/test): adjust sinsp_dns_manager test

836e96a

Signed-off-by: Melissa Kilby <[email protected]>

incertum dismissed FedeDP’s stale review via 836e96a December 12, 2023 16:18

poiana removed the lgtm label Dec 12, 2023

poiana requested a review from FedeDP December 12, 2023 16:18

FedeDP approved these changes Dec 12, 2023

View reviewed changes

poiana added the lgtm label Dec 12, 2023

LucaGuerra approved these changes Dec 12, 2023

View reviewed changes

poiana assigned LucaGuerra Dec 12, 2023

poiana merged commit 1d4cabc into falcosecurity:master Dec 12, 2023
21 checks passed

incertum deleted the cleanup-dns-manager branch December 12, 2023 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(libsinsp): use smart pointer for m_resolver in sinsp_dns_manager #1558

refactor(libsinsp): use smart pointer for m_resolver in sinsp_dns_manager #1558

incertum commented Dec 7, 2023

incertum commented Dec 7, 2023

federico-sysdig left a comment

federico-sysdig Dec 7, 2023

incertum Dec 8, 2023

incertum commented Dec 8, 2023 •

edited

Loading

FedeDP left a comment

poiana commented Dec 12, 2023

gnosek left a comment

gnosek Dec 12, 2023

LucaGuerra Dec 12, 2023

Andreagit97 Dec 12, 2023

gnosek Dec 12, 2023

gnosek Dec 12, 2023

LucaGuerra Dec 12, 2023

gnosek Dec 12, 2023

gnosek commented Dec 12, 2023

FedeDP left a comment

poiana commented Dec 12, 2023

incertum commented Dec 12, 2023

LucaGuerra left a comment

poiana commented Dec 12, 2023

refactor(libsinsp): use smart pointer for m_resolver in sinsp_dns_manager #1558

refactor(libsinsp): use smart pointer for m_resolver in sinsp_dns_manager #1558

Conversation

incertum commented Dec 7, 2023

incertum commented Dec 7, 2023

federico-sysdig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum commented Dec 8, 2023 • edited Loading

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Dec 12, 2023

gnosek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnosek commented Dec 12, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Dec 12, 2023

incertum commented Dec 12, 2023

LucaGuerra left a comment

Choose a reason for hiding this comment

poiana commented Dec 12, 2023

incertum commented Dec 8, 2023 •

edited

Loading