[PATCH 1/2] trace-cruncher: Optimize get_arg_list()

[PATCH 1/2] trace-cruncher: Optimize get_arg_list()

[Yordan Karadzhov (VMware)]

The extraction of a 'C' string from a Python list of strings gets
defined as a static method instead of being coded in the body of
'get_arg_list()'. The new static method will be used in following
patches. The error message emitted in the case of an error is
remover since it will never get printed (it is overwritten by the
error messages of the functions calling 'get_arg_list()').

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/ftracepy-utils.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/ftracepy-utils.c b/src/ftracepy-utils.c
index d8115f6..5c8bcca 100644
--- a/src/ftracepy-utils.c
+++ b/src/ftracepy-utils.c
@@ -274,10 +274,19 @@ PyObject *PyTepEvent_get_pid(PyTepEvent* self, PyObject *args,
 	return PyLong_FromLong(pid);
 }
 
+static const char *str_from_list(PyObject *py_list, int i)
+{
+	PyObject *item = PyList_GetItem(py_list, i);
+
+	if (!PyUnicode_Check(item))
+		return NULL;
+
+	return PyUnicode_DATA(item);
+}
+
 static const char **get_arg_list(PyObject *py_list)
 {
 	const char **argv = NULL;
-	PyObject *arg_py;
 	int i, n;
 
 	if (!PyList_CheckExact(py_list))
@@ -286,18 +295,14 @@ static const char **get_arg_list(PyObject *py_list)
 	n = PyList_Size(py_list);
 	argv = calloc(n + 1, sizeof(*argv));
 	for (i = 0; i < n; ++i) {
-		arg_py = PyList_GetItem(py_list, i);
-		if (!PyUnicode_Check(arg_py))
+		argv[i] = str_from_list(py_list, i);
+		if (!argv[i])
 			goto fail;
-
-		argv[i] = PyUnicode_DATA(arg_py);
 	}
 
 	return argv;
 
  fail:
-	PyErr_SetString(TRACECRUNCHER_ERROR,
-			"Failed to parse argument list.");
 	free(argv);
 	return NULL;
 }
-- 
2.30.2

[PATCH 2/2] trace-cruncher: Modify the APIs for enable/disable multiple events

[Yordan Karadzhov (VMware)]

The current version of these APIs takes the list of systems and the
'list of list' of events to enable/disable in two independent arguments.
This can potentially create confusion since the user is expected to
provide two correlated lists via disjoined arguments. Here we switch to
using a single 'events' argument for these APIs that takes a dictionary
where the 'systems' are 'keys' and the lists of events from each system
are the 'value'.

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 examples/start_tracing.py                     |  4 +-
 src/ftracepy-utils.c                          | 87 ++++++-------------
 .../tests/1_unit/test_01_ftracepy_unit.py     | 41 +++------
 3 files changed, 40 insertions(+), 92 deletions(-)

diff --git a/examples/start_tracing.py b/examples/start_tracing.py
index c9960e3..aaeb0cf 100755
--- a/examples/start_tracing.py
+++ b/examples/start_tracing.py
@@ -13,8 +13,8 @@ inst = ft.create_instance()
 
 # Enable all static events from systems "sched" and "irq".
 ft.enable_events(instance=inst,
-                 systems=['sched', 'irq'],
-                 events=[['sched_switch'],['all']])
+                 events={'sched': ['sched_switch', 'sched_waking'],
+                         'irq':   ['all']})
 
 # Print the stream of trace events. "Ctrl+c" to stop tracing.
 ft.read_trace(instance=inst)
diff --git a/src/ftracepy-utils.c b/src/ftracepy-utils.c
index 5c8bcca..d25d873 100644
--- a/src/ftracepy-utils.c
+++ b/src/ftracepy-utils.c
@@ -1161,90 +1161,59 @@ PyObject *PyFtrace_disable_event(PyObject *self, PyObject *args,
 static bool set_enable_events(PyObject *self, PyObject *args, PyObject *kwargs,
 			      bool enable)
 {
-	static char *kwlist[] = {"instance", "systems", "events", NULL};
-	PyObject *system_list = NULL, *event_list = NULL, *system_event_list;
-	const char **systems = NULL, **events = NULL;
+	static char *kwlist[] = {"events", "instance", NULL};
+	PyObject *event_dict = NULL, *py_inst = NULL;
 	struct tracefs_instance *instance;
-	PyObject *py_inst = NULL;
-	char *file = NULL;
-	int ret, s, e;
+	PyObject *py_system, *py_events;
+	const char *system, *event;
+	Py_ssize_t pos = 0;
+	int i, n;
 
 	if (!PyArg_ParseTupleAndKeywords(args,
 					 kwargs,
-					 "|OOO",
+					 "O|O",
 					 kwlist,
-					 &py_inst,
-					 &system_list,
-					 &event_list)) {
+					 &event_dict,
+					 &py_inst)) {
 		return false;
 	}
 
 	if (!get_optional_instance(py_inst, &instance))
 		return false;
 
-	if (!system_list && !event_list)
-		return event_enable_disable(instance, NULL, NULL, enable);
+	if (!PyDict_CheckExact(event_dict))
+		goto fail_with_err;
 
-	if (!system_list && event_list) {
-		if (PyUnicode_Check(event_list) &&
-		    is_all(PyUnicode_DATA(event_list))) {
-			return event_enable_disable(instance, NULL, NULL, enable);
-		} else {
-			TfsError_setstr(instance,
-					"Failed to enable events for unspecified system");
-			return false;
-		}
-	}
+	while (PyDict_Next(event_dict, &pos, &py_system, &py_events)) {
+		if (!PyUnicode_Check(py_system) ||
+		    !PyList_CheckExact(py_events))
+			goto fail_with_err;
 
-	systems = get_arg_list(system_list);
-	if (!systems) {
-		TfsError_setstr(instance, "Inconsistent \"systems\" argument.");
-		return false;
-	}
+		system = PyUnicode_DATA(py_system);
+		n = PyList_Size(py_events);
 
-	if (!event_list) {
-		for (s = 0; systems[s]; ++s) {
-			ret = event_enable_disable(instance, systems[s], NULL, enable);
-			if (ret < 0)
+		if (n == 0 || (n == 1 && is_all(str_from_list(py_events, 0)))) {
+			if (!event_enable_disable(instance, system, NULL,
+						  enable))
 				return false;
+			continue;
 		}
 
-		return true;
-	}
-
-	if (!PyList_CheckExact(event_list))
-		goto fail_with_err;
-
-	for (s = 0; systems[s]; ++s) {
-		system_event_list = PyList_GetItem(event_list, s);
-		if (!system_event_list || !PyList_CheckExact(system_event_list))
-			goto fail_with_err;
-
-		events = get_arg_list(system_event_list);
-		if (!events)
-			goto fail_with_err;
+		for (i = 0; i < n; ++i) {
+			event = str_from_list(py_events, i);
+			if (!event)
+				goto fail_with_err;
 
-		for (e = 0; events[e]; ++e) {
-			if (!event_enable_disable(instance, systems[s], events[e], enable))
-				goto fail;
+			if (!event_enable_disable(instance, system, event,
+						  enable))
+				return false;
 		}
-
-		free(events);
-		events = NULL;
 	}
 
-	free(systems);
-
 	return true;
 
  fail_with_err:
 	TfsError_setstr(instance, "Inconsistent \"events\" argument.");
-
- fail:
-	free(systems);
-	free(events);
-	free(file);
-
 	return false;
 }
 
diff --git a/tracecruncher/tests/1_unit/test_01_ftracepy_unit.py b/tracecruncher/tests/1_unit/test_01_ftracepy_unit.py
index d3e3960..b9c8fd0 100644
--- a/tracecruncher/tests/1_unit/test_01_ftracepy_unit.py
+++ b/tracecruncher/tests/1_unit/test_01_ftracepy_unit.py
@@ -270,20 +270,8 @@ class EventsTestCase(unittest.TestCase):
     def test_enable_events(self):
         inst = ft.create_instance(instance_name)
         ft.enable_events(instance=inst,
-                         events='all')
-
-        ret = ft.event_is_enabled(instance=inst,
-                                  event='all')
-        self.assertEqual(ret, '1')
-        ft.disable_events(instance=inst,
-                          events='all')
-
-        ret = ft.event_is_enabled(instance=inst,
-                                  event='all')
-        self.assertEqual(ret, '0')
-
-        ft.enable_events(instance=inst,
-                         systems=['sched', 'irq'])
+                         events={'sched': ['all'],
+                                 'irq': ['all']})
 
         ret = ft.event_is_enabled(instance=inst,
                                   system='sched',
@@ -296,7 +284,8 @@ class EventsTestCase(unittest.TestCase):
         self.assertEqual(ret, '1')
 
         ft.disable_events(instance=inst,
-                          systems=['sched', 'irq'])
+                          events={'sched': ['all'],
+                                  'irq': ['all']})
 
         ret = ft.event_is_enabled(instance=inst,
                                   system='sched',
@@ -309,9 +298,8 @@ class EventsTestCase(unittest.TestCase):
         self.assertEqual(ret, '0')
 
         ft.enable_events(instance=inst,
-                         systems=['sched', 'irq'],
-                         events=[['sched_switch', 'sched_waking'],
-                                 ['all']])
+                         events={'sched': ['sched_switch', 'sched_waking'],
+                                 'irq':   ['all']})
 
         ret = ft.event_is_enabled(instance=inst,
                                   system='sched',
@@ -334,9 +322,8 @@ class EventsTestCase(unittest.TestCase):
         self.assertEqual(ret, '1')
 
         ft.disable_events(instance=inst,
-                          systems=['sched', 'irq'],
-                          events=[['sched_switch', 'sched_waking'],
-                                  ['all']])
+                          events={'sched': ['sched_switch', 'sched_waking'],
+                                  'irq':   ['all']})
 
         ret = ft.event_is_enabled(instance=inst,
                                   system='sched',
@@ -359,21 +346,13 @@ class EventsTestCase(unittest.TestCase):
         err = 'Inconsistent \"events\" argument'
         with self.assertRaises(Exception) as context:
             ft.enable_events(instance=inst,
-                             systems=['sched'],
-                             events=['all'])
-        self.assertTrue(err in str(context.exception))
-
-        err = 'Failed to enable events for unspecified system'
-        with self.assertRaises(Exception) as context:
-            ft.enable_events(instance=inst,
-                             events=['sched_switch', 'sched_wakeup'])
+                             events='all')
         self.assertTrue(err in str(context.exception))
 
         err = 'Failed to enable/disable event'
         with self.assertRaises(Exception) as context:
             ft.enable_events(instance=inst,
-                             systems=['sched'],
-                             events=[['no_event']])
+                             events={'sched': ['no_event']})
         self.assertTrue(err in str(context.exception))
 
 
-- 
2.30.2