[PATCH 1/5] dbus: Coalesce InterfacesAdded/Removed signals

[PATCH 1/5] dbus: Coalesce InterfacesAdded/Removed signals

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 14493 bytes --]

When multiple interfaces are added or removed during the same main loop
cycle emit one signal per object with all the interfaces added or
removed.  Move the signals to the idle callback.
---
 ell/dbus-service.c | 337 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 257 insertions(+), 80 deletions(-)

diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index afd0ed9..3e8005d 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -38,6 +38,7 @@
 #include "dbus-service.h"
 #include "dbus-private.h"
 #include "private.h"
+#include "idle.h"
 
 #define XML_ID "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN"
 #define XML_DTD "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd"
@@ -104,6 +105,20 @@ struct object_node {
 struct object_manager {
 	char *path;
 	struct l_dbus *dbus;
+	struct l_queue *announce_added;
+	struct l_queue *announce_removed;
+};
+
+struct interface_add_record {
+	char *path;
+	struct object_node *object;
+	struct l_queue *instances;
+};
+
+struct interface_remove_record {
+	char *path;
+	struct object_node *object;
+	struct l_queue *interface_names;
 };
 
 struct _dbus_object_tree {
@@ -111,6 +126,7 @@ struct _dbus_object_tree {
 	struct l_hashmap *objects;
 	struct object_node *root;
 	struct l_queue *object_managers;
+	struct l_idle *emit_signals_work;
 };
 
 void _dbus_method_introspection(struct _dbus_method *info,
@@ -520,6 +536,24 @@ static bool match_interface_instance(const void *a, const void *b)
 	return false;
 }
 
+static void interfaces_added_rec_free(void *data)
+{
+	struct interface_add_record *rec = data;
+
+	l_free(rec->path);
+	l_queue_destroy(rec->instances, NULL);
+	l_free(rec);
+}
+
+static void interfaces_removed_rec_free(void *data)
+{
+	struct interface_remove_record *rec = data;
+
+	l_free(rec->path);
+	l_queue_destroy(rec->interface_names, l_free);
+	l_free(rec);
+}
+
 static void properties_setup_func(struct l_dbus_interface *);
 static void object_manager_setup_func(struct l_dbus_interface *);
 
@@ -578,6 +612,8 @@ static void object_manager_free(void *data)
 	struct object_manager *manager = data;
 
 	l_free(manager->path);
+	l_queue_destroy(manager->announce_added, interfaces_added_rec_free);
+	l_queue_destroy(manager->announce_removed, interfaces_removed_rec_free);
 	l_free(manager);
 }
 
@@ -591,6 +627,9 @@ void _dbus_object_tree_free(struct _dbus_object_tree *tree)
 
 	l_queue_destroy(tree->object_managers, object_manager_free);
 
+	if (tree->emit_signals_work)
+		l_idle_remove(tree->emit_signals_work);
+
 	l_free(tree);
 }
 
@@ -764,6 +803,163 @@ bool _dbus_object_tree_object_destroy(struct _dbus_object_tree *tree,
 	return true;
 }
 
+static bool get_properties_dict(struct l_dbus *dbus,
+				struct l_dbus_message *message,
+				struct l_dbus_message_builder *builder,
+				const struct l_dbus_interface *interface,
+				void *user_data)
+{
+	const struct l_queue_entry *entry;
+	const struct _dbus_property *property;
+	const char *signature;
+
+	l_dbus_message_builder_enter_array(builder, "{sv}");
+
+	for (entry = l_queue_get_entries(interface->properties); entry;
+			entry = entry->next) {
+		property = entry->data;
+		signature = property->metainfo + strlen(property->metainfo) + 1;
+
+		l_dbus_message_builder_enter_dict(builder, "sv");
+		l_dbus_message_builder_append_basic(builder, 's',
+							property->metainfo);
+		l_dbus_message_builder_enter_variant(builder, signature);
+
+		if (!property->getter(dbus, message, builder, user_data))
+			return false;
+
+		l_dbus_message_builder_leave_variant(builder);
+		l_dbus_message_builder_leave_dict(builder);
+	}
+
+	l_dbus_message_builder_leave_array(builder);
+
+	return true;
+}
+
+static struct l_dbus_message *build_interfaces_removed_signal(
+				const struct object_manager *manager,
+				const struct interface_remove_record *rec)
+{
+	struct l_dbus_message *signal;
+	struct l_dbus_message_builder *builder;
+	const struct l_queue_entry *entry;
+
+	signal = l_dbus_message_new_signal(manager->dbus, manager->path,
+						DBUS_INTERFACE_OBJECT_MANAGER,
+						"InterfacesRemoved");
+
+	builder = l_dbus_message_builder_new(signal);
+
+	l_dbus_message_builder_append_basic(builder, 'o', rec->path);
+	l_dbus_message_builder_enter_array(builder, "s");
+
+	for (entry = l_queue_get_entries(rec->interface_names); entry;
+			entry = entry->next)
+		l_dbus_message_builder_append_basic(builder, 's', entry->data);
+
+	l_dbus_message_builder_leave_array(builder);
+	l_dbus_message_builder_finalize(builder);
+	l_dbus_message_builder_destroy(builder);
+
+	return signal;
+}
+
+static struct l_dbus_message *build_interfaces_added_signal(
+					const struct object_manager *manager,
+					const struct interface_add_record *rec)
+{
+	struct l_dbus_message *signal;
+	struct l_dbus_message_builder *builder;
+	const struct l_queue_entry *entry;
+	const struct interface_instance *instance;
+
+	signal = l_dbus_message_new_signal(manager->dbus, manager->path,
+						DBUS_INTERFACE_OBJECT_MANAGER,
+						"InterfacesAdded");
+
+	builder = l_dbus_message_builder_new(signal);
+
+	l_dbus_message_builder_append_basic(builder, 'o', rec->path);
+	l_dbus_message_builder_enter_array(builder, "{sa{sv}}");
+
+	for (entry = l_queue_get_entries(rec->instances); entry;
+			entry = entry->next) {
+		instance = entry->data;
+
+		l_dbus_message_builder_enter_dict(builder, "sa{sv}");
+		l_dbus_message_builder_append_basic(builder, 's',
+						instance->interface->name);
+
+		if (!get_properties_dict(manager->dbus, signal, builder,
+						instance->interface,
+						instance->user_data)) {
+			l_dbus_message_builder_destroy(builder);
+			l_dbus_message_unref(signal);
+
+			return NULL;
+		}
+
+		l_dbus_message_builder_leave_dict(builder);
+	}
+
+	l_dbus_message_builder_leave_array(builder);
+	l_dbus_message_builder_finalize(builder);
+	l_dbus_message_builder_destroy(builder);
+
+	return signal;
+}
+
+static void emit_signals(struct l_idle *idle, void *user_data)
+{
+	struct l_dbus *dbus = user_data;
+	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+	struct interface_add_record *interfaces_added_rec;
+	struct interface_remove_record *interfaces_removed_rec;
+	struct property_changes *property_changes_rec;
+	const struct l_queue_entry *entry;
+	struct object_manager *manager;
+	struct l_dbus_message *signal;
+
+	l_idle_remove(tree->emit_signals_work);
+	tree->emit_signals_work = NULL;
+
+	for (entry = l_queue_get_entries(tree->object_managers); entry;
+			entry = entry->next) {
+		manager = entry->data;
+
+		while ((interfaces_removed_rec =
+				l_queue_pop_head(manager->announce_removed))) {
+			signal = build_interfaces_removed_signal(manager,
+							interfaces_removed_rec);
+			interfaces_removed_rec_free(interfaces_removed_rec);
+
+			if (signal)
+				l_dbus_send(manager->dbus, signal);
+		}
+
+		while ((interfaces_added_rec =
+				l_queue_pop_head(manager->announce_added))) {
+			signal = build_interfaces_added_signal(manager,
+							interfaces_added_rec);
+			interfaces_added_rec_free(interfaces_added_rec);
+
+			if (signal)
+				l_dbus_send(manager->dbus, signal);
+		}
+	}
+}
+
+static void schedule_emit_signals(struct l_dbus *dbus)
+{
+	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+
+	if (tree->emit_signals_work)
+		return;
+
+	tree->emit_signals_work = l_idle_create(emit_signals, dbus, NULL);
+}
+
 /* Send the signals associated with a property value change */
 bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
 					const char *path,
@@ -941,40 +1137,6 @@ static struct l_dbus_message *old_set_property(struct l_dbus *dbus,
 	return NULL;
 }
 
-static bool get_properties_dict(struct l_dbus *dbus,
-				struct l_dbus_message *message,
-				struct l_dbus_message_builder *builder,
-				const struct l_dbus_interface *interface,
-				void *user_data)
-{
-	const struct l_queue_entry *entry;
-	const struct _dbus_property *property;
-	const char *signature;
-
-	l_dbus_message_builder_enter_array(builder, "{sv}");
-
-	for (entry = l_queue_get_entries(interface->properties); entry;
-			entry = entry->next) {
-		property = entry->data;
-		signature = property->metainfo + strlen(property->metainfo) + 1;
-
-		l_dbus_message_builder_enter_dict(builder, "sv");
-		l_dbus_message_builder_append_basic(builder, 's',
-							property->metainfo);
-		l_dbus_message_builder_enter_variant(builder, signature);
-
-		if (!property->getter(dbus, message, builder, user_data))
-			return false;
-
-		l_dbus_message_builder_leave_variant(builder);
-		l_dbus_message_builder_leave_dict(builder);
-	}
-
-	l_dbus_message_builder_leave_array(builder);
-
-	return true;
-}
-
 static struct l_dbus_message *old_get_properties(struct l_dbus *dbus,
 						struct l_dbus_message *message,
 						void *user_data)
@@ -1088,42 +1250,18 @@ bool _dbus_object_tree_unregister_interface(struct _dbus_object_tree *tree,
 	return true;
 }
 
-static bool build_interfaces_added_signal(const struct object_manager *manager,
-				const char *path,
-				const struct interface_instance *instance)
+static bool match_interfaces_added_object(const void *a, const void *b)
 {
-	struct l_dbus_message *signal;
-	struct l_dbus_message_builder *builder;
-
-	signal = l_dbus_message_new_signal(manager->dbus, manager->path,
-						DBUS_INTERFACE_OBJECT_MANAGER,
-						"InterfacesAdded");
-	builder = l_dbus_message_builder_new(signal);
-
-	l_dbus_message_builder_append_basic(builder, 'o', path);
-	l_dbus_message_builder_enter_array(builder, "{sa{sv}}");
-	l_dbus_message_builder_enter_dict(builder, "sa{sv}");
-	l_dbus_message_builder_append_basic(builder, 's',
-						instance->interface->name);
-
-	if (!get_properties_dict(manager->dbus, signal, builder,
-					instance->interface,
-					instance->user_data)) {
-		l_dbus_message_builder_destroy(builder);
-		l_dbus_message_unref(signal);
-
-		return false;
-	}
-
-	l_dbus_message_builder_leave_dict(builder);
-	l_dbus_message_builder_leave_array(builder);
+	const struct interface_add_record *rec = a;
 
-	l_dbus_message_builder_finalize(builder);
-	l_dbus_message_builder_destroy(builder);
+	return rec->object == b;
+}
 
-	l_dbus_send(manager->dbus, signal);
+static bool match_interfaces_removed_object(const void *a, const void *b)
+{
+	const struct interface_add_record *rec = a;
 
-	return true;
+	return rec->object == b;
 }
 
 bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
@@ -1136,6 +1274,7 @@ bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
 	const struct l_queue_entry *entry;
 	struct object_manager *manager;
 	size_t path_len;
+	struct interface_add_record *change_rec;
 
 	dbi = l_hashmap_lookup(tree->interfaces, interface);
 	if (!dbi)
@@ -1173,13 +1312,30 @@ bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
 				 path[path_len] != '/' && path_len > 1))
 			continue;
 
-		build_interfaces_added_signal(manager, path, instance);
+		change_rec = l_queue_find(manager->announce_added,
+						match_interfaces_added_object,
+						object);
+		if (!change_rec) {
+			change_rec = l_new(struct interface_add_record, 1);
+			change_rec->path = l_strdup(path);
+			change_rec->object = object;
+			change_rec->instances = l_queue_new();
+
+			l_queue_push_tail(manager->announce_added, change_rec);
+		}
+
+		/* No need to check for duplicates here */
+		l_queue_push_tail(change_rec->instances, instance);
+
+		schedule_emit_signals(manager->dbus);
 	}
 
 	if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER)) {
 		manager = l_new(struct object_manager, 1);
 		manager->path = l_strdup(path);
 		manager->dbus = instance->user_data;
+		manager->announce_added = l_queue_new();
+		manager->announce_removed = l_queue_new();
 
 		l_queue_push_tail(tree->object_managers, manager);
 	}
@@ -1201,9 +1357,9 @@ bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
 	struct interface_instance *instance;
 	const struct l_queue_entry *entry;
 	struct object_manager *manager;
-	struct l_dbus_message *signal;
-	struct l_dbus_message_builder *builder;
 	size_t path_len;
+	struct interface_add_record *interfaces_added_rec;
+	struct interface_remove_record *interfaces_removed_rec;
 
 	node = l_hashmap_lookup(tree->objects, path);
 	if (!node)
@@ -1234,19 +1390,40 @@ bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
 				 path[path_len] != '/' && path_len > 1))
 			continue;
 
-		signal = l_dbus_message_new_signal(manager->dbus, manager->path,
-						DBUS_INTERFACE_OBJECT_MANAGER,
-						"InterfacesRemoved");
+		interfaces_added_rec = l_queue_find(manager->announce_added,
+						match_interfaces_added_object,
+						node);
+		if (interfaces_added_rec && l_queue_remove(
+						interfaces_added_rec->instances,
+						instance)) {
+			if (l_queue_isempty(interfaces_added_rec->instances))
+				l_queue_remove(manager->announce_added,
+						interfaces_added_rec);
 
-		builder = l_dbus_message_builder_new(signal);
+			interfaces_added_rec_free(interfaces_added_rec);
 
-		l_dbus_message_builder_append_basic(builder, 'o', path);
-		l_dbus_message_builder_enter_array(builder, "s");
-		l_dbus_message_builder_append_basic(builder, 's', interface);
-		l_dbus_message_builder_leave_array(builder);
-		l_dbus_message_builder_finalize(builder);
-		l_dbus_message_builder_destroy(builder);
-		l_dbus_send(manager->dbus, signal);
+			continue;
+		}
+
+		interfaces_removed_rec = l_queue_find(manager->announce_removed,
+						match_interfaces_removed_object,
+						node);
+		if (!interfaces_removed_rec) {
+			interfaces_removed_rec =
+				l_new(struct interface_remove_record, 1);
+			interfaces_removed_rec->path = l_strdup(path);
+			interfaces_removed_rec->object = node;
+			interfaces_removed_rec->interface_names =
+				l_queue_new();
+			l_queue_push_tail(manager->announce_removed,
+						interfaces_removed_rec);
+		}
+
+		/* No need to check for duplicates here */
+		l_queue_push_tail(interfaces_removed_rec->interface_names,
+					l_strdup(interface));
+
+		schedule_emit_signals(manager->dbus);
 	}
 
 	return true;
-- 
2.5.0

[PATCH 2/5] dbus: Coalesce PropertiesChanged signals

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 12118 bytes --]

When multiple properties are changed in one main loop cycle emit only
one signal per interface with all the changes.
---
 ell/dbus-private.h |   3 +-
 ell/dbus-service.c | 256 ++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 177 insertions(+), 82 deletions(-)

diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 873396d..f676bdb 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -200,8 +200,7 @@ bool _dbus_object_tree_dispatch(struct _dbus_object_tree *tree,
 bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
 					const char *path,
 					const char *interface_name,
-					const char *property_name,
-					struct l_dbus_message_iter *variant);
+					const char *property_name);
 
 void _dbus_kernel_bloom_add(uint64_t filter[], size_t size, uint8_t num_hash,
 				const char *prefix, const char *str);
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 3e8005d..063cdd3 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -121,11 +121,19 @@ struct interface_remove_record {
 	struct l_queue *interface_names;
 };
 
+struct property_changes {
+	char *path;
+	struct object_node *object;
+	struct interface_instance *instance;
+	struct l_queue *properties;
+};
+
 struct _dbus_object_tree {
 	struct l_hashmap *interfaces;
 	struct l_hashmap *objects;
 	struct object_node *root;
 	struct l_queue *object_managers;
+	struct l_queue *property_changes;
 	struct l_idle *emit_signals_work;
 };
 
@@ -554,6 +562,15 @@ static void interfaces_removed_rec_free(void *data)
 	l_free(rec);
 }
 
+static void property_change_rec_free(void *data)
+{
+	struct property_changes *rec = data;
+
+	l_free(rec->path);
+	l_queue_destroy(rec->properties, NULL);
+	l_free(rec);
+}
+
 static void properties_setup_func(struct l_dbus_interface *);
 static void object_manager_setup_func(struct l_dbus_interface *);
 
@@ -572,6 +589,8 @@ struct _dbus_object_tree *_dbus_object_tree_new()
 
 	tree->root = l_new(struct object_node, 1);
 
+	tree->property_changes = l_queue_new();
+
 	_dbus_object_tree_register_interface(tree, DBUS_INTERFACE_PROPERTIES,
 						properties_setup_func, NULL,
 						false);
@@ -627,6 +646,8 @@ void _dbus_object_tree_free(struct _dbus_object_tree *tree)
 
 	l_queue_destroy(tree->object_managers, object_manager_free);
 
+	l_queue_destroy(tree->property_changes, property_change_rec_free);
+
 	if (tree->emit_signals_work)
 		l_idle_remove(tree->emit_signals_work);
 
@@ -910,6 +931,93 @@ static struct l_dbus_message *build_interfaces_added_signal(
 	return signal;
 }
 
+static struct l_dbus_message *build_old_property_changed_signal(
+					struct l_dbus *dbus,
+					const struct property_changes *rec,
+					const struct _dbus_property *property)
+{
+	struct l_dbus_message *signal;
+	struct l_dbus_message_builder *builder;
+	const char *signature;
+
+	signature = property->metainfo + strlen(property->metainfo) + 1;
+
+	signal = l_dbus_message_new_signal(dbus, rec->path,
+						rec->instance->interface->name,
+						"PropertyChanged");
+
+	builder = l_dbus_message_builder_new(signal);
+
+	l_dbus_message_builder_append_basic(builder, 's', property->metainfo);
+	l_dbus_message_builder_enter_variant(builder, signature);
+
+	if (!property->getter(dbus, signal, builder,
+				rec->instance->user_data)) {
+		l_dbus_message_builder_destroy(builder);
+		l_dbus_message_unref(signal);
+
+		return NULL;
+	}
+
+	l_dbus_message_builder_leave_variant(builder);
+
+	l_dbus_message_builder_finalize(builder);
+	l_dbus_message_builder_destroy(builder);
+
+	return signal;
+}
+
+static struct l_dbus_message *build_properties_changed_signal(
+					struct l_dbus *dbus,
+					const struct property_changes *rec)
+{
+	struct l_dbus_message *signal;
+	struct l_dbus_message_builder *builder;
+	const struct l_queue_entry *entry;
+	const struct _dbus_property *property;
+	const char *signature;
+
+	signal = l_dbus_message_new_signal(dbus, rec->path,
+						DBUS_INTERFACE_PROPERTIES,
+						"PropertiesChanged");
+
+	builder = l_dbus_message_builder_new(signal);
+
+	l_dbus_message_builder_append_basic(builder, 's',
+						rec->instance->interface->name);
+	l_dbus_message_builder_enter_array(builder, "{sv}");
+
+	for (entry = l_queue_get_entries(rec->properties); entry;
+			entry = entry->next) {
+		property = entry->data;
+		signature = property->metainfo + strlen(property->metainfo) + 1;
+
+		l_dbus_message_builder_enter_dict(builder, "sv");
+		l_dbus_message_builder_append_basic(builder, 's',
+							property->metainfo);
+		l_dbus_message_builder_enter_variant(builder, signature);
+
+		if (!property->getter(dbus, signal, builder,
+					rec->instance->user_data)) {
+			l_dbus_message_builder_destroy(builder);
+			l_dbus_message_unref(signal);
+
+			return NULL;
+		}
+
+		l_dbus_message_builder_leave_variant(builder);
+		l_dbus_message_builder_leave_dict(builder);
+	}
+
+	l_dbus_message_builder_leave_array(builder);
+	l_dbus_message_builder_enter_array(builder, "s");
+	l_dbus_message_builder_leave_array(builder);
+	l_dbus_message_builder_finalize(builder);
+	l_dbus_message_builder_destroy(builder);
+
+	return signal;
+}
+
 static void emit_signals(struct l_idle *idle, void *user_data)
 {
 	struct l_dbus *dbus = user_data;
@@ -948,6 +1056,33 @@ static void emit_signals(struct l_idle *idle, void *user_data)
 				l_dbus_send(manager->dbus, signal);
 		}
 	}
+
+	while ((property_changes_rec =
+			l_queue_pop_head(tree->property_changes))) {
+		if (property_changes_rec->instance->interface->
+				handle_old_style_properties)
+			for (entry = l_queue_get_entries(property_changes_rec->
+								properties);
+					entry; entry = entry->next) {
+				signal = build_old_property_changed_signal(dbus,
+							property_changes_rec,
+							entry->data);
+				if (signal)
+					l_dbus_send(dbus, signal);
+			}
+
+
+		if (l_queue_find(property_changes_rec->object->instances,
+					match_interface_instance,
+					DBUS_INTERFACE_PROPERTIES)) {
+			signal = build_properties_changed_signal(dbus,
+							property_changes_rec);
+			if (signal)
+				l_dbus_send(dbus, signal);
+		}
+
+		property_change_rec_free(property_changes_rec);
+	}
 }
 
 static void schedule_emit_signals(struct l_dbus *dbus)
@@ -960,22 +1095,28 @@ static void schedule_emit_signals(struct l_dbus *dbus)
 	tree->emit_signals_work = l_idle_create(emit_signals, dbus, NULL);
 }
 
-/* Send the signals associated with a property value change */
+static bool match_property_changes_instance(const void *a, const void *b)
+{
+	const struct property_changes *rec = a;
+
+	return rec->instance == b;
+}
+
+static bool match_pointer(const void *a, const void *b)
+{
+	return a == b;
+}
+
 bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
 					const char *path,
 					const char *interface_name,
-					const char *property_name,
-					struct l_dbus_message_iter *variant)
+					const char *property_name)
 {
-	struct l_dbus_message *signal;
-	struct l_dbus_message_builder *builder;
-	const struct _dbus_property *property;
-	const char *signature;
-	const struct interface_instance *instance;
-	const struct object_node *object;
-	struct l_dbus_message_iter value;
+	struct property_changes *rec;
+	struct object_node *object;
+	struct interface_instance *instance;
+	struct _dbus_property *property;
 	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
-	bool r;
 
 	object = l_hashmap_lookup(tree->objects, path);
 	if (!object)
@@ -991,76 +1132,25 @@ bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
 	if (!property)
 		return false;
 
-	signature = property->metainfo + strlen(property->metainfo) + 1;
-
-	if (instance->interface->handle_old_style_properties) {
-		signal = l_dbus_message_new_signal(dbus, path,
-							interface_name,
-							"PropertyChanged");
+	rec = l_queue_find(tree->property_changes,
+				match_property_changes_instance, instance);
 
-		builder = l_dbus_message_builder_new(signal);
-
-		l_dbus_message_builder_append_basic(builder, 's',
-							property_name);
-		l_dbus_message_builder_enter_variant(builder, signature);
+	if (rec) {
+		if (l_queue_find(rec->properties, match_pointer, property))
+			return true;
+	} else {
+		rec = l_new(struct property_changes, 1);
+		rec->path = l_strdup(path);
+		rec->object = object;
+		rec->instance = instance;
+		rec->properties = l_queue_new();
 
-		if (variant) {
-			memcpy(&value, variant, sizeof(value));
-			r = l_dbus_message_builder_append_from_iter(builder,
-								&value);
-		} else {
-			r = property->getter(dbus, signal, builder,
-						instance->user_data);
-		}
-
-		if (r) {
-			l_dbus_message_builder_leave_variant(builder);
-
-			l_dbus_message_builder_finalize(builder);
-			l_dbus_send(dbus, signal);
-		}
-
-		l_dbus_message_builder_destroy(builder);
+		l_queue_push_tail(tree->property_changes, rec);
 	}
 
-	if (l_queue_find(object->instances, match_interface_instance,
-				DBUS_INTERFACE_PROPERTIES)) {
-		signal = l_dbus_message_new_signal(dbus, path,
-						DBUS_INTERFACE_PROPERTIES,
-						"PropertiesChanged");
-
-		builder = l_dbus_message_builder_new(signal);
-
-		l_dbus_message_builder_append_basic(builder, 's',
-							interface_name);
-		l_dbus_message_builder_enter_array(builder, "{sv}");
-		l_dbus_message_builder_enter_dict(builder, "sv");
-		l_dbus_message_builder_append_basic(builder, 's',
-							property_name);
-		l_dbus_message_builder_enter_variant(builder, signature);
+	l_queue_push_tail(rec->properties, property);
 
-		if (variant) {
-			memcpy(&value, variant, sizeof(value));
-			r = l_dbus_message_builder_append_from_iter(builder,
-								&value);
-		} else {
-			r = property->getter(dbus, signal, builder,
-						instance->user_data);
-		}
-
-		if (r) {
-			l_dbus_message_builder_leave_variant(builder);
-			l_dbus_message_builder_leave_dict(builder);
-			l_dbus_message_builder_leave_array(builder);
-			l_dbus_message_builder_enter_array(builder, "s");
-			l_dbus_message_builder_leave_array(builder);
-
-			l_dbus_message_builder_finalize(builder);
-			l_dbus_send(dbus, signal);
-		}
-
-		l_dbus_message_builder_destroy(builder);
-	}
+	schedule_emit_signals(dbus);
 
 	return true;
 }
@@ -1090,7 +1180,7 @@ static void set_property_complete(struct l_dbus *dbus,
 	_dbus_object_tree_property_changed(dbus,
 					l_dbus_message_get_path(message),
 					l_dbus_message_get_interface(message),
-					property_name, &variant);
+					property_name);
 
 	l_dbus_message_unref(message);
 }
@@ -1360,6 +1450,7 @@ bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
 	size_t path_len;
 	struct interface_add_record *interfaces_added_rec;
 	struct interface_remove_record *interfaces_removed_rec;
+	struct property_changes *property_change_rec;
 
 	node = l_hashmap_lookup(tree->objects, path);
 	if (!node)
@@ -1426,6 +1517,12 @@ bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
 		schedule_emit_signals(manager->dbus);
 	}
 
+	property_change_rec = l_queue_remove_if(tree->property_changes,
+						match_property_changes_instance,
+						instance);
+	if (property_change_rec)
+		property_change_rec_free(property_change_rec);
+
 	return true;
 }
 
@@ -1534,7 +1631,7 @@ LIB_EXPORT bool l_dbus_property_changed(struct l_dbus *dbus, const char *path,
 					const char *property)
 {
 	return _dbus_object_tree_property_changed(dbus, path, interface,
-							property, NULL);
+							property);
 }
 
 static struct l_dbus_message *properties_get(struct l_dbus *dbus,
@@ -1630,8 +1727,7 @@ static void properties_set_complete(struct l_dbus *dbus,
 
 	_dbus_object_tree_property_changed(dbus,
 					l_dbus_message_get_path(message),
-					interface_name, property_name,
-					&variant);
+					interface_name, property_name);
 
 	l_dbus_message_unref(message);
 }
-- 
2.5.0

Re: [PATCH 2/5] dbus: Coalesce PropertiesChanged signals

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

Hi Andrew,

On 02/17/2016 09:20 PM, Andrew Zaborowski wrote:
> When multiple properties are changed in one main loop cycle emit only
> one signal per interface with all the changes.
> ---
>   ell/dbus-private.h |   3 +-
>   ell/dbus-service.c | 256 ++++++++++++++++++++++++++++++++++++-----------------
>   2 files changed, 177 insertions(+), 82 deletions(-)
>

<snip>

> @@ -991,76 +1132,25 @@ bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
>   	if (!property)
>   		return false;
>
> -	signature = property->metainfo + strlen(property->metainfo) + 1;
> -
> -	if (instance->interface->handle_old_style_properties) {
> -		signal = l_dbus_message_new_signal(dbus, path,
> -							interface_name,
> -							"PropertyChanged");
> +	rec = l_queue_find(tree->property_changes,
> +				match_property_changes_instance, instance);
>
> -		builder = l_dbus_message_builder_new(signal);
> -
> -		l_dbus_message_builder_append_basic(builder, 's',
> -							property_name);
> -		l_dbus_message_builder_enter_variant(builder, signature);

This looks a bit suspicious.  Should we be also comparing the object 
pointer, not just the instance pointer here?

> +	if (rec) {
> +		if (l_queue_find(rec->properties, match_pointer, property))
> +			return true;
> +	} else {
> +		rec = l_new(struct property_changes, 1);
> +		rec->path = l_strdup(path);
> +		rec->object = object;
> +		rec->instance = instance;
> +		rec->properties = l_queue_new();
>
> -		if (variant) {
> -			memcpy(&value, variant, sizeof(value));
> -			r = l_dbus_message_builder_append_from_iter(builder,
> -								&value);
> -		} else {
> -			r = property->getter(dbus, signal, builder,
> -						instance->user_data);
> -		}
> -
> -		if (r) {
> -			l_dbus_message_builder_leave_variant(builder);
> -
> -			l_dbus_message_builder_finalize(builder);
> -			l_dbus_send(dbus, signal);
> -		}
> -
> -		l_dbus_message_builder_destroy(builder);
> +		l_queue_push_tail(tree->property_changes, rec);
>   	}
>
> -	if (l_queue_find(object->instances, match_interface_instance,
> -				DBUS_INTERFACE_PROPERTIES)) {
> -		signal = l_dbus_message_new_signal(dbus, path,
> -						DBUS_INTERFACE_PROPERTIES,
> -						"PropertiesChanged");
> -
> -		builder = l_dbus_message_builder_new(signal);
> -
> -		l_dbus_message_builder_append_basic(builder, 's',
> -							interface_name);
> -		l_dbus_message_builder_enter_array(builder, "{sv}");
> -		l_dbus_message_builder_enter_dict(builder, "sv");
> -		l_dbus_message_builder_append_basic(builder, 's',
> -							property_name);
> -		l_dbus_message_builder_enter_variant(builder, signature);
> +	l_queue_push_tail(rec->properties, property);
>
> -		if (variant) {
> -			memcpy(&value, variant, sizeof(value));
> -			r = l_dbus_message_builder_append_from_iter(builder,
> -								&value);
> -		} else {
> -			r = property->getter(dbus, signal, builder,
> -						instance->user_data);
> -		}
> -
> -		if (r) {
> -			l_dbus_message_builder_leave_variant(builder);
> -			l_dbus_message_builder_leave_dict(builder);
> -			l_dbus_message_builder_leave_array(builder);
> -			l_dbus_message_builder_enter_array(builder, "s");
> -			l_dbus_message_builder_leave_array(builder);
> -
> -			l_dbus_message_builder_finalize(builder);
> -			l_dbus_send(dbus, signal);
> -		}
> -
> -		l_dbus_message_builder_destroy(builder);
> -	}
> +	schedule_emit_signals(dbus);
>
>   	return true;
>   }

Regards,
-Denis

Re: [PATCH 2/5] dbus: Coalesce PropertiesChanged signals

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

Hi Denis,

On 26 February 2016 at 21:30, Denis Kenzior <denkenz@gmail.com> wrote:
> On 02/17/2016 09:20 PM, Andrew Zaborowski wrote:
>>         if (!property)
>>                 return false;
>>
>> -       signature = property->metainfo + strlen(property->metainfo) + 1;
>> -
>> -       if (instance->interface->handle_old_style_properties) {
>> -               signal = l_dbus_message_new_signal(dbus, path,
>> -                                                       interface_name,
>> -
>> "PropertyChanged");
>> +       rec = l_queue_find(tree->property_changes,
>> +                               match_property_changes_instance,
>> instance);
>>
>> -               builder = l_dbus_message_builder_new(signal);
>> -
>> -               l_dbus_message_builder_append_basic(builder, 's',
>> -                                                       property_name);
>> -               l_dbus_message_builder_enter_variant(builder, signature);
>
>
> This looks a bit suspicious.  Should we be also comparing the object
> pointer, not just the instance pointer here?

Assuming that only one object can have the instance it shouldn't
matter.  When the object or the instance are removed, the property
change record is also removed so it won't accidentally point at a
different instance.

We also rely on that for the interface added notifications.

Best regards

Re: [PATCH 2/5] dbus: Coalesce PropertiesChanged signals

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]

Hi Andrew,

On 02/29/2016 03:21 AM, Andrzej Zaborowski wrote:
> Hi Denis,
>
> On 26 February 2016 at 21:30, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 02/17/2016 09:20 PM, Andrew Zaborowski wrote:
>>>          if (!property)
>>>                  return false;
>>>
>>> -       signature = property->metainfo + strlen(property->metainfo) + 1;
>>> -
>>> -       if (instance->interface->handle_old_style_properties) {
>>> -               signal = l_dbus_message_new_signal(dbus, path,
>>> -                                                       interface_name,
>>> -
>>> "PropertyChanged");
>>> +       rec = l_queue_find(tree->property_changes,
>>> +                               match_property_changes_instance,
>>> instance);
>>>

The l_queue_find call only takes the instance into consideration, not 
the object pointer.

>>> -               builder = l_dbus_message_builder_new(signal);
>>> -
>>> -               l_dbus_message_builder_append_basic(builder, 's',
>>> -                                                       property_name);
>>> -               l_dbus_message_builder_enter_variant(builder, signature);
>>
>>
>> This looks a bit suspicious.  Should we be also comparing the object
>> pointer, not just the instance pointer here?
>
> Assuming that only one object can have the instance it shouldn't
> matter.  When the object or the instance are removed, the property
> change record is also removed so it won't accidentally point at a
> different instance.
>
> We also rely on that for the interface added notifications.
>

What about multiple objects with the same interface?

E.g. org.foo.Foobar interface on /bar1 and /bar2.  If Foobar.Baz 
property changes on /bar1 and /bar2.

Alternatively, if Foobar.Baz1 changes on /bar1, and Foobar.Baz2 changes 
on /bar2, you can also get into a weird situation.

Or am I missing something?

Regards,
-Denis

Re: [PATCH 2/5] dbus: Coalesce PropertiesChanged signals

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

After talking to Andrew, I realized I was off-base here.

 >> This looks a bit suspicious.  Should we be also comparing the object
>> pointer, not just the instance pointer here?
>
> Assuming that only one object can have the instance it shouldn't
> matter.  When the object or the instance are removed, the property
> change record is also removed so it won't accidentally point at a
> different instance.

I for some reason was thinking we're tracking l_dbus_interface objects, 
not interface_instance.  So after being being reminded of that, the code 
is just fine.

Regards,
-Denis

[PATCH 3/5] dbus: Move standard interface name defines to public header.

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 5688 bytes --]

---
 ell/dbus-service.c | 19 ++++++++-----------
 ell/dbus.c         | 12 ++++--------
 ell/dbus.h         |  5 +++++
 3 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 063cdd3..f875437 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -50,9 +50,6 @@ static const char *static_introspectable =
 		"\t\t\t<arg name=\"xml\" type=\"s\" direction=\"out\"/>\n"
 		"\t\t</method>\n\t</interface>\n";
 
-#define DBUS_INTERFACE_PROPERTIES	"org.freedesktop.DBus.Properties"
-#define DBUS_INTERFACE_OBJECT_MANAGER	"org.freedesktop.DBus.ObjectManager"
-
 struct _dbus_method {
 	l_dbus_interface_method_cb_t cb;
 	uint32_t flags;
@@ -591,14 +588,14 @@ struct _dbus_object_tree *_dbus_object_tree_new()
 
 	tree->property_changes = l_queue_new();
 
-	_dbus_object_tree_register_interface(tree, DBUS_INTERFACE_PROPERTIES,
+	_dbus_object_tree_register_interface(tree, L_DBUS_INTERFACE_PROPERTIES,
 						properties_setup_func, NULL,
 						false);
 
 	tree->object_managers = l_queue_new();
 
 	_dbus_object_tree_register_interface(tree,
-						DBUS_INTERFACE_OBJECT_MANAGER,
+						L_DBUS_INTERFACE_OBJECT_MANAGER,
 						object_manager_setup_func, NULL,
 						false);
 
@@ -867,7 +864,7 @@ static struct l_dbus_message *build_interfaces_removed_signal(
 	const struct l_queue_entry *entry;
 
 	signal = l_dbus_message_new_signal(manager->dbus, manager->path,
-						DBUS_INTERFACE_OBJECT_MANAGER,
+						L_DBUS_INTERFACE_OBJECT_MANAGER,
 						"InterfacesRemoved");
 
 	builder = l_dbus_message_builder_new(signal);
@@ -896,7 +893,7 @@ static struct l_dbus_message *build_interfaces_added_signal(
 	const struct interface_instance *instance;
 
 	signal = l_dbus_message_new_signal(manager->dbus, manager->path,
-						DBUS_INTERFACE_OBJECT_MANAGER,
+						L_DBUS_INTERFACE_OBJECT_MANAGER,
 						"InterfacesAdded");
 
 	builder = l_dbus_message_builder_new(signal);
@@ -978,7 +975,7 @@ static struct l_dbus_message *build_properties_changed_signal(
 	const char *signature;
 
 	signal = l_dbus_message_new_signal(dbus, rec->path,
-						DBUS_INTERFACE_PROPERTIES,
+						L_DBUS_INTERFACE_PROPERTIES,
 						"PropertiesChanged");
 
 	builder = l_dbus_message_builder_new(signal);
@@ -1074,7 +1071,7 @@ static void emit_signals(struct l_idle *idle, void *user_data)
 
 		if (l_queue_find(property_changes_rec->object->instances,
 					match_interface_instance,
-					DBUS_INTERFACE_PROPERTIES)) {
+					L_DBUS_INTERFACE_PROPERTIES)) {
 			signal = build_properties_changed_signal(dbus,
 							property_changes_rec);
 			if (signal)
@@ -1420,7 +1417,7 @@ bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
 		schedule_emit_signals(manager->dbus);
 	}
 
-	if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER)) {
+	if (!strcmp(interface, L_DBUS_INTERFACE_OBJECT_MANAGER)) {
 		manager = l_new(struct object_manager, 1);
 		manager->path = l_strdup(path);
 		manager->dbus = instance->user_data;
@@ -1463,7 +1460,7 @@ bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
 
 	interface_instance_free(instance);
 
-	if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER)) {
+	if (!strcmp(interface, L_DBUS_INTERFACE_OBJECT_MANAGER)) {
 		manager = l_queue_remove_if(tree->object_managers,
 						match_object_manager_path,
 						(char *) path);
diff --git a/ell/dbus.c b/ell/dbus.c
index 153bd0b..730eb74 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -49,10 +49,6 @@
 
 #define DBUS_PATH_DBUS		"/org/freedesktop/DBus"
 
-#define DBUS_INTERFACE_DBUS		"org.freedesktop.DBus"
-#define DBUS_INTERFACE_INTROSPECTABLE	"org.freedesktop.DBus.Introspectable"
-#define DBUS_INTERFACE_OBJECT_MANAGER	"org.freedesktop.DBus.ObjectManager"
-
 #define DBUS_MAXIMUM_MATCH_RULE_LENGTH	1024
 
 enum auth_state {
@@ -408,7 +404,7 @@ static bool auth_write_handler(struct l_io *io, void *user_data)
 		message = l_dbus_message_new_method_call(dbus,
 							DBUS_SERVICE_DBUS,
 							DBUS_PATH_DBUS,
-							DBUS_INTERFACE_DBUS,
+							L_DBUS_INTERFACE_DBUS,
 							"Hello");
 		l_dbus_message_set_arguments(message, "");
 
@@ -1390,7 +1386,7 @@ LIB_EXPORT bool l_dbus_object_manager_enable(struct l_dbus *dbus)
 		return false;
 
 	return _dbus_object_tree_add_interface(dbus->tree, "/",
-						DBUS_INTERFACE_OBJECT_MANAGER,
+						L_DBUS_INTERFACE_OBJECT_MANAGER,
 						dbus);
 }
 
@@ -1471,7 +1467,7 @@ static void dbus1_send_match(struct l_dbus *dbus, const char *rule,
 	message = l_dbus_message_new_method_call(dbus,
 						DBUS_SERVICE_DBUS,
 						DBUS_PATH_DBUS,
-						DBUS_INTERFACE_DBUS,
+						L_DBUS_INTERFACE_DBUS,
 						method);
 
 	l_dbus_message_set_arguments(message, "s", rule);
@@ -1577,7 +1573,7 @@ LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
 
 	data = _dbus1_filter_data_get(dbus, _dbus1_name_owner_changed_filter,
 				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
-				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
+				L_DBUS_INTERFACE_DBUS, "NameOwnerChanged",
 				name,
 				disconnect_func,
 				user_data,
diff --git a/ell/dbus.h b/ell/dbus.h
index 84bf7a4..3f1a5f7 100644
--- a/ell/dbus.h
+++ b/ell/dbus.h
@@ -32,6 +32,11 @@
 extern "C" {
 #endif
 
+#define L_DBUS_INTERFACE_DBUS		"org.freedesktop.DBus"
+#define L_DBUS_INTERFACE_INTROSPECTABLE	"org.freedesktop.DBus.Introspectable"
+#define L_DBUS_INTERFACE_PROPERTIES	"org.freedesktop.DBus.Properties"
+#define L_DBUS_INTERFACE_OBJECT_MANAGER	"org.freedesktop.DBus.ObjectManager"
+
 enum l_dbus_bus {
 	L_DBUS_SYSTEM_BUS,
 	L_DBUS_SESSION_BUS,
-- 
2.5.0

[PATCH 4/5] examples: dbus-service: Use interface name macros.

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

---
 examples/dbus-service.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/examples/dbus-service.c b/examples/dbus-service.c
index 964d735..347faae 100644
--- a/examples/dbus-service.c
+++ b/examples/dbus-service.c
@@ -213,7 +213,7 @@ int main(int argc, char *argv[])
 
 	l_dbus_method_call(dbus, "org.freedesktop.DBus",
 				"/org/freedesktop/DBus",
-				"org.freedesktop.DBus", "RequestName",
+				L_DBUS_INTERFACE_DBUS, "RequestName",
 				request_name_setup,
 				request_name_callback, NULL, NULL);
 
@@ -240,7 +240,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (!l_dbus_object_add_interface(dbus, "/test",
-				"org.freedesktop.DBus.Properties", NULL)) {
+					L_DBUS_INTERFACE_PROPERTIES, NULL)) {
 		l_info("Unable to instantiate the properties interface");
 		test_data_destroy(test);
 		goto cleanup;
-- 
2.5.0

[PATCH 5/5] unit: Update PropertyChanged value check.

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

Don't expect that the value given in the PropertyChanged events is that
taken from the Set or SetProperty call, it's now the value returned by
the getter after the operation which seems more correct too although
in real life they should be the same.
---
 unit/test-dbus-properties.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/unit/test-dbus-properties.c b/unit/test-dbus-properties.c
index 73172c5..cc9efa3 100644
--- a/unit/test-dbus-properties.c
+++ b/unit/test-dbus-properties.c
@@ -624,7 +624,7 @@ static void signal_timeout_callback(struct l_timeout *timeout, void *user_data)
 }
 
 static bool old_signal_received, new_signal_received;
-static const char *signal_string_value;
+static bool signal_success;
 
 static void test_signal_callback(struct l_dbus_message *message)
 {
@@ -646,7 +646,7 @@ static void test_signal_callback(struct l_dbus_message *message)
 		test_assert(!strcmp(property, "String"));
 		test_assert(l_dbus_message_iter_get_variant(&variant, "s",
 								&value));
-		test_assert(!strcmp(value, signal_string_value));
+		test_assert(!strcmp(value, "foo"));
 
 		test_assert(!old_signal_received);
 		old_signal_received = true;
@@ -665,7 +665,7 @@ static void test_signal_callback(struct l_dbus_message *message)
 		test_assert(!strcmp(property, "String"));
 		test_assert(l_dbus_message_iter_get_variant(&variant, "s",
 								&value));
-		test_assert(!strcmp(value, signal_string_value));
+		test_assert(!strcmp(value, "foo"));
 
 		test_assert(!l_dbus_message_iter_next_entry(&changed, &property,
 								&variant));
@@ -682,7 +682,9 @@ static void test_signal_callback(struct l_dbus_message *message)
 	l_timeout_remove(signal_timeout);
 	signal_timeout = NULL;
 
-	if (!strcmp(signal_string_value, "foo")) {
+	if (!signal_success) {
+		signal_success = true;
+
 		/* Now repeat the test for the signal triggered by Set */
 
 		old_signal_received = false;
@@ -692,8 +694,6 @@ static void test_signal_callback(struct l_dbus_message *message)
 							NULL, NULL);
 		test_assert(signal_timeout);
 
-		signal_string_value = "bar";
-
 		call = l_dbus_message_new_method_call(dbus, "org.test", "/test",
 					"org.freedesktop.DBus.Properties",
 					"Set");
@@ -721,8 +721,6 @@ static void test_property_signals(struct l_dbus *dbus, void *test_data)
 						NULL, NULL);
 	test_assert(signal_timeout);
 
-	signal_string_value = "foo";
-
 	test_assert(l_dbus_property_changed(dbus, "/test",
 						"org.test", "String"));
 }
-- 
2.5.0

Re: [PATCH 1/5] dbus: Coalesce InterfacesAdded/Removed signals

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

Hi Andrew,

On 02/17/2016 09:20 PM, Andrew Zaborowski wrote:
> When multiple interfaces are added or removed during the same main loop
> cycle emit one signal per object with all the interfaces added or
> removed.  Move the signals to the idle callback.
> ---
>   ell/dbus-service.c | 337 ++++++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 257 insertions(+), 80 deletions(-)

I looked over these and was fine with them.  I went ahead and applied 
all five patches.  I did tweak some naming afterward to be more 
consistent.  Please eyeball that and let me know if you're okay with those.

One question I have is whether we could save a bit of space by 
coalescing the path and object pointers in the three record objects into 
a single structure.  E.g. go from

struct interface_add_record {
         char *path;
         struct object_node *object;
         struct l_queue *instances;
};

struct interface_remove_record {
         char *path;
         struct object_node *object;
         struct l_queue *interface_names;
};

struct property_change_record {
         char *path;
         struct object_node *object;
         struct interface_instance *instance;
         struct l_queue *properties;
};

to:

struct emit_signal_record {
	char *path;
	struct object_node *object;
	struct l_queue *removed_interface_names;
	struct l_queue *added_interface_instances;
	struct l_queue *changed_properties;
};

?

Regards,
-Denis