Hi,
this is an attempt to get rid of duplicate code in satyr python bindings. I
tried to reuse the code for a function that is currently repeated several
times. This is accomplished by treating all pointers that can have different
types as void* and accessing structure members through pointer arithmetic.
As a consequence it bypasses the type checking and thus is somewhat difficult
to debug. I'm starting to think that writing the function as a preprocessor
macro might be a better approach. I'll be grateful for any advice.
-->8-->8-->8-->8-->8-->8-->8-->8-->8-->8-->8-->8--
The code that performs C linked list reconstruction from python list
object was consolidated into single function which treats all pointers
as void* in order to work with different (yet structurally similar)
structures.
Signed-off-by: Martin Milata <mmilata(a)redhat.com>
---
python/Makefile.am | 2 +
python/py_common.c | 78 ++++++++++++++++++++++++++++++
python/py_common.h | 109 ++++++++++++++++++++++++++++++++++++++++++
python/py_gdb_stacktrace.c | 100 ++++++++++----------------------------
python/py_gdb_thread.c | 48 +++++--------------
python/py_java_stacktrace.c | 51 ++++----------------
python/py_java_thread.c | 47 ++++--------------
python/py_koops_stacktrace.c | 49 +++++--------------
python/py_python_stacktrace.c | 47 ++++--------------
tests/python/metrics.py | 9 ++++
10 files changed, 276 insertions(+), 264 deletions(-)
create mode 100644 python/py_common.c
create mode 100644 python/py_common.h
diff --git a/python/Makefile.am b/python/Makefile.am
index d72012a..a710e18 100644
--- a/python/Makefile.am
+++ b/python/Makefile.am
@@ -32,6 +32,8 @@ _satyr_la_SOURCES = \
py_java_stacktrace.c \
py_metrics.h \
py_metrics.c \
+ py_common.h \
+ py_common.c \
py_module.c
_satyr_la_CPPFLAGS = \
diff --git a/python/py_common.c b/python/py_common.c
new file mode 100644
index 0000000..f35cfd4
--- /dev/null
+++ b/python/py_common.c
@@ -0,0 +1,78 @@
+/*
+ py_common.c
+
+ Copyright (C) 2013 Red Hat, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program; if not, write to the Free Software Foundation, Inc.,
+ 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+
+#include "py_common.h"
+
+int
+prepare_linked_list(void *owner, PyTypeObject *inferior_type, const char
*owner_pylist_name,
+ size_t owner_pylist_off, size_t owner_payload_off, size_t payload_list_off,
+ size_t inferior_payload_off, size_t next_off)
+{
+ int i;
+ PyObject *item;
+ void *current = NULL, *prev = NULL;
+
+ /* XXX: check that MEMB(owner, owner_pylist_off) is indeed list */
+
+ for (i = 0; i < PyList_Size(MEMB(owner, owner_pylist_off)); ++i)
+ {
+ item = PyList_GetItem(MEMB(owner, owner_pylist_off), i);
+ if (!item)
+ return -1;
+
+ Py_INCREF(item);
+
+ if (!PyObject_TypeCheck(item, inferior_type))
+ {
+ char error_msg[256];
+ Py_XDECREF(item);
+ Py_XDECREF(prev);
+
+ PyOS_snprintf(error_msg, sizeof(error_msg), "%s must be a list of %s
objects",
+ owner_pylist_name, inferior_type->tp_name);
+ PyErr_SetString(PyExc_TypeError, error_msg);
+ return -1;
+ }
+
+ current = item;
+ if (i == 0)
+ {
+ /* owner->owner_payload->payload_list = current->inferior_payload
*/
+ MEMB(MEMB(owner, owner_payload_off), payload_list_off)
+ = MEMB(current, inferior_payload_off);
+ }
+ else
+ /* prev->inferior_payload->next = current->inferior_payload */
+ MEMB(MEMB(prev, inferior_payload_off), next_off)
+ = MEMB(current, inferior_payload_off);
+
+ Py_XDECREF(prev);
+ prev = current;
+ }
+
+ if (current)
+ {
+ //current->inferior_c_struct->next = NULL;
+ MEMB(MEMB(current, inferior_payload_off), next_off) = NULL;
+ Py_DECREF(current);
+ }
+
+ return 0;
+}
diff --git a/python/py_common.h b/python/py_common.h
new file mode 100644
index 0000000..284b96c
--- /dev/null
+++ b/python/py_common.h
@@ -0,0 +1,109 @@
+/*
+ py_common.h
+
+ Copyright (C) 2013 Red Hat, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program; if not, write to the Free Software Foundation, Inc.,
+ 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+#ifndef SATYR_PY_COMMON_H
+#define SATYR_PY_COMMON_H
+
+/**
+ * @file
+ * @brief Common routines for python bindings.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <Python.h>
+#include <structmember.h>
+
+/*
+ * MEMB(st, offsetof(typeof(st), member)) == (void*)(st->member)
+ */
+#define MEMB(st, off) *(void**)((char*)st + off)
+
+/**
+ * @brief Creates C linked list that can be used in a function from its python
+ * representation.
+ *
+ * Satyr uses linked lists heavily, and their python representation follows a
+ * common pattern. When a C structure contains a linked list, the python API
+ * exposes it as a python list. The python list contains references to
+ * individual elements and takes care of freeing them after no references point
+ * to them. When we need to call a C function on such object, we build a
+ * C-style linked list from the python list by updating the next pointers to
+ * reflect the ordering in the python list. This function does the updating.
+ * There are four kinds of objects involved:
+ *
+ * owner: Owner is the python object that wraps the satyr object and exposes
+ * the list contained in that satyr object as a python list. It contains
+ * reference to the C object (payload), and python list of the items contained
+ * in payload.
+ *
+ * inferior: Inferior is python object that wraps the individual elements in
+ * the list.
+ *
+ * payload: Payload is the C structure that is wrapped by the owner object.
+ * Inferior also wraps a python object, which we'll call inferior-payload.
+ *
+ * Take GDB thread as an example. In this context, struct sr_py_gdb_thread is
+ * the owner -- the main object we care about. It is a wrapper around struct
+ * sr_gdb_thread, which is the payload. Threads are sequences of frames and
+ * we'll refer to the python object containing a frame, struct sr_py_gdb_frame,
+ * as an inferior. And last, struct sr_gdb_frame, is the inferior-payload.
+ *
+ * @param owner The main python object for which we'll be reconstructing the
+ * linked list.
+ *
+ * @param inferior_type Pointer to the python type object of the inferiors.
+ * E.g. &sr_py_gdb_frame_type for GDB thread (note the &).
+ *
+ * @param owner_pylist_name Name of the attribute containing the python list.
+ * Used for error message.
+ *
+ * @param owner_pylist_off Offset of the python list in the owner structure.
+ * E.g. offsetof(struct sr_py_gdb_thread, frames) for GDB thread.
+ *
+ * @param owner_payload_off Offset of the payload in the owner structure.
+ * E.g. offsetof(struct sr_py_gdb_thread, thread) for GDB thread.
+ *
+ * @param payload_list_off Offset of the list pointer in the payload structure.
+ * E.g. offsetof(struct sr_gdb_thread, frames) for GDB thread.
+ *
+ * @param inferior_payload_off Offset of the payload pointer in the inferior
+ * python object. E.g. offsetof(struct sr_py_gdb_frame, frame) for GDB thread.
+ *
+ * @param next_off Offset of the next pointer in the inferior-payload C
+ * structure. E.g. offsetof(struct sr_gdb_frame, next) for GDB thread.
+ *
+ * @returns 0 on success, -1 on error
+ */
+int prepare_linked_list(void *owner,
+ PyTypeObject *inferior_type,
+ const char *owner_pylist_name,
+ size_t owner_pylist_off,
+ size_t owner_payload_off,
+ size_t payload_list_off,
+ size_t inferior_payload_off,
+ size_t next_off);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* SATYR_PY_COMMON_H */
diff --git a/python/py_gdb_stacktrace.c b/python/py_gdb_stacktrace.c
index f16965b..3126b38 100644
--- a/python/py_gdb_stacktrace.c
+++ b/python/py_gdb_stacktrace.c
@@ -2,6 +2,7 @@
#include "py_gdb_thread.h"
#include "py_gdb_frame.h"
#include "py_gdb_sharedlib.h"
+#include "py_common.h"
#include "lib/strbuf.h"
#include "lib/gdb_stacktrace.h"
#include "lib/gdb_thread.h"
@@ -155,81 +156,30 @@ PyTypeObject sr_py_gdb_stacktrace_type = {
int
stacktrace_prepare_linked_list(struct sr_py_gdb_stacktrace *stacktrace)
{
- int i;
- PyObject *item;
-
- /* thread */
- struct sr_py_gdb_thread *current = NULL, *prev = NULL;
- for (i = 0; i < PyList_Size(stacktrace->threads); ++i)
- {
- item = PyList_GetItem(stacktrace->threads, i);
- if (!item)
- return -1;
-
- Py_INCREF(item);
- if (!PyObject_TypeCheck(item, &sr_py_gdb_thread_type))
- {
- Py_XDECREF(current);
- Py_XDECREF(prev);
- PyErr_SetString(PyExc_TypeError,
- "threads must be a list of satyr.Thread objects");
- return -1;
- }
-
- current = (struct sr_py_gdb_thread*)item;
- if (thread_prepare_linked_list(current) < 0)
- return -1;
-
- if (i == 0)
- stacktrace->stacktrace->threads = current->thread;
- else
- prev->thread->next = current->thread;
-
- Py_XDECREF(prev);
- prev = current;
- }
-
- if (current)
- {
- current->thread->next = NULL;
- Py_XDECREF(current);
- }
-
- /* sharedlib */
- struct sr_py_gdb_sharedlib *currentlib = NULL, *prevlib = NULL;
- for (i = 0; i < PyList_Size(stacktrace->libs); ++i)
- {
- item = PyList_GetItem(stacktrace->libs, i);
- if (!item)
- return -1;
-
- Py_INCREF(item);
- if (!PyObject_TypeCheck(item, &sr_py_gdb_sharedlib_type))
- {
- Py_XDECREF(currentlib);
- Py_XDECREF(prevlib);
- PyErr_SetString(PyExc_TypeError,
- "libs must be a list of satyr.Sharedlib objects");
- return -1;
- }
-
- currentlib = (struct sr_py_gdb_sharedlib*)item;
- if (i == 0)
- stacktrace->stacktrace->libs = currentlib->sharedlib;
- else
- prevlib->sharedlib->next = currentlib->sharedlib;
-
- Py_XDECREF(prevlib);
- prevlib = currentlib;
- }
-
- if (currentlib)
- {
- currentlib->sharedlib->next = NULL;
- Py_XDECREF(currentlib);
- }
-
- return 0;
+ int res = prepare_linked_list(
+ stacktrace,
+ &sr_py_gdb_thread_type,
+ "threads",
+ offsetof(struct sr_py_gdb_stacktrace, threads),
+ offsetof(struct sr_py_gdb_stacktrace, stacktrace),
+ offsetof(struct sr_gdb_stacktrace, threads),
+ offsetof(struct sr_py_gdb_thread, thread),
+ offsetof(struct sr_gdb_thread, next)
+ );
+
+ if (res < 0)
+ return res;
+
+ return prepare_linked_list(
+ stacktrace,
+ &sr_py_gdb_sharedlib_type,
+ "libs",
+ offsetof(struct sr_py_gdb_stacktrace, libs),
+ offsetof(struct sr_py_gdb_stacktrace, stacktrace),
+ offsetof(struct sr_gdb_stacktrace, libs),
+ offsetof(struct sr_py_gdb_sharedlib, sharedlib),
+ offsetof(struct sr_gdb_sharedlib, next)
+ );
}
int
diff --git a/python/py_gdb_thread.c b/python/py_gdb_thread.c
index c5098bb..bbbffc1 100644
--- a/python/py_gdb_thread.c
+++ b/python/py_gdb_thread.c
@@ -1,5 +1,6 @@
#include "py_gdb_thread.h"
#include "py_gdb_frame.h"
+#include "py_common.h"
#include "lib/strbuf.h"
#include "lib/gdb_thread.h"
#include "lib/gdb_frame.h"
@@ -121,43 +122,16 @@ PyTypeObject sr_py_gdb_thread_type =
int
thread_prepare_linked_list(struct sr_py_gdb_thread *thread)
{
- int i;
- PyObject *item;
- struct sr_py_gdb_frame *current = NULL, *prev = NULL;
-
- for (i = 0; i < PyList_Size(thread->frames); ++i)
- {
- item = PyList_GetItem(thread->frames, i);
- if (!item)
- return -1;
-
- Py_INCREF(item);
-
- if (!PyObject_TypeCheck(item, &sr_py_gdb_frame_type))
- {
- Py_XDECREF(item);
- Py_XDECREF(prev);
- PyErr_SetString(PyExc_TypeError, "frames must be a list of satyr.Frame
objects");
- return -1;
- }
-
- current = (struct sr_py_gdb_frame*)item;
- if (i == 0)
- thread->thread->frames = current->frame;
- else
- prev->frame->next = current->frame;
-
- Py_XDECREF(prev);
- prev = current;
- }
-
- if (current)
- {
- current->frame->next = NULL;
- Py_DECREF(current);
- }
-
- return 0;
+ return prepare_linked_list(
+ thread, /* owner */
+ &sr_py_gdb_frame_type, /*inferior_type*/
+ "frames", /* owner_pylist_name */
+ offsetof(struct sr_py_gdb_thread, frames), /* owner_pylist_off */
+ offsetof(struct sr_py_gdb_thread, thread), /* owner_payload_off */
+ offsetof(struct sr_gdb_thread, frames), /* payload_list_off */
+ offsetof(struct sr_py_gdb_frame, frame), /* inferior_payload_off */
+ offsetof(struct sr_gdb_frame, next) /* next_off */
+ );
}
int
diff --git a/python/py_java_stacktrace.c b/python/py_java_stacktrace.c
index ed91088..b15da77 100644
--- a/python/py_java_stacktrace.c
+++ b/python/py_java_stacktrace.c
@@ -92,47 +92,16 @@ PyTypeObject sr_py_java_stacktrace_type = {
int
java_stacktrace_prepare_linked_list(struct sr_py_java_stacktrace *stacktrace)
{
- int i;
- PyObject *item;
-
- /* thread */
- struct sr_py_java_thread *current = NULL, *prev = NULL;
- for (i = 0; i < PyList_Size(stacktrace->threads); ++i)
- {
- item = PyList_GetItem(stacktrace->threads, i);
- if (!item)
- return -1;
-
- Py_INCREF(item);
- if (!PyObject_TypeCheck(item, &sr_py_java_thread_type))
- {
- Py_XDECREF(current);
- Py_XDECREF(prev);
- PyErr_SetString(PyExc_TypeError,
- "threads must be a list of satyr.JavaThread
objects");
- return -1;
- }
-
- current = (struct sr_py_java_thread*)item;
- if (java_thread_prepare_linked_list(current) < 0)
- return -1;
-
- if (i == 0)
- stacktrace->stacktrace->threads = current->thread;
- else
- prev->thread->next = current->thread;
-
- Py_XDECREF(prev);
- prev = current;
- }
-
- if (current)
- {
- current->thread->next = NULL;
- Py_XDECREF(current);
- }
-
- return 0;
+ return prepare_linked_list(
+ stacktrace,
+ &sr_py_java_thread_type,
+ "threads",
+ offsetof(struct sr_py_java_stacktrace, threads),
+ offsetof(struct sr_py_java_stacktrace, stacktrace),
+ offsetof(struct sr_java_stacktrace, threads),
+ offsetof(struct sr_py_java_thread, thread),
+ offsetof(struct sr_java_thread, next)
+ );
}
int
diff --git a/python/py_java_thread.c b/python/py_java_thread.c
index 4416918..d15e8ba 100644
--- a/python/py_java_thread.c
+++ b/python/py_java_thread.c
@@ -122,43 +122,16 @@ PyTypeObject sr_py_java_thread_type =
int
java_thread_prepare_linked_list(struct sr_py_java_thread *thread)
{
- int i;
- PyObject *item;
- struct sr_py_java_frame *current = NULL, *prev = NULL;
-
- for (i = 0; i < PyList_Size(thread->frames); ++i)
- {
- item = PyList_GetItem(thread->frames, i);
- if (!item)
- return -1;
-
- Py_INCREF(item);
-
- if (!PyObject_TypeCheck(item, &sr_py_java_frame_type))
- {
- Py_XDECREF(item);
- Py_XDECREF(prev);
- PyErr_SetString(PyExc_TypeError, "frames must be a list of
satyr.JavaFrame objects");
- return -1;
- }
-
- current = (struct sr_py_java_frame*)item;
- if (i == 0)
- thread->thread->frames = current->frame;
- else
- prev->frame->next = current->frame;
-
- Py_XDECREF(prev);
- prev = current;
- }
-
- if (current)
- {
- current->frame->next = NULL;
- Py_DECREF(current);
- }
-
- return 0;
+ return prepare_linked_list(
+ thread,
+ &sr_py_java_frame_type,
+ "frames",
+ offsetof(struct sr_py_java_thread, frames),
+ offsetof(struct sr_py_java_thread, thread),
+ offsetof(struct sr_java_thread, frames),
+ offsetof(struct sr_py_java_frame, frame),
+ offsetof(struct sr_java_frame, next)
+ );
}
int
diff --git a/python/py_koops_stacktrace.c b/python/py_koops_stacktrace.c
index 345664e..9fe603a 100644
--- a/python/py_koops_stacktrace.c
+++ b/python/py_koops_stacktrace.c
@@ -1,5 +1,6 @@
#include "py_koops_frame.h"
#include "py_koops_stacktrace.h"
+#include "py_common.h"
#include "lib/strbuf.h"
#include "lib/koops_frame.h"
#include "lib/koops_stacktrace.h"
@@ -91,47 +92,21 @@ PyTypeObject sr_py_koops_stacktrace_type = {
NULL, /* tp_weaklist */
};
+
/* helpers */
int
koops_stacktrace_prepare_linked_list(struct sr_py_koops_stacktrace *stacktrace)
{
- int i;
- PyObject *item;
-
- struct sr_py_koops_frame *current = NULL, *prev = NULL;
- for (i = 0; i < PyList_Size(stacktrace->frames); ++i)
- {
- item = PyList_GetItem(stacktrace->frames, i);
- if (!item)
- return -1;
-
- Py_INCREF(item);
- if (!PyObject_TypeCheck(item, &sr_py_koops_frame_type))
- {
- Py_XDECREF(current);
- Py_XDECREF(prev);
- PyErr_SetString(PyExc_TypeError,
- "frames must be a list of satyr.KerneloopsFrame
objects");
- return -1;
- }
-
- current = (struct sr_py_koops_frame*)item;
- if (i == 0)
- stacktrace->stacktrace->frames = current->frame;
- else
- prev->frame->next = current->frame;
-
- Py_XDECREF(prev);
- prev = current;
- }
-
- if (current)
- {
- current->frame->next = NULL;
- Py_XDECREF(current);
- }
-
- return 0;
+ return prepare_linked_list(
+ stacktrace,
+ &sr_py_koops_frame_type,
+ "frames",
+ offsetof(struct sr_py_koops_stacktrace, frames),
+ offsetof(struct sr_py_koops_stacktrace, stacktrace),
+ offsetof(struct sr_koops_stacktrace, frames),
+ offsetof(struct sr_py_koops_frame, frame),
+ offsetof(struct sr_koops_frame, next)
+ );
}
PyObject *
diff --git a/python/py_python_stacktrace.c b/python/py_python_stacktrace.c
index 5ad3ac0..3e908ba 100644
--- a/python/py_python_stacktrace.c
+++ b/python/py_python_stacktrace.c
@@ -119,43 +119,16 @@ PyTypeObject sr_py_python_stacktrace_type = {
int
python_stacktrace_prepare_linked_list(struct sr_py_python_stacktrace *stacktrace)
{
- int i;
- PyObject *item;
-
- struct sr_py_python_frame *current = NULL, *prev = NULL;
- for (i = 0; i < PyList_Size(stacktrace->frames); ++i)
- {
- item = PyList_GetItem(stacktrace->frames, i);
- if (!item)
- return -1;
-
- Py_INCREF(item);
- if (!PyObject_TypeCheck(item, &sr_py_python_frame_type))
- {
- Py_XDECREF(current);
- Py_XDECREF(prev);
- PyErr_SetString(PyExc_TypeError,
- "frames must be a list of satyr.PythonFrame
objects");
- return -1;
- }
-
- current = (struct sr_py_python_frame*)item;
- if (i == 0)
- stacktrace->stacktrace->frames = current->frame;
- else
- prev->frame->next = current->frame;
-
- Py_XDECREF(prev);
- prev = current;
- }
-
- if (current)
- {
- current->frame->next = NULL;
- Py_XDECREF(current);
- }
-
- return 0;
+ return prepare_linked_list(
+ stacktrace,
+ &sr_py_python_frame_type,
+ "frames",
+ offsetof(struct sr_py_python_stacktrace, frames),
+ offsetof(struct sr_py_python_stacktrace, stacktrace),
+ offsetof(struct sr_python_stacktrace, frames),
+ offsetof(struct sr_py_python_frame, frame),
+ offsetof(struct sr_python_frame, next)
+ );
}
PyObject *
diff --git a/tests/python/metrics.py b/tests/python/metrics.py
index a97e5eb..3d11920 100755
--- a/tests/python/metrics.py
+++ b/tests/python/metrics.py
@@ -43,5 +43,14 @@ class TestDistances(unittest.TestCase):
distances = satyr.Distances("levenshtein", [thread1, thread2], 2)
self.assertAlmostEqual(distances.get_distance(0, 1), 0.0)
+ # we should get a TypeError if we replace frames with garbage
+ thread1.frames = ['WAT?!!!']
+ self.assertRaises(TypeError, satyr.Distances, "levenshtein", [thread1,
thread2], 2)
+
+ ## what if frames is not list?
+ #thread1.frames = 42
+ #thread2.frames = None
+ #distances = satyr.Distances("levenshtein", [thread1, thread2], 2)
+
if __name__ == '__main__':
unittest.main()
--
1.7.11.7