Improve class initialization #3074

sgaist · 2024-02-10T22:25:04Z

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

/kind release

Any specific area of the project related to this PR?

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

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

This PR refactors member variables initialization so that they are done in the constructor initialization list.

They were detected by cppcheck.

Which issue(s) this PR fixes:

Fixes #3073

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

github-actions · 2024-02-10T22:25:27Z

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

incertum

/approve

Yes this is also in line with some feedback @federico-sysdig gave in some libs PRs.

poiana · 2024-02-11T01:33:20Z

LGTM label has been added.

Git tree hash: bba4873503e26fad06b18b25ae3b2f332aa9f89e

incertum · 2024-02-11T01:33:46Z

/unhold

federico-sysdig

I really like these kinds of cleanups. I left a few comments on how it can be done to further simplify the implementation.

userspace/engine/falco_common.h

federico-sysdig · 2024-02-11T08:17:08Z

userspace/falco/versions_info.cpp

+    : falco_version(FALCO_VERSION)
+    , engine_version(FALCO_ENGINE_VERSION)
+    , libs_version(FALCOSECURITY_LIBS_VERSION)
+    , plugin_api_version(inspector->get_plugin_api_version())
+    , driver_api_version(get_driver_api_version(inspector))
+    , driver_schema_version(get_driver_schema_version(inspector))
+    , default_driver_version(DRIVER_VERSION)
+
 {
-    falco_version = FALCO_VERSION;
-    engine_version = FALCO_ENGINE_VERSION;
-    libs_version = FALCOSECURITY_LIBS_VERSION;
-    plugin_api_version = inspector->get_plugin_api_version();
-    driver_api_version = get_driver_api_version(inspector);
-    driver_schema_version = get_driver_schema_version(inspector);
-    default_driver_version = DRIVER_VERSION;


Here 4 data members are initialized to a constant, thus they could/should have been initialized right at the declaration site (falco_version, engine_version, libs_version, and default_driver_version).
Note that the 4 SMF's, copy/move constructors + copy/move assignments, are generated by the compiler and an explicit default definition can/should be avoided.

struct versions_info { /** * @brief Construct a versions info by using an inspector to obtain * versions about the drivers and the loaded plugins. */ explicit versions_info(const std::shared_ptr<sinsp>&); /** * @brief Encode the versions info as a JSON object */ nlohmann::json as_json() const; std::string falco_version = FALCO_VERSION; std::string engine_version = FALCO_ENGINE_VERSION; std::string libs_version = FALCOSECURITY_LIBS_VERSION; std::string plugin_api_version; std::string driver_api_version; std::string driver_schema_version; std::string default_driver_version = DRIVER_VERSION; std::unordered_map<std::string, std::string> plugin_versions; };

Note:
This is not related to this PR, but passing objects by shared_ptr when only the object is used, as we do for inspector in the constructor of versions_info, is overkill and should be avoided by passing a reference or a raw pointer. In this case the need comes from the two functions get_driver_api_version and get_driver_schema_version which take inspector as a smart pointer, again, without any real need to do so. Again, overkill for readability and, sometimes, performance.

Good point, would this something to be done in a follow up PR ?

Good point, would this something to be done in a follow up PR ?

The note on the changing the parameter shared_ptr<sinsp> to a reference or a pointer? Yes, I'd say it can be addressed later.

federico-sysdig · 2024-02-11T08:53:25Z

userspace/falco/stats_writer.cpp

-	: m_initialized(false), m_total_samples(0)
+	: m_initialized(false)
+	, m_total_samples(0)
+	, m_config(config)


Also here, m_initialized and m_total_samples can be more simply initialized at the declaration site.

Should we have a PR that does this on a more global scale ?

Perhaps, but you can get warmed up here, if you care.

FedeDP · 2024-02-12T13:54:15Z

/milestone 0.38.0

FedeDP

/approve

poiana · 2024-02-12T14:00:26Z

LGTM label has been added.

Git tree hash: 11f862a17bdc9e18fd9eb9c62a37538613199ccf

…n list Reported by cppcheck Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch> # Conflicts: # userspace/engine/falco_common.h

The implementation provides more or less the same implementation and thus it makes more sense to base it on std::runtime_error. Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>

Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>

FedeDP

/approve

poiana · 2024-02-12T16:08:39Z

LGTM label has been added.

Git tree hash: 9df54a0b87e02fa96709e32dd3608a7d3f3525d0

incertum

/approve

poiana · 2024-02-12T17:45:17Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,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 release-note-none kind/cleanup dco-signoff: yes area/engine labels Feb 10, 2024

poiana requested review from jasondellaluce and Kaizhe February 10, 2024 22:25

poiana added the size/M label Feb 10, 2024

poiana added the do-not-merge/hold label Feb 10, 2024

incertum previously approved these changes Feb 11, 2024

View reviewed changes

poiana assigned incertum Feb 11, 2024

poiana added the lgtm label Feb 11, 2024

poiana added the approved label Feb 11, 2024

poiana removed the do-not-merge/hold label Feb 11, 2024

federico-sysdig reviewed Feb 11, 2024

View reviewed changes

sgaist dismissed incertum’s stale review via 99b7518 February 11, 2024 21:30

poiana added dco-signoff: no and removed lgtm labels Feb 11, 2024

poiana requested a review from incertum February 11, 2024 21:30

poiana removed the dco-signoff: yes label Feb 11, 2024

poiana added this to the 0.38.0 milestone Feb 12, 2024

sgaist force-pushed the 3073_improve_initialization branch from 99b6571 to 3a021d1 Compare February 12, 2024 13:57

poiana added dco-signoff: yes and removed dco-signoff: no labels Feb 12, 2024

FedeDP previously approved these changes Feb 12, 2024

View reviewed changes

poiana assigned FedeDP Feb 12, 2024

poiana added the lgtm label Feb 12, 2024

sgaist added 4 commits February 12, 2024 17:03

fix(c++): refactor member initialization in constructor initializatio…

9c5568d

…n list Reported by cppcheck Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch> # Conflicts: # userspace/engine/falco_common.h

refactor: make falco_exception a std::runtime_error

0f074b8

The implementation provides more or less the same implementation and thus it makes more sense to base it on std::runtime_error. Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>

fix(c++): move trivial initializations to declaration site

4eba3a4

Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>

fix(c++): add missing member initialisation to grpc server

2b76feb

Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>

sgaist dismissed FedeDP’s stale review via 2b76feb February 12, 2024 16:05

sgaist force-pushed the 3073_improve_initialization branch from 3a021d1 to 2b76feb Compare February 12, 2024 16:05

poiana removed the lgtm label Feb 12, 2024

poiana requested a review from FedeDP February 12, 2024 16:05

FedeDP approved these changes Feb 12, 2024

View reviewed changes

poiana added the lgtm label Feb 12, 2024

incertum approved these changes Feb 12, 2024

View reviewed changes

poiana merged commit 8c98ca5 into falcosecurity:master Feb 12, 2024
29 checks passed

sgaist deleted the 3073_improve_initialization branch February 12, 2024 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve class initialization #3074

Improve class initialization #3074

sgaist commented Feb 10, 2024

github-actions bot commented Feb 10, 2024

incertum left a comment

poiana commented Feb 11, 2024

incertum commented Feb 11, 2024

federico-sysdig left a comment

federico-sysdig Feb 11, 2024 •

edited

sgaist Feb 11, 2024

federico-sysdig Feb 11, 2024

federico-sysdig Feb 11, 2024

sgaist Feb 11, 2024

federico-sysdig Feb 11, 2024

FedeDP commented Feb 12, 2024

FedeDP left a comment

poiana commented Feb 12, 2024

FedeDP left a comment

poiana commented Feb 12, 2024

incertum left a comment

poiana commented Feb 12, 2024

Improve class initialization #3074

Improve class initialization #3074

Conversation

sgaist commented Feb 10, 2024

github-actions bot commented Feb 10, 2024

incertum left a comment

Choose a reason for hiding this comment

poiana commented Feb 11, 2024

incertum commented Feb 11, 2024

federico-sysdig left a comment

Choose a reason for hiding this comment

federico-sysdig Feb 11, 2024 • edited

Choose a reason for hiding this comment

sgaist Feb 11, 2024

Choose a reason for hiding this comment

federico-sysdig Feb 11, 2024

Choose a reason for hiding this comment

federico-sysdig Feb 11, 2024

Choose a reason for hiding this comment

sgaist Feb 11, 2024

Choose a reason for hiding this comment

federico-sysdig Feb 11, 2024

Choose a reason for hiding this comment

FedeDP commented Feb 12, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Feb 12, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Feb 12, 2024

incertum left a comment

Choose a reason for hiding this comment

poiana commented Feb 12, 2024

federico-sysdig Feb 11, 2024 •

edited