cleanup: address review comments

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

Author: Andreagit97

userspace/libscap/engine/savefile/converter/CMakeLists.txt

@@ -15,7 +15,7 @@
 
 # Since we have circular dependencies between libscap and the savefile engine, make this library
 # always static (directly linked into libscap)
-add_library(scap_savefile_converter STATIC converter.cpp)
+add_library(scap_savefile_converter STATIC converter.cpp table.cpp)
 
 add_dependencies(scap_savefile_converter uthash)
 target_include_directories(

userspace/libscap/engine/savefile/converter/converter.cpp

@@ -19,7 +19,6 @@ limitations under the License.
 #include <driver/ppm_events_public.h>
 #include <converter/types.h>
 #include <converter/results.h>
-#include <converter/table.h>
 #include <converter/debug_macro.h>
 #include <stdarg.h>
 #include <cstdio>
@@ -29,6 +28,8 @@ limitations under the License.
 #include <stdexcept>
 #include <memory>
 
+extern std::unordered_map<conversion_key, conversion_info> g_conversion_table;
+
 typedef std::shared_ptr<scap_evt> safe_scap_evt_t;
 
 static inline safe_scap_evt_t safe_scap_evt(scap_evt *evt) {
@@ -124,7 +125,6 @@ static void push_default_parameter(scap_evt *evt, uint16_t *params_offset, uint8
 	// Otherwise we will access the wrong entry in the event table.
 	const struct ppm_event_info *event_info = &(g_event_info[evt->type]);
 	uint16_t len = scap_get_size_bytes_from_type(event_info->params[param_num].type);
-	char *ptr = scap_get_default_value_from_type(event_info->params[param_num].type);
 	uint16_t lens_offset = sizeof(scap_evt) + param_num * sizeof(uint16_t);
 
 	PRINT_MESSAGE(
@@ -136,8 +136,11 @@ static void push_default_parameter(scap_evt *evt, uint16_t *params_offset, uint8
 	        *params_offset,
 	        lens_offset);
 
-	// If value is NULL, the len should be 0
-	memcpy((char *)evt + *params_offset, ptr, len);
+	// The default param will be always 0 so we just need to copy the right number of 0 bytes.
+	// `uint64_t` should be enough for all the types considering that types like CHARBUF, BYTEBUF
+	// have `len==0`
+	static uint64_t val = 0;
+	memcpy((char *)evt + *params_offset, (char *)&val, len);
 	*params_offset += len;
 	memcpy((char *)evt + lens_offset, &len, sizeof(uint16_t));
 }

…ibscap/engine/savefile/converter/table.h …scap/engine/savefile/converter/table.cppuserspace/libscap/engine/savefile/converter/table.h renamed to userspace/libscap/engine/savefile/converter/table.cpp userspace/libscap/engine/savefile/converter/table.h renamed to userspace/libscap/engine/savefile/converter/table.cpp

@@ -15,12 +15,10 @@ See the License for the specific language governing permissions and
 limitations under the License.
 
 */
-#pragma once
 
 #include <converter/types.h>
 #include <libscap/scap_const.h>
 #include <driver/ppm_events_public.h>
-
 #include <unordered_map>
 
-static std::unordered_map<conversion_key, conversion_info> g_conversion_table = {};
+std::unordered_map<conversion_key, conversion_info> g_conversion_table = {};

userspace/libscap/scap.h

@@ -856,7 +856,6 @@ int32_t scap_event_encode_params_v(struct scap_sized_buffer event_buf,
                                    uint32_t n,
                                    va_list args);
 uint8_t scap_get_size_bytes_from_type(enum ppm_param_type t);
-char* scap_get_default_value_from_type(enum ppm_param_type t);
 bool scap_compare_events(scap_evt* curr, scap_evt* expected, char* error);
 scap_evt* scap_create_event_v(char* error,
                               uint64_t ts,

userspace/libscap/scap_event.c

@@ -410,59 +410,6 @@ uint8_t scap_get_size_bytes_from_type(enum ppm_param_type t) {
 	return 0;
 }
 
-char *scap_get_default_value_from_type(enum ppm_param_type t) {
-	static uint8_t default_uint8 = 0;
-	static uint16_t default_uint16 = 0;
-	static uint32_t default_uint32 = 0;
-	static uint64_t default_uint64 = 0;
-	switch(t) {
-	case PT_INT8:
-	case PT_UINT8:
-	case PT_FLAGS8:
-	case PT_ENUMFLAGS8:
-		return (char *)&default_uint8;
-	case PT_INT16:
-	case PT_UINT16:
-	case PT_FLAGS16:
-	case PT_ENUMFLAGS16:
-	case PT_SYSCALLID:
-		return (char *)&default_uint16;
-	case PT_INT32:
-	case PT_UINT32:
-	case PT_FLAGS32:
-	case PT_ENUMFLAGS32:
-	case PT_UID:
-	case PT_GID:
-	case PT_MODE:
-		return (char *)&default_uint32;
-	case PT_INT64:
-	case PT_UINT64:
-	case PT_RELTIME:
-	case PT_ABSTIME:
-	case PT_ERRNO:
-	case PT_FD:
-	case PT_PID:
-		return (char *)&default_uint64;
-	// Should be ok to return NULL since the len will be 0
-	case PT_BYTEBUF:
-	case PT_CHARBUF:
-	case PT_SOCKADDR:
-	case PT_SOCKTUPLE:
-	case PT_FDLIST:
-	case PT_FSPATH:
-	case PT_CHARBUFARRAY:
-	case PT_CHARBUF_PAIR_ARRAY:
-	case PT_FSRELPATH:
-	case PT_DYN:
-		return NULL;
-	default:
-		// We forgot to handle something
-		ASSERT(false);
-		break;
-	}
-	return 0;
-}
-
 // This should be only used for testing purposes in production we should use directly `memcmp` for
 // the whole event
 bool scap_compare_events(scap_evt *curr, scap_evt *expected, char *error) {