refactor(userspace/engine): apply C++ best practices to newest engine classes

This include making a coherent use of const, remove private inheritance, and adding virtual destructors.

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

tests/engine/test_filter_evttype_resolver.cpp

@@ -38,8 +38,7 @@ string to_string(set<uint16_t> s)
 void compare_evttypes(ast::expr* f, set<uint16_t> &expected)
 {
     set<uint16_t> actual;
-    filter_evttype_resolver resolver;
+    filter_evttype_resolver().evttypes(f, actual);
-    resolver.evttypes(f, actual);
     for(auto &etype : expected)
     {
         REQUIRE(actual.find(etype) != actual.end());
@@ -52,13 +51,11 @@ void compare_evttypes(ast::expr* f, set<uint16_t> &expected)
 
 ast::expr* compile(const string &fltstr)
 {
-    libsinsp::filter::parser p(fltstr);
+    return libsinsp::filter::parser(fltstr).parse();
-    return p.parse();
 }
 
 TEST_CASE("Should find event types from filter", "[rule_loader]")
 {
-    filter_evttype_resolver resolver;
     set<uint16_t> openat_only{
 		PPME_SYSCALL_OPENAT_2_E, PPME_SYSCALL_OPENAT_2_X };
 

userspace/engine/filter_evttype_resolver.cpp

@@ -27,7 +27,7 @@ static bool is_evttype_operator(const string& op)
 	return op == "==" || op == "=" || op == "!=" || op == "in";
 }
 
-void filter_evttype_resolver::inversion(set<uint16_t>& types)
+void filter_evttype_resolver::visitor::inversion(set<uint16_t>& types)
 {
 	set<uint16_t> all_types;
 	evttypes("", all_types);
@@ -41,7 +41,7 @@ void filter_evttype_resolver::inversion(set<uint16_t>& types)
 	}
 }
 
-void filter_evttype_resolver::evttypes(string evtname, set<uint16_t>& out)
+void filter_evttype_resolver::visitor::evttypes(string evtname, set<uint16_t>& out)
 {
 	// Fill in from 2 to PPM_EVENT_MAX-1. 0 and 1 are excluded as
 	// those are PPM_GENERIC_E/PPME_GENERIC_X
@@ -58,26 +58,31 @@ void filter_evttype_resolver::evttypes(string evtname, set<uint16_t>& out)
 	}
 }
 
-void filter_evttype_resolver::evttypes(ast::expr* filter, set<uint16_t>& out)
+void filter_evttype_resolver::evttypes(
+	ast::expr* filter,
+	set<uint16_t>& out) const
 {
-	m_expect_value = false;
+	visitor v;
-	m_last_node_evttypes.clear();
+	v.m_expect_value = false;
-	filter->accept(this);
+	v.m_last_node_evttypes.clear();
-	out.insert(m_last_node_evttypes.begin(), m_last_node_evttypes.end());
+	filter->accept(&v);
+	out.insert(v.m_last_node_evttypes.begin(), v.m_last_node_evttypes.end());
 }
 
 void filter_evttype_resolver::evttypes(
-	shared_ptr<ast::expr> filter, set<uint16_t>& out)
+	shared_ptr<ast::expr> filter,
+	set<uint16_t>& out) const
 {
-	m_expect_value = false;
+	visitor v;
-	m_last_node_evttypes.clear();
+	v.m_expect_value = false;
-	filter.get()->accept(this);
+	v.m_last_node_evttypes.clear();
-	out.insert(m_last_node_evttypes.begin(), m_last_node_evttypes.end());
+	filter.get()->accept(&v);
+	out.insert(v.m_last_node_evttypes.begin(), v.m_last_node_evttypes.end());
 }
 
 // "and" nodes evttypes are the intersection of the evttypes of their children.
 // we initialize the set with "all event types"
-void filter_evttype_resolver::visit(ast::and_expr* e)
+void filter_evttype_resolver::visitor::visit(ast::and_expr* e)
 {
 	set<uint16_t> types, inters;
 	evttypes("", types);
@@ -96,7 +101,7 @@ void filter_evttype_resolver::visit(ast::and_expr* e)
 }
 
 // "or" nodes evttypes are the union of the evttypes their children
-void filter_evttype_resolver::visit(ast::or_expr* e)
+void filter_evttype_resolver::visitor::visit(ast::or_expr* e)
 {
 	set<uint16_t> types;
 	m_last_node_evttypes.clear();
@@ -108,14 +113,14 @@ void filter_evttype_resolver::visit(ast::or_expr* e)
 	m_last_node_evttypes = types;
 }
 
-void filter_evttype_resolver::visit(ast::not_expr* e)
+void filter_evttype_resolver::visitor::visit(ast::not_expr* e)
 {
 	m_last_node_evttypes.clear();
 	e->child->accept(this);
 	inversion(m_last_node_evttypes);
 }
 
-void filter_evttype_resolver::visit(ast::binary_check_expr* e)
+void filter_evttype_resolver::visitor::visit(ast::binary_check_expr* e)
 {
 	m_last_node_evttypes.clear();
 	if (e->field == "evt.type" && is_evttype_operator(e->op))
@@ -132,13 +137,13 @@ void filter_evttype_resolver::visit(ast::binary_check_expr* e)
 	evttypes("", m_last_node_evttypes);
 }
 
-void filter_evttype_resolver::visit(ast::unary_check_expr* e)
+void filter_evttype_resolver::visitor::visit(ast::unary_check_expr* e)
 {
 	m_last_node_evttypes.clear();
 	evttypes("", m_last_node_evttypes);
 }
 
-void filter_evttype_resolver::visit(ast::value_expr* e)
+void filter_evttype_resolver::visitor::visit(ast::value_expr* e)
 {
 	m_last_node_evttypes.clear();
 	if (m_expect_value)
@@ -149,7 +154,7 @@ void filter_evttype_resolver::visit(ast::value_expr* e)
 	evttypes("", m_last_node_evttypes);
 }
 
-void filter_evttype_resolver::visit(ast::list_expr* e)
+void filter_evttype_resolver::visitor::visit(ast::list_expr* e)
 {
 	m_last_node_evttypes.clear();
 	if (m_expect_value)

userspace/engine/filter_evttype_resolver.h

@@ -24,7 +24,7 @@ limitations under the License.
 /*!
 	\brief Helper class for finding event types
 */
-class filter_evttype_resolver: private libsinsp::filter::ast::expr_visitor
+class filter_evttype_resolver
 {
 public:
 	/*!
@@ -35,7 +35,10 @@ public:
 		string is passed, all the available evttypes are collected
 		\param out The set to be filled with the evttypes
 	*/
-	void evttypes(std::string evtname, std::set<uint16_t>& out);
+	inline void evttypes(std::string evtname, std::set<uint16_t>& out) const
+	{
+		visitor().evttypes(evtname, out);
+	}
 
 	/*!
 		\brief Visits a filter AST and collects all the evttypes for which
@@ -45,25 +48,32 @@ public:
 		\param filter The filter AST to be explored
 		\param out The set to be filled with the evttypes
 	*/
-	void evttypes(libsinsp::filter::ast::expr* filter, std::set<uint16_t>& out);
+	void evttypes(
+		libsinsp::filter::ast::expr* filter,
+		std::set<uint16_t>& out) const;
 
 	/*!
 		\brief Overloaded version of evttypes() that supports filters wrapped
 		in shared pointers
 	*/
-	void evttypes(std::shared_ptr<libsinsp::filter::ast::expr> filter,
+	void evttypes(
-		std::set<uint16_t>& out);
+		std::shared_ptr<libsinsp::filter::ast::expr> filter,
+		std::set<uint16_t>& out) const;
 
 private:
-	void visit(libsinsp::filter::ast::and_expr* e) override;
+	struct visitor : public libsinsp::filter::ast::expr_visitor
-	void visit(libsinsp::filter::ast::or_expr* e) override;
+	{
-	void visit(libsinsp::filter::ast::not_expr* e) override;
+		bool m_expect_value;
-	void visit(libsinsp::filter::ast::value_expr* e) override;
+		std::set<uint16_t> m_last_node_evttypes;
-	void visit(libsinsp::filter::ast::list_expr* e) override;
-	void visit(libsinsp::filter::ast::unary_check_expr* e) override;
-	void visit(libsinsp::filter::ast::binary_check_expr* e) override;
-	void inversion(std::set<uint16_t>& types);
 
-	bool m_expect_value;
+		void visit(libsinsp::filter::ast::and_expr* e) override;
-	std::set<uint16_t> m_last_node_evttypes;
+		void visit(libsinsp::filter::ast::or_expr* e) override;
+		void visit(libsinsp::filter::ast::not_expr* e) override;
+		void visit(libsinsp::filter::ast::value_expr* e) override;
+		void visit(libsinsp::filter::ast::list_expr* e) override;
+		void visit(libsinsp::filter::ast::unary_check_expr* e) override;
+		void visit(libsinsp::filter::ast::binary_check_expr* e) override;
+		void inversion(std::set<uint16_t>& types);
+		void evttypes(std::string evtname, std::set<uint16_t>& out);
+	};
 };

userspace/engine/filter_macro_resolver.cpp

@@ -21,29 +21,37 @@ using namespace libsinsp::filter;
 
 bool filter_macro_resolver::run(libsinsp::filter::ast::expr*& filter)
 {
+	visitor v;
 	m_unknown_macros.clear();
 	m_resolved_macros.clear();
-	m_last_node_changed = false;
+	v.m_unknown_macros = &m_unknown_macros;
-	m_last_node = filter;
+	v.m_resolved_macros = &m_resolved_macros;
-	filter->accept(this);
+	v.m_macros = &m_macros;
-	if (m_last_node_changed)
+	v.m_last_node_changed = false;
+	v.m_last_node = filter;
+	filter->accept(&v);
+	if (v.m_last_node_changed)
 	{
 		delete filter;
-		filter = m_last_node;
+		filter = v.m_last_node;
 	}
 	return !m_resolved_macros.empty();
 }
 
 bool filter_macro_resolver::run(std::shared_ptr<libsinsp::filter::ast::expr>& filter)
 {
+	visitor v;
 	m_unknown_macros.clear();
 	m_resolved_macros.clear();
-	m_last_node_changed = false;
+	v.m_unknown_macros = &m_unknown_macros;
-	m_last_node = filter.get();
+	v.m_resolved_macros = &m_resolved_macros;
-	filter->accept(this);
+	v.m_macros = &m_macros;
-	if (m_last_node_changed)
+	v.m_last_node_changed = false;
+	v.m_last_node = filter.get();
+	filter->accept(&v);
+	if (v.m_last_node_changed)
 	{
-		filter.reset(m_last_node);
+		filter.reset(v.m_last_node);
 	}
 	return !m_resolved_macros.empty();
 }
@@ -55,17 +63,17 @@ void filter_macro_resolver::set_macro(
 	m_macros[name] = macro;
 }
 
-set<string>& filter_macro_resolver::get_unknown_macros()
+const set<string>& filter_macro_resolver::get_unknown_macros() const
 {
 	return m_unknown_macros;
 }
 
-set<string>& filter_macro_resolver::get_resolved_macros()
+const set<string>& filter_macro_resolver::get_resolved_macros() const
 {
 	return m_resolved_macros;
 }
 
-void filter_macro_resolver::visit(ast::and_expr* e)
+void filter_macro_resolver::visitor::visit(ast::and_expr* e)
 {
 	for (size_t i = 0; i < e->children.size(); i++)
 	{
@@ -80,7 +88,7 @@ void filter_macro_resolver::visit(ast::and_expr* e)
 	m_last_node_changed = false;
 }
 
-void filter_macro_resolver::visit(ast::or_expr* e)
+void filter_macro_resolver::visitor::visit(ast::or_expr* e)
 {
 	for (size_t i = 0; i < e->children.size(); i++)
 	{
@@ -95,7 +103,7 @@ void filter_macro_resolver::visit(ast::or_expr* e)
 	m_last_node_changed = false;
 }
 
-void filter_macro_resolver::visit(ast::not_expr* e)
+void filter_macro_resolver::visitor::visit(ast::not_expr* e)
 {
 	e->child->accept(this);
 	if (m_last_node_changed)
@@ -107,13 +115,13 @@ void filter_macro_resolver::visit(ast::not_expr* e)
 	m_last_node_changed = false;
 }
 
-void filter_macro_resolver::visit(ast::list_expr* e)
+void filter_macro_resolver::visitor::visit(ast::list_expr* e)
 {
 	m_last_node = e;
 	m_last_node_changed = false;
 }
 
-void filter_macro_resolver::visit(ast::binary_check_expr* e)
+void filter_macro_resolver::visitor::visit(ast::binary_check_expr* e)
 {
 	// avoid exploring checks, so that we can be sure that each
 	// value_expr* node visited is a macro identifier
@@ -121,29 +129,29 @@ void filter_macro_resolver::visit(ast::binary_check_expr* e)
 	m_last_node_changed = false;
 }
 
-void filter_macro_resolver::visit(ast::unary_check_expr* e)
+void filter_macro_resolver::visitor::visit(ast::unary_check_expr* e)
 {
 	m_last_node = e;
 	m_last_node_changed = false;
 }
 
-void filter_macro_resolver::visit(ast::value_expr* e)
+void filter_macro_resolver::visitor::visit(ast::value_expr* e)
 {
 	// we are supposed to get here only in case
 	// of identier-only children from either a 'not',
 	// an 'and' or an 'or'.
-	auto macro = m_macros.find(e->value);
+	auto macro = m_macros->find(e->value);
-	if (macro != m_macros.end() && macro->second) // skip null-ptr macros
+	if (macro != m_macros->end() && macro->second) // skip null-ptr macros
 	{
 		ast::expr* new_node = ast::clone(macro->second.get());
 		new_node->accept(this); // this sets m_last_node
 		m_last_node_changed = true;
-		m_resolved_macros.insert(e->value);
+		m_resolved_macros->insert(e->value);
 	}
 	else
 	{
 		m_last_node = e;
 		m_last_node_changed = false;
-		m_unknown_macros.insert(e->value);
+		m_unknown_macros->insert(e->value);
 	}
 }

userspace/engine/filter_macro_resolver.h

@@ -26,7 +26,7 @@ limitations under the License.
 	\brief Helper class for substituting and resolving macro
 	references in parsed filters.
 */
-class filter_macro_resolver: private libsinsp::filter::ast::expr_visitor
+class filter_macro_resolver
 {
 	public:
 		/*!
@@ -63,7 +63,7 @@ class filter_macro_resolver: private libsinsp::filter::ast::expr_visitor
 			substituted during the last invocation of run(). Should be
 			non-empty if the last invocation of run() returned true.
 		*/
-		std::set<std::string>& get_resolved_macros();
+		const std::set<std::string>& get_resolved_macros() const;
 
 		/*!
 			\brief Returns a set containing the names of all the macros
@@ -71,23 +71,32 @@ class filter_macro_resolver: private libsinsp::filter::ast::expr_visitor
 			A macro remains unresolved if it is found inside the processed
 			filter but it was not defined with set_macro();
 		*/
-		std::set<std::string>& get_unknown_macros();
+		const std::set<std::string>& get_unknown_macros() const;
 		
 	private:
-		void visit(libsinsp::filter::ast::and_expr* e) override;
+		typedef std::map<
-		void visit(libsinsp::filter::ast::or_expr* e) override;
-		void visit(libsinsp::filter::ast::not_expr* e) override;
-		void visit(libsinsp::filter::ast::value_expr* e) override;
-		void visit(libsinsp::filter::ast::list_expr* e) override;
-		void visit(libsinsp::filter::ast::unary_check_expr* e) override;
-		void visit(libsinsp::filter::ast::binary_check_expr* e) override;
-
-		bool m_last_node_changed;
-		libsinsp::filter::ast::expr* m_last_node;
-		std::set<std::string> m_unknown_macros;
-		std::set<std::string> m_resolved_macros;
-		std::map<
 			std::string,
 			std::shared_ptr<libsinsp::filter::ast::expr>
-		> m_macros;
+		> macro_defs;
+
+		struct visitor : public libsinsp::filter::ast::expr_visitor
+		{
+			bool m_last_node_changed;
+			libsinsp::filter::ast::expr* m_last_node;
+			std::set<std::string>* m_unknown_macros;
+			std::set<std::string>* m_resolved_macros;
+			macro_defs* m_macros;
+
+			void visit(libsinsp::filter::ast::and_expr* e) override;
+			void visit(libsinsp::filter::ast::or_expr* e) override;
+			void visit(libsinsp::filter::ast::not_expr* e) override;
+			void visit(libsinsp::filter::ast::value_expr* e) override;
+			void visit(libsinsp::filter::ast::list_expr* e) override;
+			void visit(libsinsp::filter::ast::unary_check_expr* e) override;
+			void visit(libsinsp::filter::ast::binary_check_expr* e) override;
+		};
+
+		std::set<std::string> m_unknown_macros;
+		std::set<std::string> m_resolved_macros;
+		macro_defs m_macros;
 };

userspace/engine/indexed_vector.h

@@ -28,10 +28,12 @@ template <typename T>
 class indexed_vector
 {
 public:
+	virtual ~indexed_vector() = default;
+
 	/*!
 		\brief Returns the number of elements
 	*/
-	virtual inline size_t size()
+	virtual inline size_t size() const
 	{
 		return m_entries.size();
 	}
@@ -39,7 +41,7 @@ public:
 	/*!
 		\brief Returns true if the vector is empty
 	*/
-	virtual inline bool empty()
+	virtual inline bool empty() const
 	{
 		return m_entries.empty();
 	}

userspace/engine/rule_loader.cpp

@@ -724,8 +724,7 @@ void rule_loader::compile_rule_infos(
 			set<uint16_t> evttypes;
 			if(rule.source == falco_common::syscall_source)
 			{
-				filter_evttype_resolver resolver;
+				filter_evttype_resolver().evttypes(ast, evttypes);
-				resolver.evttypes(ast, evttypes);
 				if ((evttypes.empty() || evttypes.size() > 100)
 					&& r.warn_evttypes)
 				{

userspace/engine/rule_loader.h

@@ -67,7 +67,7 @@ public:
 	*/
 	struct configuration
 	{
-		configuration(const std::string& cont): content(cont) {}
+		explicit configuration(const std::string& cont): content(cont) {}
 		const std::string& content;
 		std::string output_extra;
 		bool replace_output_container_info;
@@ -172,6 +172,8 @@ public:
 		bool skip_if_unknown_filter;
 	};
 
+	virtual ~rule_loader() = default;
+
 	/*!
 		\brief Erases all the internal state and definitions
 	*/

userspace/engine/rule_reader.h

@@ -27,6 +27,8 @@ limitations under the License.
 class rule_reader
 {
 public:
+    virtual ~rule_reader() = default;
+
     /*!
 		\brief Reads the contents of a ruleset and uses a loader to store
         thew new definitions