Fix: only remove newly added components on connection error

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>

diff --git a/lib/component/graph.c b/lib/component/graph.c
index 9982f5d076210d6d825f6d703d56be95a24e0ef1..51f3f957de65f0b7dd32776cc714c7bd1256b5cb 100644 (file)
--- a/lib/component/graph.c
+++ b/lib/component/graph.c
@@ -96,6 +96,8 @@ struct bt_connection *bt_graph_connect(struct bt_graph *graph,
        enum bt_component_status component_status;
        bool upstream_was_already_in_graph;
        bool downstream_was_already_in_graph;
+       int components_to_remove = 0;
+       int i;
 
        if (!graph || !upstream_port || !downstream_port) {
                goto end;
@@ -166,28 +168,84 @@ struct bt_connection *bt_graph_connect(struct bt_graph *graph,
        component_status = bt_component_new_connection(upstream_component,
                        upstream_port, connection);
        if (component_status != BT_COMPONENT_STATUS_OK) {
-               goto error;
+               goto error_rollback;
        }
        component_status = bt_component_new_connection(downstream_component,
                        downstream_port, connection);
        if (component_status != BT_COMPONENT_STATUS_OK) {
-               goto error;
+               goto error_rollback;
        }
 end:
        bt_put(upstream_graph);
        bt_put(downstream_graph);
+       bt_put(upstream_component);
+       bt_put(downstream_component);
        return connection;
-error:
-       if (components_added) {
+error_rollback:
+       /*
+        * Remove newly-added components from the graph, being careful
+        * not to remove a component that was already present in the graph
+        * and is connected to other components.
+        */
+       components_to_remove += upstream_was_already_in_graph ? 0 : 1;
+       components_to_remove += downstream_was_already_in_graph ? 0 : 1;
+
+       if (!downstream_was_already_in_graph) {
                if (bt_component_get_class_type(downstream_component) ==
                                BT_COMPONENT_CLASS_TYPE_SINK) {
                        g_queue_pop_tail(graph->sinks_to_consume);
                }
-               g_ptr_array_set_size(graph->connections,
-                               graph->connections->len - 1);
+       }
+       /* Remove newly created connection. */
+       g_ptr_array_set_size(graph->connections,
+                       graph->connections->len - 1);
+
+       /*
+        * Remove newly added components.
+        *
+        * Note that this is a tricky situation. The graph, being the parent
+        * of the components, does not hold a reference to them. Normally,
+        * components are destroyed right away when the graph is released since
+        * the graph, being their parent, bounds their lifetime
+        * (see doc/ref-counting.md).
+        *
+        * In this particular case, we must take a number of steps:
+        *   1) unset the components' parent to rollback the initial state of
+        *      the components being connected.
+        *      Note that the reference taken by the component on its graph is
+        *      released by the set_parent call.
+        *   2) set the pointer in the components array to NULL so that the
+        *      destruction function called on the array's resize in invoked on
+        *      NULL (no effect),
+        *
+        * NOTE: Point #1 assumes that *something* holds a reference to both
+        *       components being connected. The fact that a reference is being
+        *       held to a component means that it must hold a reference to its
+        *       parent to prevent the parent from being destroyed (again, refer
+        *       to doc/red-counting.md). This reference to a component is
+        *       most likely being held *transitively* by the caller which holds
+        *       a reference to both ports (a port has its component as a
+        *       parent).
+        *
+        *       This assumes that a graph is not connecting components by
+        *       itself while not holding a reference to the ports/components
+        *       being connected (i.e. "cheating" by using internal APIs).
+        */
+       for (i = 0; i < components_to_remove; i++) {
+               struct bt_component *component = g_ptr_array_index(
+                               graph->components, graph->components->len - 1);
+
+               bt_component_set_graph(component, NULL);
+               g_ptr_array_index(graph->components,
+                               graph->components->len - 1) = NULL;
                g_ptr_array_set_size(graph->components,
-                               graph->components->len - 2);
+                               graph->components->len - 1);
        }
+       /* NOTE: Resizing the ptr_arrays invokes the destruction of the elements. */
+       goto end;
+error:
+       BT_PUT(upstream_component);
+       BT_PUT(downstream_component);
        goto end;
 }