update: address some review comments

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

test/drivers/README.md

@@ -26,15 +26,15 @@ Now all the engines should be built, but if you want to assert against the kmod
 make driver bpf
 ```
 
-> __NOTE__: the modern bpf probe is bundled inside its engine so every time you type `make drivers_test` it will automatically compiled without any additional command.
+> __NOTE__: the modern bpf probe is bundled inside its engine so every time you type `make drivers_test` it will be automatically compiled without any additional command.
 
 We are ready to run our tests:
 
 ```
 sudo ./test/drivers/drivers_test -k
 ```
 
-The `-k` option stands for kmod, so you are running all the tests against the kmod. Available options are:
+The `-k` option stands for kmod, so you are running all the tests against the kmod. Some other available options are:
 
 - `-k` to run tests against the kernel module.
 - `-m` to run tests against the modern bpf probe.

test/drivers/event_class/event_class.cpp

@@ -96,7 +96,7 @@ event_test::~event_test()
 	clear_ring_buffers();
 
 	/* 2 - clean all interesting syscalls. */
-	scap_clear_ppm_sc_mask(scap_handle);
+	scap_clear_ppm_sc_mask(s_scap_handle);
 }
 
 /* This constructor must be used with generic tracepoints
@@ -167,7 +167,7 @@ event_test::event_test(int syscall_id, int event_direction):
 	m_current_param = 0;
 
 	/* Set the current as the only interesting syscall. */
-	scap_set_ppm_sc(scap_handle, g_syscall_table[syscall_id].ppm_sc, true);
+	scap_set_ppm_sc(s_scap_handle, g_syscall_table[syscall_id].ppm_sc, true);
 }
 
 /* This constructor must be used with syscalls events when you
@@ -185,18 +185,18 @@ event_test::event_test():
 	/* Enable all the syscalls */
 	for(int ppm_sc = 0; ppm_sc < PPM_SC_MAX; ppm_sc++)
 	{
-		scap_set_ppm_sc(scap_handle, ppm_sc, true);
+		scap_set_ppm_sc(s_scap_handle, ppm_sc, true);
 	}
 }
 
 void event_test::enable_capture()
 {
-	/* Here I should enable the necessary tracepoints */
+	/* Here we should enable the necessary tracepoints */
 	for(int i = 0; i < TP_VAL_MAX; i++)
 	{
 		if(m_tp_set[i])
 		{
-			scap_set_tpmask(scap_handle, i, true);
+			scap_set_tpmask(s_scap_handle, i, true);
 		}
 	}
 	/* We need to clear all the `ring-buffers` because maybe during
@@ -207,7 +207,7 @@ void event_test::enable_capture()
 
 void event_test::disable_capture()
 {
-	scap_stop_capture(scap_handle);
+	scap_stop_capture(s_scap_handle);
 }
 
 void event_test::clear_ring_buffers()
@@ -216,7 +216,7 @@ void event_test::clear_ring_buffers()
 	/* First timeout means that all the buffers are empty. If the capture is not
 	 * stopped it is possible that we will never receive a `SCAP_TIMEOUT`.
 	 */
-	while(scap_next(scap_handle, (scap_evt**)&m_event_header, &cpu_id) != SCAP_TIMEOUT)
+	while(scap_next(s_scap_handle, (scap_evt**)&m_event_header, &cpu_id) != SCAP_TIMEOUT)
 	{
 	}
 }
@@ -231,7 +231,7 @@ void event_test::get_event_from_ringbuffer(uint16_t* cpu_id)
 	/* Try 2 times just to be sure that all the buffers are empty. */
 	while(attempts <= 1)
 	{
-		res = scap_next(scap_handle, (scap_evt**)&m_event_header, cpu_id);
+		res = scap_next(s_scap_handle, (scap_evt**)&m_event_header, cpu_id);
 		if(res == SCAP_SUCCESS && m_event_header != NULL)
 		{
 			return;
@@ -242,7 +242,6 @@ void event_test::get_event_from_ringbuffer(uint16_t* cpu_id)
 		}
 		attempts++;
 	}
-	return;
 }
 
 void event_test::parse_event()

test/drivers/event_class/event_class.h

@@ -84,11 +84,11 @@ void assert_syscall_state(int syscall_state, const char* syscall_name, long sysc
 class event_test
 {
 public:
-	static scap_t* scap_handle;
+	static scap_t* s_scap_handle;
 
 	static void set_scap_handle(scap_t* handle)
 	{
-		scap_handle = handle;
+		s_scap_handle = handle;
 	}
 
 	/* Please note: only methods with `assert` in the name use Google assertions. */
@@ -147,7 +147,7 @@ class event_test
 	void clear_ring_buffers();
 
 	/**
-	 * @brief Return the event with the lowest timestamp in the ring buffer.
+	 * @brief Retrieve the event with the lowest timestamp in the ring buffer.
 	 * Return the CPU from which we extracted the event. Return NULL
 	 * in case of no events.
 	 *
@@ -171,7 +171,7 @@ class event_test
 	 */
 	bool is_bpf_engine()
 	{
-		return scap_check_current_engine(scap_handle, BPF_ENGINE);
+		return scap_check_current_engine(s_scap_handle, BPF_ENGINE);
 	}
 
 	/**
@@ -181,7 +181,7 @@ class event_test
 	 */
 	bool is_modern_bpf_engine()
 	{
-		return scap_check_current_engine(scap_handle, MODERN_BPF_ENGINE);
+		return scap_check_current_engine(s_scap_handle, MODERN_BPF_ENGINE);
 	}
 
 	/**
@@ -191,7 +191,7 @@ class event_test
 	 */
 	bool is_kmod_engine()
 	{
-		return scap_check_current_engine(scap_handle, KMOD_ENGINE);
+		return scap_check_current_engine(s_scap_handle, KMOD_ENGINE);
 	}
 
 	/////////////////////////////////

test/drivers/start_tests.cpp

@@ -8,6 +8,7 @@
 #define UNKNOWN_ENGINE "unknown"
 
 /* We support only these arguments */
+#define HELP_OPTION "help"
 #define KMOD_OPTION "kmod"
 #define BPF_OPTION "bpf"
 #define MODERN_BPF_OPTION "modern-bpf"
@@ -16,7 +17,7 @@
 #define KMOD_DEFAULT_PATH "/driver/scap.ko"
 #define KMOD_NAME "scap"
 
-scap_t* event_test::scap_handle = NULL;
+scap_t* event_test::s_scap_handle = NULL;
 
 int remove_kmod()
 {
@@ -75,13 +76,49 @@ int insert_kmod(const std::string& kmod_path)
 	return EXIT_SUCCESS;
 }
 
+void abort_if_already_configured(scap_open_args* oargs)
+{
+	if(strcmp(oargs->engine_name, UNKNOWN_ENGINE) != 0)
+	{
+		std::cerr << "* '" << oargs->engine_name << "' engine is already configured. Please specify just one engine!" << std::endl;
+		exit(EXIT_FAILURE);
+	}
+}
+
+void print_message(std::string msg)
+{
+	std::cout << std::endl;
+	std::cout << "-----------------------------------------------------" << std::endl;
+	std::cout << "- " << msg << std::endl;
+	std::cout << "-----------------------------------------------------" << std::endl;
+	std::cout << std::endl;
+}
+
+void print_menu_and_exit()
+{
+	std::string usage = R"(Usage: drivers_test [options]
+
+Overview: The goal of this binary is to run tests against one of our drivers.
+
+Options:
+  -k, --kmod <path>       Run tests against the kernel module. Default path is `./driver/scap.ko`.
+  -m, --modern-bpf        Run tests against the modern bpf probe.
+  -b, --bpf <path>        Run tests against the bpf probe. Default path is `./driver/bpf/probe.o`.
+  -d, --buffer-dim <dim>  Change the dimension of shared buffers between userspace and kernel. You must specify the dimension in bytes.
+  -h, --help              This page.
+)";
+	std::cout << usage << std::endl;
+	exit(EXIT_SUCCESS);
+}
+
 int open_engine(int argc, char** argv)
 {
 	static struct option long_options[] = {
 		{BPF_OPTION, optional_argument, 0, 'b'},
 		{MODERN_BPF_OPTION, no_argument, 0, 'm'},
 		{KMOD_OPTION, optional_argument, 0, 'k'},
 		{BUFFER_OPTION, required_argument, 0, 'd'},
+		{HELP_OPTION, no_argument, 0, 'h'},
 		{0, 0, 0, 0}};
 
 	int ret = 0;
@@ -114,12 +151,13 @@ int open_engine(int argc, char** argv)
 	int op = 0;
 	int long_index = 0;
 	while((op = getopt_long(argc, argv,
-				"b::mk::d:",
+				"b::mk::d:h",
 				long_options, &long_index)) != -1)
 	{
 		switch(op)
 		{
 		case 'b':
+			abort_if_already_configured(&oargs);
 			oargs.engine_name = BPF_ENGINE;
 			bpf_params.buffer_bytes_dim = buffer_bytes_dim;
 			if(optarg == NULL)
@@ -147,13 +185,15 @@ int open_engine(int argc, char** argv)
 			break;
 
 		case 'm':
+			abort_if_already_configured(&oargs);
 			oargs.engine_name = MODERN_BPF_ENGINE;
 			modern_bpf_params.buffer_bytes_dim = buffer_bytes_dim;
 			oargs.engine_params = &modern_bpf_params;
 			std::cout << "* Configure modern BPF probe tests!" << std::endl;
 			break;
 
 		case 'k':
+			abort_if_already_configured(&oargs);
 			oargs.engine_name = KMOD_ENGINE;
 			kmod_params.buffer_bytes_dim = buffer_bytes_dim;
 			if(optarg == NULL)
@@ -177,6 +217,10 @@ int open_engine(int argc, char** argv)
 			buffer_bytes_dim = strtoul(optarg, NULL, 10);
 			break;
 
+		case 'h':
+			print_menu_and_exit();
+			break;
+
 		default:
 			break;
 		}
@@ -190,47 +234,20 @@ int open_engine(int argc, char** argv)
 	}
 
 	char error_buffer[FILENAME_MAX] = {0};
-	event_test::scap_handle = scap_open(&oargs, error_buffer, &ret);
-	if(!event_test::scap_handle)
+	event_test::s_scap_handle = scap_open(&oargs, error_buffer, &ret);
+	if(!event_test::s_scap_handle)
 	{
 		std::cerr << "Unable to open the engine: " << error_buffer << std::endl;
 		return EXIT_FAILURE;
 	}
 	return EXIT_SUCCESS;
 }
 
-void print_setup_phase_message()
-{
-	std::cout << std::endl;
-	std::cout << "-----------------------------------------------------" << std::endl;
-	std::cout << "-------------------- Setup phase --------------------" << std::endl;
-	std::cout << "-----------------------------------------------------" << std::endl;
-	std::cout << std::endl;
-}
-
-void print_start_test_message()
-{
-	std::cout << std::endl;
-	std::cout << "-----------------------------------------------------" << std::endl;
-	std::cout << "------------------- Testing phase -------------------" << std::endl;
-	std::cout << "-----------------------------------------------------" << std::endl;
-	std::cout << std::endl;
-}
-
-void print_teardown_test_message()
-{
-	std::cout << std::endl;
-	std::cout << "-----------------------------------------------------" << std::endl;
-	std::cout << "------------------- Teardown phase ------------------" << std::endl;
-	std::cout << "-----------------------------------------------------" << std::endl;
-	std::cout << std::endl;
-}
-
 int main(int argc, char** argv)
 {
 	int res = EXIT_SUCCESS;
 
-	print_setup_phase_message();
+	print_message("Setup phase");
 
 	::testing::InitGoogleTest(&argc, argv);
 
@@ -241,33 +258,33 @@ int main(int argc, char** argv)
 	}
 
 	/* We need to start the capture to calibrate socket with bpf engine */
-	if(scap_start_capture(event_test::scap_handle) != SCAP_SUCCESS)
+	if(scap_start_capture(event_test::s_scap_handle) != SCAP_SUCCESS)
 	{
-		std::cout << "Error in starting the capture: " << scap_getlasterr(event_test::scap_handle) << std::endl;
+		std::cout << "Error in starting the capture: " << scap_getlasterr(event_test::s_scap_handle) << std::endl;
 		goto cleanup_tests;
 	}
 
 	/* We need to detach all tracepoints before starting tests. */
-	if(scap_stop_capture(event_test::scap_handle) != SCAP_SUCCESS)
+	if(scap_stop_capture(event_test::s_scap_handle) != SCAP_SUCCESS)
 	{
-		std::cout << "Error in stopping the capture: " << scap_getlasterr(event_test::scap_handle) << std::endl;
+		std::cout << "Error in stopping the capture: " << scap_getlasterr(event_test::s_scap_handle) << std::endl;
 		goto cleanup_tests;
 	}
 
 	/* We need to disable also all the interesting syscalls */
-	if(scap_clear_ppm_sc_mask(event_test::scap_handle) != SCAP_SUCCESS)
+	if(scap_clear_ppm_sc_mask(event_test::s_scap_handle) != SCAP_SUCCESS)
 	{
-		std::cout << "Error in clearing the syscalls of interests: " << scap_getlasterr(event_test::scap_handle) << std::endl;
+		std::cout << "Error in clearing the syscalls of interests: " << scap_getlasterr(event_test::s_scap_handle) << std::endl;
 		goto cleanup_tests;
 	}
 
-	print_start_test_message();
+	print_message("Testing phase");
 
 	res = RUN_ALL_TESTS();
 
 cleanup_tests:
-	print_teardown_test_message();
-	scap_close(event_test::scap_handle);
+	print_message("Teardown phase");
+	scap_close(event_test::s_scap_handle);
 	remove_kmod();
 	return res;
 }