From f3d6b4c2f7592fde9ff770989b25bacaa4820dc8 Mon Sep 17 00:00:00 2001 From: Philippe Proulx Date: Wed, 11 Dec 2019 16:50:21 -0500 Subject: [PATCH] lib: graph API: remove "listener removed" callback parameters Before this patch, when you add a "port added" listener to a graph, you can pass a callback which gets called when the listener is removed. This only happens when the graph is destroyed. This "listener removed" callback feature seems unnecessary as the graph user, who adds the "port added" listener, has total control over the graph object. Therefore she can ensure that anything needed by her "port added" listeners exists as long as the graph exists. Therefore this patch removes those parameters and everything related. In Python, we used to keep a strong reference on the partial Python object (listener's data) and release it when our "listener removed" function was called. Now, the listener's data is a weak reference, but we keep a list of strong partial references within the graph object itself (`self._listener_partials`) to ensure that the partials exist as long as the graph exists. Signed-off-by: Philippe Proulx Change-Id: I4c06ff139740f887ae2ace7633d2edeb01fd2fa0 Reviewed-on: https://review.lttng.org/c/babeltrace/+/2637 Tested-by: jenkins --- include/babeltrace2/graph/graph.h | 14 +--- src/bindings/python/bt2/bt2/graph.py | 7 ++ .../python/bt2/bt2/native_bt_graph.i.h | 21 +---- src/cli/babeltrace2.c | 4 +- src/lib/graph/graph.c | 84 +++---------------- src/lib/graph/graph.h | 1 - tests/lib/test_graph_topo.c | 4 +- 7 files changed, 29 insertions(+), 106 deletions(-) diff --git a/include/babeltrace2/graph/graph.h b/include/babeltrace2/graph/graph.h index f66b9538..0a7484e4 100644 --- a/include/babeltrace2/graph/graph.h +++ b/include/babeltrace2/graph/graph.h @@ -89,8 +89,6 @@ typedef bt_graph_listener_func_status const bt_port_output *upstream_port, const bt_port_input *downstream_port, void *data); -typedef void (* bt_graph_listener_removed_func)(void *data); - typedef enum bt_graph_simple_sink_component_initialize_func_status { BT_GRAPH_SIMPLE_SINK_COMPONENT_INITIALIZE_FUNC_STATUS_OK = __BT_FUNC_STATUS_OK, BT_GRAPH_SIMPLE_SINK_COMPONENT_INITIALIZE_FUNC_STATUS_ERROR = __BT_FUNC_STATUS_ERROR, @@ -214,29 +212,25 @@ extern bt_graph_add_listener_status bt_graph_add_filter_component_input_port_added_listener( bt_graph *graph, bt_graph_filter_component_input_port_added_listener_func listener, - bt_graph_listener_removed_func listener_removed, void *data, - bt_listener_id *listener_id); + void *data, bt_listener_id *listener_id); extern bt_graph_add_listener_status bt_graph_add_sink_component_input_port_added_listener( bt_graph *graph, bt_graph_sink_component_input_port_added_listener_func listener, - bt_graph_listener_removed_func listener_removed, void *data, - bt_listener_id *listener_id); + void *data, bt_listener_id *listener_id); extern bt_graph_add_listener_status bt_graph_add_source_component_output_port_added_listener( bt_graph *graph, bt_graph_source_component_output_port_added_listener_func listener, - bt_graph_listener_removed_func listener_removed, void *data, - bt_listener_id *listener_id); + void *data, bt_listener_id *listener_id); extern bt_graph_add_listener_status bt_graph_add_filter_component_output_port_added_listener( bt_graph *graph, bt_graph_filter_component_output_port_added_listener_func listener, - bt_graph_listener_removed_func listener_removed, void *data, - bt_listener_id *listener_id); + void *data, bt_listener_id *listener_id); typedef enum bt_graph_add_interrupter_status { BT_GRAPH_ADD_INTERRUPTER_STATUS_OK = __BT_FUNC_STATUS_OK, diff --git a/src/bindings/python/bt2/bt2/graph.py b/src/bindings/python/bt2/bt2/graph.py index b1d59700..a3926e59 100644 --- a/src/bindings/python/bt2/bt2/graph.py +++ b/src/bindings/python/bt2/bt2/graph.py @@ -57,6 +57,10 @@ class Graph(object._SharedObject): super().__init__(ptr) + # list of listener partials to keep a reference as long as + # this graph exists + self._listener_partials = [] + def add_component( self, component_class, @@ -140,6 +144,9 @@ class Graph(object._SharedObject): if listener_ids is None: raise bt2._Error('cannot add listener to graph object') + # keep the partial's reference + self._listener_partials.append(listener_from_native) + def run_once(self): status = native_bt.graph_run_once(self._ptr) utils._handle_func_status(status, 'graph object could not run once') diff --git a/src/bindings/python/bt2/bt2/native_bt_graph.i.h b/src/bindings/python/bt2/bt2/native_bt_graph.i.h index e45fea61..74684b82 100644 --- a/src/bindings/python/bt2/bt2/native_bt_graph.i.h +++ b/src/bindings/python/bt2/bt2/native_bt_graph.i.h @@ -22,13 +22,6 @@ * THE SOFTWARE. */ -static -void graph_listener_removed(void *py_callable) -{ - BT_ASSERT(py_callable); - Py_DECREF(py_callable); -} - static bt_graph_listener_func_status port_added_listener( const void *component, swig_type_info *component_swig_type, @@ -145,7 +138,7 @@ PyObject *bt_bt2_graph_add_port_added_listener(struct bt_graph *graph, /* source output port */ status = bt_graph_add_source_component_output_port_added_listener( graph, source_component_output_port_added_listener, - graph_listener_removed, py_callable, &listener_id); + py_callable, &listener_id); if (status != __BT_FUNC_STATUS_OK) { /* * bt_graph_add_source_component_output_port_added_listener has @@ -167,7 +160,7 @@ PyObject *bt_bt2_graph_add_port_added_listener(struct bt_graph *graph, /* filter input port */ status = bt_graph_add_filter_component_input_port_added_listener( graph, filter_component_input_port_added_listener, - graph_listener_removed, py_callable, &listener_id); + py_callable, &listener_id); if (status != __BT_FUNC_STATUS_OK) { /* * bt_graph_add_filter_component_input_port_added_listener has @@ -189,7 +182,7 @@ PyObject *bt_bt2_graph_add_port_added_listener(struct bt_graph *graph, /* filter output port */ status = bt_graph_add_filter_component_output_port_added_listener( graph, filter_component_output_port_added_listener, - graph_listener_removed, py_callable, &listener_id); + py_callable, &listener_id); if (status != __BT_FUNC_STATUS_OK) { /* * bt_graph_add_filter_component_output_port_added_listener has @@ -211,7 +204,7 @@ PyObject *bt_bt2_graph_add_port_added_listener(struct bt_graph *graph, /* sink input port */ status = bt_graph_add_sink_component_input_port_added_listener( graph, sink_component_input_port_added_listener, - graph_listener_removed, py_callable, &listener_id); + py_callable, &listener_id); if (status != __BT_FUNC_STATUS_OK) { /* * bt_graph_add_sink_component_input_port_added_listener has @@ -229,12 +222,6 @@ PyObject *bt_bt2_graph_add_port_added_listener(struct bt_graph *graph, PyTuple_SET_ITEM(py_listener_ids, 3, py_listener_id); py_listener_id = NULL; - - Py_INCREF(py_callable); - Py_INCREF(py_callable); - Py_INCREF(py_callable); - Py_INCREF(py_callable); - goto end; error: diff --git a/src/cli/babeltrace2.c b/src/cli/babeltrace2.c index ad211fa2..ad8ec4b7 100644 --- a/src/cli/babeltrace2.c +++ b/src/cli/babeltrace2.c @@ -1899,7 +1899,7 @@ int cmd_run_ctx_init(struct cmd_run_ctx *ctx, struct bt_config *cfg) bt_graph_add_interrupter(ctx->graph, the_interrupter); add_listener_status = bt_graph_add_source_component_output_port_added_listener( - ctx->graph, graph_source_output_port_added_listener, NULL, ctx, + ctx->graph, graph_source_output_port_added_listener, ctx, NULL); if (add_listener_status != BT_GRAPH_ADD_LISTENER_STATUS_OK) { BT_CLI_LOGE_APPEND_CAUSE( @@ -1908,7 +1908,7 @@ int cmd_run_ctx_init(struct cmd_run_ctx *ctx, struct bt_config *cfg) } add_listener_status = bt_graph_add_filter_component_output_port_added_listener( - ctx->graph, graph_filter_output_port_added_listener, NULL, ctx, + ctx->graph, graph_filter_output_port_added_listener, ctx, NULL); if (add_listener_status != BT_GRAPH_ADD_LISTENER_STATUS_OK) { BT_CLI_LOGE_APPEND_CAUSE( diff --git a/src/lib/graph/graph.c b/src/lib/graph/graph.c index 8416b1f6..70d00c3d 100644 --- a/src/lib/graph/graph.c +++ b/src/lib/graph/graph.c @@ -58,14 +58,9 @@ typedef enum bt_graph_listener_func_status typedef enum bt_component_class_initialize_method_status (*comp_init_method_t)(const void *, void *, const void *, void *); -struct bt_graph_listener { - bt_graph_listener_removed_func removed; - void *data; -}; - struct bt_graph_listener_port_added { - struct bt_graph_listener base; port_added_func_t func; + void *data; }; #define INIT_LISTENERS_ARRAY(_type, _listeners) \ @@ -77,23 +72,6 @@ struct bt_graph_listener_port_added { } \ } while (0) -#define CALL_REMOVE_LISTENERS(_type, _listeners) \ - do { \ - size_t i; \ - \ - if (!_listeners) { \ - break; \ - } \ - for (i = 0; i < (_listeners)->len; i++) { \ - _type *listener = \ - &g_array_index((_listeners), _type, i); \ - \ - if (listener->base.removed) { \ - listener->base.removed(listener->base.data); \ - } \ - } \ - } while (0) - static void destroy_graph(struct bt_object *obj) { @@ -129,16 +107,6 @@ void destroy_graph(struct bt_object *obj) obj->ref_count++; graph->config_state = BT_GRAPH_CONFIGURATION_STATE_DESTROYING; - /* Call all remove listeners */ - CALL_REMOVE_LISTENERS(struct bt_graph_listener_port_added, - graph->listeners.source_output_port_added); - CALL_REMOVE_LISTENERS(struct bt_graph_listener_port_added, - graph->listeners.filter_output_port_added); - CALL_REMOVE_LISTENERS(struct bt_graph_listener_port_added, - graph->listeners.filter_input_port_added); - CALL_REMOVE_LISTENERS(struct bt_graph_listener_port_added, - graph->listeners.sink_input_port_added); - if (graph->messages) { g_ptr_array_free(graph->messages, TRUE); graph->messages = NULL; @@ -710,25 +678,17 @@ enum bt_graph_add_listener_status bt_graph_add_source_component_output_port_added_listener( struct bt_graph *graph, bt_graph_source_component_output_port_added_listener_func func, - bt_graph_listener_removed_func listener_removed, void *data, - bt_listener_id *out_listener_id) + void *data, bt_listener_id *out_listener_id) { struct bt_graph_listener_port_added listener = { - .base = { - .removed = listener_removed, - .data = data, - }, .func = (port_added_func_t) func, + .data = data, }; bt_listener_id listener_id; BT_ASSERT_PRE_NO_ERROR(); BT_ASSERT_PRE_NON_NULL(graph, "Graph"); BT_ASSERT_PRE_NON_NULL(func, "Listener"); - BT_ASSERT_PRE_NON_NULL(func, "\"Listener removed\" listener"); - BT_ASSERT_PRE(!graph->in_remove_listener, - "Graph currently executing a \"listener removed\" listener: " - "%!+g", graph); g_array_append_val(graph->listeners.source_output_port_added, listener); listener_id = graph->listeners.source_output_port_added->len - 1; BT_LIB_LOGD("Added \"source component output port added\" listener to graph: " @@ -746,25 +706,17 @@ enum bt_graph_add_listener_status bt_graph_add_filter_component_output_port_added_listener( struct bt_graph *graph, bt_graph_filter_component_output_port_added_listener_func func, - bt_graph_listener_removed_func listener_removed, void *data, - bt_listener_id *out_listener_id) + void *data, bt_listener_id *out_listener_id) { struct bt_graph_listener_port_added listener = { - .base = { - .removed = listener_removed, - .data = data, - }, .func = (port_added_func_t) func, + .data = data, }; bt_listener_id listener_id; BT_ASSERT_PRE_NO_ERROR(); BT_ASSERT_PRE_NON_NULL(graph, "Graph"); BT_ASSERT_PRE_NON_NULL(func, "Listener"); - BT_ASSERT_PRE_NON_NULL(func, "\"Listener removed\" listener"); - BT_ASSERT_PRE(!graph->in_remove_listener, - "Graph currently executing a \"listener removed\" listener: " - "%!+g", graph); g_array_append_val(graph->listeners.filter_output_port_added, listener); listener_id = graph->listeners.filter_output_port_added->len - 1; BT_LIB_LOGD("Added \"filter component output port added\" listener to graph: " @@ -782,25 +734,17 @@ enum bt_graph_add_listener_status bt_graph_add_filter_component_input_port_added_listener( struct bt_graph *graph, bt_graph_filter_component_input_port_added_listener_func func, - bt_graph_listener_removed_func listener_removed, void *data, - bt_listener_id *out_listener_id) + void *data, bt_listener_id *out_listener_id) { struct bt_graph_listener_port_added listener = { - .base = { - .removed = listener_removed, - .data = data, - }, .func = (port_added_func_t) func, + .data = data, }; bt_listener_id listener_id; BT_ASSERT_PRE_NO_ERROR(); BT_ASSERT_PRE_NON_NULL(graph, "Graph"); BT_ASSERT_PRE_NON_NULL(func, "Listener"); - BT_ASSERT_PRE_NON_NULL(func, "\"Listener removed\" listener"); - BT_ASSERT_PRE(!graph->in_remove_listener, - "Graph currently executing a \"listener removed\" listener: " - "%!+g", graph); g_array_append_val(graph->listeners.filter_input_port_added, listener); listener_id = graph->listeners.filter_input_port_added->len - 1; BT_LIB_LOGD("Added \"filter component input port added\" listener to graph: " @@ -818,25 +762,17 @@ enum bt_graph_add_listener_status bt_graph_add_sink_component_input_port_added_listener( struct bt_graph *graph, bt_graph_sink_component_input_port_added_listener_func func, - bt_graph_listener_removed_func listener_removed, void *data, - bt_listener_id *out_listener_id) + void *data, bt_listener_id *out_listener_id) { struct bt_graph_listener_port_added listener = { - .base = { - .removed = listener_removed, - .data = data, - }, .func = (port_added_func_t) func, + .data = data, }; bt_listener_id listener_id; BT_ASSERT_PRE_NO_ERROR(); BT_ASSERT_PRE_NON_NULL(graph, "Graph"); BT_ASSERT_PRE_NON_NULL(func, "Listener"); - BT_ASSERT_PRE_NON_NULL(func, "\"Listener removed\" listener"); - BT_ASSERT_PRE(!graph->in_remove_listener, - "Graph currently executing a \"listener removed\" listener: " - "%!+g", graph); g_array_append_val(graph->listeners.sink_input_port_added, listener); listener_id = graph->listeners.sink_input_port_added->len - 1; BT_LIB_LOGD("Added \"sink component input port added\" listener to graph: " @@ -917,7 +853,7 @@ enum bt_graph_listener_func_status bt_graph_notify_port_added( BT_ASSERT(listener->func); - status = listener->func(comp, port, listener->base.data); + status = listener->func(comp, port, listener->data); BT_ASSERT_POST_NO_ERROR_IF_NO_ERROR_STATUS(status); if (status != BT_FUNC_STATUS_OK) { goto end; diff --git a/src/lib/graph/graph.h b/src/lib/graph/graph.h index 672cb7e3..dcc15260 100644 --- a/src/lib/graph/graph.h +++ b/src/lib/graph/graph.h @@ -106,7 +106,6 @@ struct bt_graph { */ struct bt_interrupter *default_interrupter; - bool in_remove_listener; bool has_sink; /* diff --git a/tests/lib/test_graph_topo.c b/tests/lib/test_graph_topo.c index 184528d1..91e12760 100644 --- a/tests/lib/test_graph_topo.c +++ b/tests/lib/test_graph_topo.c @@ -401,10 +401,10 @@ bt_graph *create_graph(void) BT_ASSERT(graph); ret = bt_graph_add_source_component_output_port_added_listener( - graph, graph_src_output_port_added, NULL, NULL, NULL); + graph, graph_src_output_port_added, NULL, NULL); BT_ASSERT(ret >= 0); ret = bt_graph_add_sink_component_input_port_added_listener( - graph, graph_sink_input_port_added, NULL, NULL, NULL); + graph, graph_sink_input_port_added, NULL, NULL); BT_ASSERT(ret >= 0); return graph; } -- 2.34.1