[RFC PATCH 1/2] tracecmd library: Use libtraceevent record refcounting

[RFC PATCH 1/2] tracecmd library: Use libtraceevent record refcounting

[Benjamin Berg]

Moving this into libtraceevent has the advantage that a python plugin
loader can be implemented that does not need access to trace-cmd API.
---
 .../include/private/trace-cmd-private.h       |  2 +-
 lib/trace-cmd/trace-input.c                   | 94 +++++++++----------
 2 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index d73a5191..b08ebcee 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -235,7 +235,7 @@ tracecmd_peek_data_ref(struct tracecmd_input *handle, int cpu)
 {
 	struct tep_record *rec = tracecmd_peek_data(handle, cpu);
 	if (rec)
-		rec->ref_count++;
+		tep_record_ref(rec);
 	return rec;
 }
 
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index cdaa17bd..157e2d97 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -35,6 +35,16 @@
 /* for debugging read instead of mmap */
 static int force_read = 0;
 
+#define REC_PRIV(rec) ((struct record_priv*) (rec)->priv)
+struct record_priv {
+	struct page		*page;
+	int			locked;
+#if DEBUG_RECORD
+	struct tep_record	*prev;
+	struct tep_record	*next;
+#endif
+};
+
 struct page_map {
 	struct list_head	list;
 	off64_t			offset;
@@ -261,19 +271,23 @@ void *tracecmd_get_private(struct tracecmd_input *handle)
 #if DEBUG_RECORD
 static void remove_record(struct page *page, struct tep_record *record)
 {
-	if (record->prev)
-		record->prev->next = record->next;
+	struct record_priv *priv = REC_PRIV(record);
+
+	if (priv->prev)
+		REC_PRIV(priv->prev)->next = priv->next;
 	else
-		page->records = record->next;
-	if (record->next)
-		record->next->prev = record->prev;
+		page->records = priv->next;
+	if (priv->next)
+		REC_PRIV(priv->next)->prev = priv->prev;
 }
 static void add_record(struct page *page, struct tep_record *record)
 {
+	struct record_priv *priv = REC_PRIV(record);
+
 	if (page->records)
-		page->records->prev = record;
-	record->next = page->records;
-	record->prev = NULL;
+		REC_PRIV(page->records)->prev = record;
+	priv->next = page->records;
+	priv->prev = NULL;
 	page->records = record;
 }
 static const char *show_records(struct page **pages, int nr_pages)
@@ -290,7 +304,7 @@ static const char *show_records(struct page **pages, int nr_pages)
 		page = pages[i];
 		if (!page)
 			continue;
-		for (record = page->records; record; record = record->next) {
+		for (record = page->records; record; record = REC_PRIV(record)->next) {
 			int n;
 			n = snprintf(buf+len, BUFSIZ - len, " 0x%lx", record->alloc_addr);
 			len += n;
@@ -1601,13 +1615,20 @@ static void free_page(struct tracecmd_input *handle, int cpu)
 
 static void __free_record(struct tep_record *record)
 {
-	if (record->priv) {
-		struct page *page = record->priv;
+	struct record_priv *priv = record->priv;
+
+	if (priv->locked) {
+		tracecmd_critical("freeing record when it is locked!");
+		return;
+	}
+
+	record->data = NULL;
+
+	if (priv->page) {
+		struct page *page = priv->page;
 		remove_record(page, record);
 		__free_page(page->handle, page);
 	}
-
-	free(record);
 }
 
 void tracecmd_free_record(struct tep_record *record)
@@ -1615,29 +1636,12 @@ void tracecmd_free_record(struct tep_record *record)
 	if (!record)
 		return;
 
-	if (!record->ref_count) {
-		tracecmd_critical("record ref count is zero!");
-		return;
-	}
-
-	record->ref_count--;
-
-	if (record->ref_count)
-		return;
-
-	if (record->locked) {
-		tracecmd_critical("freeing record when it is locked!");
-		return;
-	}
-
-	record->data = NULL;
-
-	__free_record(record);
+	tep_record_unref(record);
 }
 
 void tracecmd_record_ref(struct tep_record *record)
 {
-	record->ref_count++;
+	tep_record_ref(record);
 #if DEBUG_RECORD
 	/* Update locating of last reference */
 	record->alloc_addr = (unsigned long)__builtin_return_address(0);
@@ -1657,7 +1661,7 @@ static void free_next(struct tracecmd_input *handle, int cpu)
 
 	handle->cpu_data[cpu].next = NULL;
 
-	record->locked = 0;
+	REC_PRIV(record)->locked = 0;
 	tracecmd_free_record(record);
 }
 
@@ -2353,12 +2357,10 @@ tracecmd_translate_data(struct tracecmd_input *handle,
 	if (size < 8)
 		return NULL;
 
-	record = malloc(sizeof(*record));
+	record = tep_record_alloc(sizeof(struct record_priv), __free_record);
 	if (!record)
 		return NULL;
-	memset(record, 0, sizeof(*record));
 
-	record->ref_count = 1;
 	if (tep_is_local_bigendian(pevent) == tep_is_file_bigendian(pevent))
 		swap = 0;
 	record->data = kbuffer_translate_data(swap, ptr, &length);
@@ -2437,10 +2439,9 @@ read_again:
 
 	index = kbuffer_curr_offset(kbuf);
 
-	record = malloc(sizeof(*record));
+	record = tep_record_alloc(sizeof(struct record_priv), __free_record);
 	if (!record)
 		return NULL;
-	memset(record, 0, sizeof(*record));
 
 	record->ts = handle->cpu_data[cpu].timestamp;
 	record->size = kbuffer_event_size(kbuf);
@@ -2448,13 +2449,12 @@ read_again:
 	record->data = data;
 	record->offset = handle->cpu_data[cpu].offset + index;
 	record->missed_events = kbuffer_missed_events(kbuf);
-	record->ref_count = 1;
-	record->locked = 1;
+	REC_PRIV(record)->locked = 1;
 
 	handle->cpu_data[cpu].next = record;
 
 	record->record_size = kbuffer_curr_size(kbuf);
-	record->priv = page;
+	REC_PRIV(record)->page = page;
 	add_record(page, record);
 	page->ref_count++;
 
@@ -2484,7 +2484,7 @@ tracecmd_read_data(struct tracecmd_input *handle, int cpu)
 	record = tracecmd_peek_data(handle, cpu);
 	handle->cpu_data[cpu].next = NULL;
 	if (record) {
-		record->locked = 0;
+		REC_PRIV(record)->locked = 0;
 #if DEBUG_RECORD
 		record->alloc_addr = (unsigned long)__builtin_return_address(0);
 #endif
@@ -5619,7 +5619,7 @@ __hidden int tracecmd_copy_trace_data(struct tracecmd_input *in_handle,
 int tracecmd_record_at_buffer_start(struct tracecmd_input *handle,
 				    struct tep_record *record)
 {
-	struct page *page = record->priv;
+	struct page *page = REC_PRIV(record)->page;
 	struct kbuffer *kbuf = handle->cpu_data[record->cpu].kbuf;
 	int offset;
 
@@ -5633,7 +5633,7 @@ int tracecmd_record_at_buffer_start(struct tracecmd_input *handle,
 unsigned long long tracecmd_page_ts(struct tracecmd_input *handle,
 				    struct tep_record *record)
 {
-	struct page *page = record->priv;
+	struct page *page = REC_PRIV(record)->page;
 	struct kbuffer *kbuf = handle->cpu_data[record->cpu].kbuf;
 
 	if (!page || !kbuf)
@@ -5646,7 +5646,7 @@ unsigned int tracecmd_record_ts_delta(struct tracecmd_input *handle,
 				      struct tep_record *record)
 {
 	struct kbuffer *kbuf = handle->cpu_data[record->cpu].kbuf;
-	struct page *page = record->priv;
+	struct page *page = REC_PRIV(record)->page;
 	int offset;
 
 	if (!page || !kbuf)
@@ -5666,7 +5666,7 @@ struct kbuffer *tracecmd_record_kbuf(struct tracecmd_input *handle,
 void *tracecmd_record_page(struct tracecmd_input *handle,
 			   struct tep_record *record)
 {
-	struct page *page = record->priv;
+	struct page *page = REC_PRIV(record)->page;
 
 	return page ? page->map : NULL;
 }
@@ -5674,7 +5674,7 @@ void *tracecmd_record_page(struct tracecmd_input *handle,
 void *tracecmd_record_offset(struct tracecmd_input *handle,
 			     struct tep_record *record)
 {
-	struct page *page = record->priv;
+	struct page *page = REC_PRIV(record)->page;
 	int offset;
 
 	if (!page)
-- 
2.37.3

[RFC PATCH 2/2] trace-cmd: Add back python support without wrapping trace-cmd API

[Benjamin Berg]

This code appears to be working at this point. Note that it is a pure
libtraceevent plugin at this point (the trace-cmd API is not wrapped
anymore). As such, the code should live in libtraceevent rather than
here.

Also, the Makefile changes are incomplete.

Coming from the GNOME side, I was considering that a good approach might
be to create a GLib based wrapper library for libtraceevent and a
separate (python) plugin loader using it. I suppose, if people are happy
with the current SWIG bindings, then it is not worth to do that though.
---
 Makefile                      |   5 +-
 python/Makefile               |  11 +++-
 python/ctracecmd.i            |  12 ++--
 python/plugin_python_loader.c | 115 ++++++++++++++++++++++++++++++++++
 python/tracecmd.py            |  91 +++------------------------
 5 files changed, 143 insertions(+), 91 deletions(-)
 create mode 100644 python/plugin_python_loader.c

diff --git a/Makefile b/Makefile
index 7178d7a9..19649f4a 100644
--- a/Makefile
+++ b/Makefile
@@ -142,7 +142,7 @@ NO_PYTHON = 1
 endif
 
 ifndef NO_PYTHON
-PYTHON		:= ctracecmd.so
+PYTHON		:= ctracecmd.so plugin_python_loader.so
 
 PYTHON_VERS ?= python
 PYTHON_PKGCONFIG_VERS ?= $(PYTHON_VERS)
@@ -585,6 +585,9 @@ export PYGTK_CFLAGS
 ctracecmd.so: force $(LIBTRACECMD_STATIC)
 	$(Q)$(MAKE) -C $(src)/python $@
 
+plugin_python_loader.so: force $(LIBTRACECMD_STATIC)
+	$(Q)$(MAKE) -C $(src)/python $@
+
 PHONY += python
 python: $(PYTHON)
 
diff --git a/python/Makefile b/python/Makefile
index 63f5736d..d0bab375 100644
--- a/python/Makefile
+++ b/python/Makefile
@@ -6,10 +6,11 @@ ifdef BUILD_PYTHON_WORKS
 PYTHON_SO_INSTALL := ctracecmd.install
 PYTHON_PY_PROGS := event-viewer.install
 PYTHON_PY_LIBS := tracecmd.install
+PYTHON_PLUGIN := plugin_python_loader.so
 endif
 
 ctracecmd.so: ctracecmd.i $(LIBTRACECMD_STATIC)
-	swig -Wall -python -noproxy -I$(src)/include/trace-cmd $(LIBTRACEEVENT_CFLAGS) ctracecmd.i
+	swig -Wall -python -noproxy $(LIBTRACEEVENT_CFLAGS) ctracecmd.i
 	$(CC) -fpic -c $(CPPFLAGS) $(CFLAGS) $(PYTHON_INCLUDES)  ctracecmd_wrap.c
 	$(CC) --shared $(LIBTRACECMD_STATIC) $(LDFLAGS) ctracecmd_wrap.o -o ctracecmd.so $(TRACE_LIBS)
 
@@ -22,7 +23,13 @@ $(PYTHON_PY_PROGS): %.install : %.py force
 $(PYTHON_PY_LIBS): %.install : %.py force
 	$(Q)$(call do_install_data,$<,$(python_dir_SQ))
 
-install_python: $(PYTHON_SO_INSTALL) $(PYTHON_PY_PROGS) $(PYTHON_PY_LIBS)
+plugin_python_loader.so: %.so: %.o
+	$(Q)$(do_python_plugin_build)
+
+plugin_python_loader.o: %.o : %.c
+	$(Q)$(do_compile_python_plugin_obj)
+
+install_python: $(PYTHON_SO_INSTALL) $(PYTHON_PY_PROGS) $(PYTHON_PY_LIBS) $(PYTHON_PLUGIN)
 
 
 clean:
diff --git a/python/ctracecmd.i b/python/ctracecmd.i
index 6d0179e3..c0c0097c 100644
--- a/python/ctracecmd.i
+++ b/python/ctracecmd.i
@@ -12,9 +12,16 @@
 %apply unsigned long long *OUTPUT {unsigned long long *}
 %apply int *OUTPUT {int *}
 
+%ignore ref_count;
+%ignore locked;
+%ignore tep_print_field;
+%rename(tep_print_field) tep_print_field_content;
+
+//%feature("ref")   record "tep_record_ref($this));"
+//%feature("unref") record "tep_record_unref($this);"
+
 
 %{
-#include "trace-cmd.h"
 #include "event-parse.h"
 #include "event-utils.h"
 #include <Python.h>
@@ -204,8 +211,6 @@ static int python_callback(struct trace_seq *s,
 	PyObject *arglist, *result;
 	int r = 0;
 
-	record->ref_count++;
-
 	arglist = Py_BuildValue("(OOO)",
 		SWIG_NewPointerObj(SWIG_as_voidptr(s),
 				   SWIGTYPE_p_trace_seq, 0),
@@ -245,6 +250,5 @@ static int python_callback(struct trace_seq *s,
 #define __attribute__(x)
 #define __thread
 
-%include "trace-cmd.h"
 %include <trace-seq.h>
 %include <event-parse.h>
diff --git a/python/plugin_python_loader.c b/python/plugin_python_loader.c
new file mode 100644
index 00000000..58482795
--- /dev/null
+++ b/python/plugin_python_loader.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: LGPL-2.1
+#include <Python.h>
+#include <stdio.h>
+#include "event-parse.h"
+#include "trace-cmd-private.h"
+
+#ifndef PYTHON_DIR
+#define PYTHON_DIR "."
+#endif
+
+static const char pypath[] =
+"import sys\n"
+"sys.path.append(\"" PYTHON_DIR "\")\n";
+
+static const char pyload[] =
+"import imp, tracecmd, ctracecmd\n"
+"fn = r'%s'\n"
+"file = open(fn, 'r')\n"
+"try:\n"
+"   module = imp.load_source('%s', fn, file)\n"
+"   module.register(tracecmd.PEvent(ctracecmd.convert_pevent(pevent)))\n"
+"finally:\n"
+"   file.close()\n";
+
+static void load_plugin(struct tep_handle *pevent, const char *path,
+		       const char *name, void *data)
+{
+	PyObject *globals = data;
+	int err;
+	int len = strlen(path) + strlen(name) + 2;
+	int nlen = strlen(name) + 1;
+	char *full = malloc(len);
+	char *n = malloc(nlen);
+	char *load;
+	PyObject *res;
+
+	if (!full || !n)
+		return;
+
+	strcpy(full, path);
+	strcat(full, "/");
+	strcat(full, name);
+
+	strcpy(n, name);
+	n[nlen - 4] = '\0';
+
+	err = asprintf(&load, pyload, full, n);
+	if (err < 0)
+		return;
+
+	res = PyRun_String(load, Py_file_input, globals, globals);
+	if (!res) {
+		fprintf(stderr, "failed loading %s\n", full);
+		PyErr_Print();
+	} else
+		Py_DECREF(res);
+
+	free(load);
+
+}
+
+void set_has_plugin(struct tep_handle *tep,
+		    const char *path,
+		    const char *name,
+		    void *data)
+{
+	*(bool*) data = true;
+}
+
+int TEP_PLUGIN_LOADER(struct tep_handle *pevent)
+{
+	PyObject *globals, *m, *py_pevent, *str, *res;
+	bool has_plugin = false;
+
+	/* Only load plugins if they exist */
+	tep_load_plugins_hook(pevent, ".py", set_has_plugin, &has_plugin);
+	if (!has_plugin)
+		return 0;
+
+	Py_Initialize();
+
+	m = PyImport_AddModule("__main__");
+	globals = PyModule_GetDict(m);
+
+	res = PyRun_String(pypath, Py_file_input, globals, globals);
+	if (!res) {
+		PyErr_Print();
+		return -1;
+	} else
+		Py_DECREF(res);
+
+	str = PyUnicode_FromString("pevent");
+	if (!str)
+		return -ENOMEM;
+
+	py_pevent = PyLong_FromUnsignedLong((unsigned long)pevent);
+	if (!py_pevent)
+		return -ENOMEM;
+
+	if (PyDict_SetItem(globals, str, py_pevent))
+		fprintf(stderr, "failed to insert pevent\n");
+
+	Py_DECREF(py_pevent);
+	Py_DECREF(str);
+
+	tep_load_plugins_hook(pevent, ".py", load_plugin, globals);
+
+	return 0;
+}
+
+int TEP_PLUGIN_UNLOADER(struct tep_handle *pevent)
+{
+	Py_Finalize();
+	return 0;
+}
diff --git a/python/tracecmd.py b/python/tracecmd.py
index 4d481576..e435712c 100644
--- a/python/tracecmd.py
+++ b/python/tracecmd.py
@@ -22,6 +22,8 @@ from functools import update_wrapper
 from ctracecmd import *
 from UserDict import DictMixin
 
+__all__ = ["Event", "TraceSeq", "FieldError", "Field", "PEvent"]
+
 """
 Python interface to the tracecmd library for parsing ftrace traces
 
@@ -61,13 +63,15 @@ class Event(object, DictMixin):
         self._record = record
         self._format = format
 
+        tep_record_ref(self._record)
+
     def __str__(self):
         return "%d.%09d CPU%d %s: pid=%d comm=%s type=%d" % \
                (self.ts/1000000000, self.ts%1000000000, self.cpu, self.name,
                 self.num_field("common_pid"), self.comm, self.type)
 
     def __del__(self):
-        free_record(self._record)
+        tep_record_unref(self._record)
 
     def __getitem__(self, n):
         f = tep_find_field(self._format, n)
@@ -126,6 +130,8 @@ class TraceSeq(object):
         self._trace_seq = trace_seq
 
     def puts(self, s):
+        if type(s) != str:
+            s = s.encode('ascii')
         return trace_seq_puts(self._trace_seq, s)
 
 class FieldError(Exception):
@@ -170,86 +176,3 @@ class PEvent(object):
             return '>'
         return '<'
 
-
-class FileFormatError(Exception):
-    pass
-
-class Trace(object):
-    """
-    Trace object represents the trace file it is created with.
-
-    The Trace object aggregates the tracecmd structures and functions that are
-    used to manage the trace and extract events from it.
-    """
-    def __init__(self, filename):
-        self._handle = tracecmd_alloc(filename)
-
-        if tracecmd_read_headers(self._handle):
-            raise FileFormatError("Invalid headers")
-
-        if tracecmd_init_data(self._handle):
-            raise FileFormatError("Failed to init data")
-
-        self._pevent = tracecmd_get_pevent(self._handle)
-
-    @cached_property
-    def cpus(self):
-        return tracecmd_cpus(self._handle)
-
-    @cached_property
-    def long_size(self):
-        return tracecmd_long_size(self._handle)
-
-    def read_event(self, cpu):
-        rec = tracecmd_read_data(self._handle, cpu)
-        if rec:
-            type = tep_data_type(self._pevent, rec)
-            format = tep_find_event(self._pevent, type)
-            # rec ownership goes over to Event instance
-            return Event(self._pevent, rec, format)
-        return None
-
-    def read_event_at(self, offset):
-        res = tracecmd_read_at(self._handle, offset)
-        # SWIG only returns the CPU if the record is None for some reason
-        if isinstance(res, int):
-            return None
-        rec, cpu = res
-        type = tep_data_type(self._pevent, rec)
-        format = tep_find_event(self._pevent, type)
-        # rec ownership goes over to Event instance
-        return Event(self._pevent, rec, format)
-
-    def read_next_event(self):
-        res = tracecmd_read_next_data(self._handle)
-        if isinstance(res, int):
-            return None
-        rec, cpu = res
-        type = tep_data_type(self._pevent, rec)
-        format = tep_find_event(self._pevent, type)
-        return Event(self._pevent, rec, format)
-
-    def peek_event(self, cpu):
-        rec = tracecmd_peek_data_ref(self._handle, cpu)
-        if rec is None:
-            return None
-        type = tep_data_type(self._pevent, rec)
-        format = tep_find_event(self._pevent, type)
-        # rec ownership goes over to Event instance
-        return Event(self._pevent, rec, format)
-
-
-# Basic builtin test, execute module directly
-if __name__ == "__main__":
-    t = Trace("trace.dat")
-    print("Trace contains data for %d cpus" % (t.cpus))
-
-    for cpu in range(0, t.cpus):
-        print("CPU %d" % (cpu))
-        ev = t.read_event(cpu)
-        while ev:
-            print("\t%s" % (ev))
-            ev = t.read_event(cpu)
-
-
-
-- 
2.37.3