cleanup(scap,sinsp): return event flags directly

The event flags are (currently) specific to the savefile engine but accessing them requires an awkward reacharound through a dedicated vtable method to a field stored on each call to next(). We can simplify this a bit and express the intent better by simply returning the flags from next() via an out pointer. Signed-off-by: Grzegorz Nosek <[email protected]>
falcosecurity · Oct 17, 2023 · 43eebdb · 43eebdb
1 parent f7b667e
commit 43eebdb
Show file tree

Hide file tree

Showing 22 changed files with 54 additions and 59 deletions.
diff --git a/userspace/libscap/engine/bpf/scap_bpf.c b/userspace/libscap/engine/bpf/scap_bpf.c
@@ -1865,9 +1865,9 @@ int32_t scap_bpf_get_n_tracepoint_hit(struct scap_engine_handle engine, long* re
 	return SCAP_SUCCESS;
 }
 
-static int32_t next(struct scap_engine_handle engine, OUT scap_evt **pevent, OUT uint16_t *pdevid)
+static int32_t next(struct scap_engine_handle engine, OUT scap_evt **pevent, OUT uint16_t *pdevid, OUT uint32_t *pflags)
 {
-	return ringbuffer_next(&engine.m_handle->m_dev_set, pevent, pdevid);
+	return ringbuffer_next(&engine.m_handle->m_dev_set, pevent, pdevid, pflags);
 }
 
 static int32_t unsupported_config(struct scap_engine_handle engine, const char* msg)

diff --git a/userspace/libscap/engine/gvisor/gvisor.cpp b/userspace/libscap/engine/gvisor/gvisor.cpp
@@ -173,9 +173,9 @@ static int32_t gvisor_stop_capture(struct scap_engine_handle engine)
 	return engine.m_handle->stop_capture();
 }
 
-static int32_t gvisor_next(struct scap_engine_handle engine, scap_evt** pevent, uint16_t* pdevid)
+static int32_t gvisor_next(struct scap_engine_handle engine, scap_evt** pevent, uint16_t* pdevid, uint32_t* pflags)
 {
-	return engine.m_handle->next(pevent, pdevid);
+	return engine.m_handle->next(pevent, pdevid, pflags);
 }
 
 static int32_t gvisor_configure(struct scap_engine_handle engine, enum scap_setting setting, unsigned long arg1, unsigned long arg2)

diff --git a/userspace/libscap/engine/gvisor/gvisor.h b/userspace/libscap/engine/gvisor/gvisor.h
@@ -142,7 +142,7 @@ class engine {
     int32_t start_capture();
     int32_t stop_capture();
 
-    int32_t next(scap_evt **pevent, uint16_t *pdevid);
+    int32_t next(scap_evt **pevent, uint16_t *pdevid, uint32_t *pflags);
 
     uint32_t get_vxid(uint64_t pid);
     int32_t get_stats(scap_stats *stats);

diff --git a/userspace/libscap/engine/gvisor/scap_gvisor.cpp b/userspace/libscap/engine/gvisor/scap_gvisor.cpp
@@ -506,7 +506,7 @@ int32_t engine::process_message_from_fd(int fd)
 	return parse_result.status;
 }
 
-int32_t engine::next(scap_evt **pevent, uint16_t *pdevid)
+int32_t engine::next(scap_evt **pevent, uint16_t *pdevid, uint32_t *pflags)
 {
 	if(m_no_events)
 	{
@@ -600,6 +600,7 @@ int32_t engine::next(scap_evt **pevent, uint16_t *pdevid)
 	if(!m_event_queue.empty())
 	{
 		*pevent = m_event_queue.front();
+		*pflags = 0;
 		m_event_queue.pop_front();
 		m_gvisor_stats.n_evts++;
 		return SCAP_SUCCESS;

diff --git a/userspace/libscap/engine/kmod/scap_kmod.c b/userspace/libscap/engine/kmod/scap_kmod.c
@@ -541,9 +541,10 @@ int32_t scap_kmod_close(struct scap_engine_handle engine)
 	return SCAP_SUCCESS;
 }
 
-int32_t scap_kmod_next(struct scap_engine_handle engine, OUT scap_evt **pevent, OUT uint16_t *pdevid)
+int32_t scap_kmod_next(struct scap_engine_handle engine, OUT scap_evt **pevent, OUT uint16_t *pdevid,
+		       OUT uint32_t *pflags)
 {
-	return ringbuffer_next(&engine.m_handle->m_dev_set, pevent, pdevid);
+	return ringbuffer_next(&engine.m_handle->m_dev_set, pevent, pdevid, pflags);
 }
 
 uint32_t scap_kmod_get_n_devs(struct scap_engine_handle engine)

diff --git a/userspace/libscap/engine/modern_bpf/scap_modern_bpf.c b/userspace/libscap/engine/modern_bpf/scap_modern_bpf.c
@@ -49,7 +49,8 @@ static void scap_modern_bpf__free_engine(struct scap_engine_handle engine)
 /* The third parameter is not the CPU number from which we extract the event but the ring buffer number.
  * For the old BPF probe and the kernel module the number of CPUs is equal to the number of buffers since we always use a per-CPU approach.
  */
-static int32_t scap_modern_bpf__next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT uint16_t* buffer_id)
+static int32_t scap_modern_bpf__next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT uint16_t* buffer_id,
+				     OUT uint32_t* pflags)
 {
 	pman_consume_first_event((void**)pevent, (int16_t*)buffer_id);
 
@@ -64,6 +65,7 @@ static int32_t scap_modern_bpf__next(struct scap_engine_handle engine, OUT scap_
 	{
 		engine.m_handle->m_retry_us = BUFFER_EMPTY_WAIT_TIME_US_START;
 	}
+	*pflags = 0;
 	return SCAP_SUCCESS;
 }
 

diff --git a/userspace/libscap/engine/nodriver/nodriver.c b/userspace/libscap/engine/nodriver/nodriver.c
@@ -44,7 +44,7 @@ static int32_t init(scap_t* handle, scap_open_args *oargs)
 	return SCAP_SUCCESS;
 }
 
-static int32_t next(struct scap_engine_handle handle, scap_evt** pevent, uint16_t* pdevid)
+static int32_t next(struct scap_engine_handle handle, scap_evt** pevent, uint16_t* pdevid, uint32_t* pflags)
 {
 	static scap_evt evt;
 	evt.len = 0;
@@ -56,6 +56,8 @@ static int32_t next(struct scap_engine_handle handle, scap_evt** pevent, uint16_
 
 	evt.ts = get_timestamp_ns();
 	*pevent = &evt;
+	*pdevid = 0;
+	*pflags = 0;
 	return SCAP_SUCCESS;
 }
 

diff --git a/userspace/libscap/engine/noop/noop.c b/userspace/libscap/engine/noop/noop.c
@@ -50,7 +50,10 @@ int noop_close_engine(struct scap_engine_handle engine)
 	return SCAP_SUCCESS;
 }
 
-int32_t noop_next(struct scap_engine_handle handle, scap_evt** pevent, uint16_t* pdevid) { return SCAP_EOF; }
+int32_t noop_next(struct scap_engine_handle handle, scap_evt** pevent, uint16_t* pdevid, uint32_t* pflags)
+{
+	return SCAP_EOF;
+}
 
 int32_t noop_start_capture(struct scap_engine_handle engine)
 {

diff --git a/userspace/libscap/engine/noop/noop.h b/userspace/libscap/engine/noop/noop.h
@@ -29,7 +29,7 @@ typedef struct scap_stats_v2 scap_stats_v2;
 struct noop_engine* noop_alloc_handle(scap_t* main_handle, char* lasterr_ptr);
 void noop_free_handle(struct scap_engine_handle engine);
 int noop_close_engine(struct scap_engine_handle engine);
-int32_t noop_next(struct scap_engine_handle handle, scap_evt** pevent, uint16_t* pdevid);
+int32_t noop_next(struct scap_engine_handle handle, scap_evt** pevent, uint16_t* pdevid, uint32_t* pflags);
 int32_t noop_start_capture(struct scap_engine_handle engine);
 int32_t noop_stop_capture(struct scap_engine_handle engine);
 int32_t unimplemented_op(char* err, size_t err_size);

diff --git a/userspace/libscap/engine/savefile/scap_savefile.c b/userspace/libscap/engine/savefile/scap_savefile.c
@@ -1925,7 +1925,7 @@ static int32_t scap_read_init(struct savefile_engine *handle, scap_reader_t* r,
 //
 // Read an event from disk
 //
-static int32_t next(struct scap_engine_handle engine, scap_evt **pevent, uint16_t *pdevid)
+static int32_t next(struct scap_engine_handle engine, scap_evt **pevent, uint16_t *pdevid, uint32_t *pflags)
 {
 	struct savefile_engine* handle = engine.m_handle;
 	block_header bh;
@@ -2039,12 +2039,12 @@ static int32_t next(struct scap_engine_handle engine, scap_evt **pevent, uint16_
 
 		if(bh.block_type == EVF_BLOCK_TYPE || bh.block_type == EVF_BLOCK_TYPE_V2 || bh.block_type == EVF_BLOCK_TYPE_V2_LARGE)
 		{
-			handle->m_last_evt_dump_flags = *(uint32_t*)(handle->m_reader_evt_buf + sizeof(uint16_t));
+			*pflags = *(uint32_t *)(handle->m_reader_evt_buf + sizeof(uint16_t));
 			*pevent = (struct ppm_evt_hdr *)(handle->m_reader_evt_buf + sizeof(uint16_t) + sizeof(uint32_t));
 		}
 		else
 		{
-			handle->m_last_evt_dump_flags = 0;
+			*pflags = 0;
 			*pevent = (struct ppm_evt_hdr *)(handle->m_reader_evt_buf + sizeof(uint16_t));
 		}
 
@@ -2351,18 +2351,12 @@ static int64_t get_readfile_offset(struct scap_engine_handle engine)
 	return engine.m_handle->m_reader->offset(engine.m_handle->m_reader);
 }
 
-static uint32_t get_event_dump_flags(struct scap_engine_handle engine)
-{
-	return engine.m_handle->m_last_evt_dump_flags;
-}
-
 static struct scap_savefile_vtable savefile_ops = {
 	.ftell_capture = scap_savefile_ftell,
 	.fseek_capture = scap_savefile_fseek,
 
 	.restart_capture = scap_savefile_restart_capture,
 	.get_readfile_offset = get_readfile_offset,
-	.get_event_dump_flags = get_event_dump_flags,
 };
 
 struct scap_vtable scap_savefile_engine = {

diff --git a/userspace/libscap/engine/source_plugin/source_plugin.c b/userspace/libscap/engine/source_plugin/source_plugin.c
@@ -155,7 +155,7 @@ static int close_engine(struct scap_engine_handle engine)
 	return SCAP_SUCCESS;
 }
 
-static int32_t next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT uint16_t* pdevid)
+static int32_t next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT uint16_t* pdevid, OUT uint32_t* pflags)
 {
 	struct source_plugin_engine *handle = engine.m_handle;
 	char *lasterr = engine.m_handle->m_lasterr;
@@ -255,6 +255,8 @@ static int32_t next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT
 	}
 
 	*pevent = evt;
+	*pdevid = 0;
+	*pflags = 0;
 	handle->m_nevts++;
 	handle->m_input_plugin_batch_idx++;
 	return SCAP_SUCCESS;

diff --git a/userspace/libscap/engine/test_input/test_input.c b/userspace/libscap/engine/test_input/test_input.c
@@ -52,7 +52,7 @@ static struct test_input_engine* alloc_handle(scap_t* main_handle, char* lasterr
 	return engine;
 }
 
-static int32_t next(struct scap_engine_handle handle, scap_evt** pevent, uint16_t* pdevid)
+static int32_t next(struct scap_engine_handle handle, scap_evt** pevent, uint16_t* pdevid, uint32_t* pflags)
 {
 	test_input_engine *engine = handle.m_handle;
 	scap_test_input_data *data = engine->m_data;
@@ -66,6 +66,7 @@ static int32_t next(struct scap_engine_handle handle, scap_evt** pevent, uint16_
 	data->event_count--;
 	/* All the events are sent by device 1 */
 	*pdevid = 1;
+	*pflags = 0;
 	return SCAP_SUCCESS;
 }
 

diff --git a/userspace/libscap/engine/udig/scap_udig.c b/userspace/libscap/engine/udig/scap_udig.c
@@ -400,9 +400,9 @@ static int close_engine(struct scap_engine_handle engine)
 	return SCAP_SUCCESS;
 }
 
-static int32_t next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT uint16_t* pdevid)
+static int32_t next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT uint16_t* pdevid, OUT uint32_t* pflags)
 {
-	return ringbuffer_next(&engine.m_handle->m_dev_set, pevent, pdevid);
+	return ringbuffer_next(&engine.m_handle->m_dev_set, pevent, pdevid, pflags);
 }
 
 //

diff --git a/userspace/libscap/examples/01-open/scap_open.c b/userspace/libscap/examples/01-open/scap_open.c
@@ -837,6 +837,7 @@ int main(int argc, char** argv)
 	int32_t res = 0;
 	scap_evt* ev = NULL;
 	uint16_t cpuid = 0;
+	uint32_t flags = 0;
 
 	printf("\n[SCAP-OPEN]: Hello!\n");
 	if(signal(SIGINT, signal_callback) == SIG_ERR)
@@ -873,7 +874,7 @@ int main(int argc, char** argv)
 
 	while(g_nevts != num_events)
 	{
-		res = scap_next(g_h, &ev, &cpuid);
+		res = scap_next(g_h, &ev, &cpuid, &flags);
 		number_of_scap_next++;
 		if(res == SCAP_UNEXPECTED_BLOCK)
 		{

diff --git a/userspace/libscap/ringbuffer/ringbuffer.h b/userspace/libscap/ringbuffer/ringbuffer.h
@@ -211,7 +211,8 @@ static inline void ringbuffer_advance_to_evt(scap_device* dev, scap_evt *event)
  * - before refilling a buffer we have to consume all the others!
  * - we perform a lot of cycles but we have to be super fast here!
  */
-static inline int32_t ringbuffer_next(struct scap_device_set* devset, OUT scap_evt** pevent, OUT uint16_t* pdevid)
+static inline int32_t ringbuffer_next(struct scap_device_set* devset, OUT scap_evt** pevent, OUT uint16_t* pdevid,
+				      OUT uint32_t* pflags)
 {
 	uint32_t j;
 	uint64_t min_ts = 0xffffffffffffffffLL;
@@ -288,6 +289,9 @@ static inline int32_t ringbuffer_next(struct scap_device_set* devset, OUT scap_e
 	 	 */
 		struct scap_device* dev = &devset->m_devs[*pdevid];
 		ADVANCE_TO_EVT(dev, (*pevent));
+
+		// we don't really store the flags in the ringbuffer anywhere
+		*pflags = 0;
 		return SCAP_SUCCESS;
 	}
 	else

diff --git a/userspace/libscap/scap.c b/userspace/libscap/scap.c
@@ -312,7 +312,7 @@ uint64_t scap_max_buf_used(scap_t* handle)
 	return 0;
 }
 
-int32_t scap_next(scap_t* handle, OUT scap_evt** pevent, OUT uint16_t* pdevid)
+int32_t scap_next(scap_t* handle, OUT scap_evt** pevent, OUT uint16_t* pdevid, OUT uint32_t* pflags)
 {
 	// Note: devid is like cpuid but not 1:1, e.g. consider CPU1 offline:
 	// CPU0 CPU1 CPU2 CPU3
@@ -321,7 +321,7 @@ int32_t scap_next(scap_t* handle, OUT scap_evt** pevent, OUT uint16_t* pdevid)
 	int32_t res = SCAP_FAILURE;
 	if(handle->m_vtable)
 	{
-		res = handle->m_vtable->next(handle->m_engine, pevent, pdevid);
+		res = handle->m_vtable->next(handle->m_engine, pevent, pdevid, pflags);
 	}
 	else
 	{
@@ -551,18 +551,6 @@ int32_t scap_set_dropfailed(scap_t* handle, bool enabled) {
 	return SCAP_FAILURE;
 }
 
-uint32_t scap_event_get_dump_flags(scap_t* handle)
-{
-	if(handle->m_vtable->savefile_ops)
-	{
-		return handle->m_vtable->savefile_ops->get_event_dump_flags(handle->m_engine);
-	}
-	else
-	{
-		return 0;
-	}
-}
-
 int32_t scap_enable_dynamic_snaplen(scap_t* handle)
 {
 	if(handle->m_vtable)

diff --git a/userspace/libscap/scap.h b/userspace/libscap/scap.h
@@ -569,13 +569,14 @@ uint64_t scap_max_buf_used(scap_t* handle);
   \param pevent User-provided event pointer that will be initialized with address of the event.
   \param pdevid User-provided event pointer that will be initialized with the ID of the device
     where the event was captured.
+  \param pflags User-provided event pointer that will be initialized with the flags of the event.
 
-  \return SCAP_SUCCESS if the call is successful and pevent and pdevid contain valid data.
+  \return SCAP_SUCCESS if the call is successful and pevent, pcpuid and pflags contain valid data.
    SCAP_TIMEOUT in case the read timeout expired and no event is available.
    SCAP_EOF when the end of an offline capture is reached.
    On Failure, SCAP_FAILURE is returned and scap_getlasterr() can be used to obtain the cause of the error.
 */
-int32_t scap_next(scap_t* handle, OUT scap_evt** pevent, OUT uint16_t* pdevid);
+int32_t scap_next(scap_t* handle, OUT scap_evt** pevent, OUT uint16_t* pcpuid, OUT uint32_t* pflags);
 
 /*!
   \brief Get the length of an event

diff --git a/userspace/libscap/scap_vtable.h b/userspace/libscap/scap_vtable.h
@@ -116,13 +116,6 @@ struct scap_savefile_vtable {
 	 * @return the current read offset, in (compressed) bytes
 	 */
 	int64_t (*get_readfile_offset)(struct scap_engine_handle engine);
-
-	/**
-	 * @brief return the flags for the last read event
-	 * @param engine the handle to the engine
-	 * @return the flags of the event (currently only SCAP_DF_LARGE is supported)
-	 */
-	uint32_t (*get_event_dump_flags)(struct scap_engine_handle engine);
 };
 
 struct scap_vtable {
@@ -176,6 +169,7 @@ struct scap_vtable {
 	 * @param pevent [out] where the pointer to the next event gets stored
 	 * @param pdevid [out] where the device on which the event was received
 	 *               gets stored
+	 * @param pflags [out] where the flags for the event get stored
 	 * @return SCAP_SUCCESS or a failure code
 	 *
 	 * SCAP_SUCCESS: event successfully returned and stored in *pevent
@@ -186,7 +180,7 @@ struct scap_vtable {
 	 * The memory pointed to by *pevent must be owned by the engine
 	 * and must remain valid at least until the next call to next()
 	 */
-	int32_t (*next)(struct scap_engine_handle engine, scap_evt** pevent, uint16_t* pdevid);
+	int32_t (*next)(struct scap_engine_handle engine, scap_evt** pevent, uint16_t* pdevid, uint32_t* pflags);
 
 	/**
 	 * @brief start a capture

diff --git a/userspace/libsinsp/event.cpp b/userspace/libsinsp/event.cpp
@@ -123,11 +123,6 @@ sinsp_evt::~sinsp_evt()
 	}
 }
 
-uint32_t sinsp_evt::get_dump_flags()
-{
-	return scap_event_get_dump_flags(m_inspector->m_h);
-}
-
 const char *sinsp_evt::get_name() const
 {
 	return m_info->name;

diff --git a/userspace/libsinsp/event.h b/userspace/libsinsp/event.h
@@ -562,7 +562,7 @@ class SINSP_PUBLIC sinsp_evt : public gen_event
 	std::string get_param_value_str(const char* name, bool resolved = true);
 	char* render_fd(int64_t fd, const char** resolved_str, sinsp_evt::param_fmt fmt);
 	int render_fd_json(Json::Value *ret, int64_t fd, const char** resolved_str, sinsp_evt::param_fmt fmt);
-	uint32_t get_dump_flags();
+	inline uint32_t get_dump_flags() const { return m_dump_flags; }
 	static bool clone_event(sinsp_evt& dest, const sinsp_evt& src);
 	int32_t get_errorcode() { return m_errorcode; }
 
@@ -586,6 +586,7 @@ VISIBILITY_PRIVATE
 	uint16_t m_cpuid;
 	uint64_t m_evtnum;
 	uint32_t m_flags;
+	uint32_t m_dump_flags;
 	bool m_params_loaded;
 	const struct ppm_event_info* m_info;
 	std::vector<sinsp_evt_param> m_params;