From 28773b18672868ebe24de3326f03afb5bd964d7b Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Tue, 20 Dec 2022 22:49:52 +0100 Subject: [PATCH] update: address some review comments Signed-off-by: Andrea Terzolo Co-authored-by: Federico Di Pierro --- test/drivers/README.md | 4 +- test/drivers/event_class/event_class.cpp | 17 ++-- test/drivers/event_class/event_class.h | 12 +-- test/drivers/start_tests.cpp | 99 ++++++++++++++---------- 4 files changed, 74 insertions(+), 58 deletions(-) diff --git a/test/drivers/README.md b/test/drivers/README.md index fb98b61ed5..caa33f324e 100644 --- a/test/drivers/README.md +++ b/test/drivers/README.md @@ -26,7 +26,7 @@ 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: @@ -34,7 +34,7 @@ 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. diff --git a/test/drivers/event_class/event_class.cpp b/test/drivers/event_class/event_class.cpp index 7244343cfc..c563e29683 100644 --- a/test/drivers/event_class/event_class.cpp +++ b/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() diff --git a/test/drivers/event_class/event_class.h b/test/drivers/event_class/event_class.h index 2a632de04e..9621517ad0 100644 --- a/test/drivers/event_class/event_class.h +++ b/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); } ///////////////////////////////// diff --git a/test/drivers/start_tests.cpp b/test/drivers/start_tests.cpp index d04fe018f7..c679b44723 100644 --- a/test/drivers/start_tests.cpp +++ b/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,6 +76,41 @@ 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 Run tests against the kernel module. Default path is `./driver/scap.ko`. + -m, --modern-bpf Run tests against the modern bpf probe. + -b, --bpf Run tests against the bpf probe. Default path is `./driver/bpf/probe.o`. + -d, --buffer-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[] = { @@ -82,6 +118,7 @@ int open_engine(int argc, char** argv) {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,6 +185,7 @@ 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; @@ -154,6 +193,7 @@ int open_engine(int argc, char** argv) 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,8 +234,8 @@ 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; @@ -199,38 +243,11 @@ int open_engine(int argc, char** argv) 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; }