[PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy

[PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

g_dbus_client_get_proxy gives the possibilitity to just check if a
proxy exist for the given path and interface pair instead of using
g_dbus_proxy_new which end up creating a proxy if it doesn't exists
which is not always necessary.
---
 gdbus/client.c | 8 ++++----
 gdbus/gdbus.h  | 3 +++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gdbus/client.c b/gdbus/client.c
index c03e3a4..ea56023 100644
--- a/gdbus/client.c
+++ b/gdbus/client.c
@@ -328,8 +328,8 @@ static void get_all_properties(GDBusProxy *proxy)
 	dbus_message_unref(msg);
 }
 
-static GDBusProxy *proxy_lookup(GDBusClient *client, const char *path,
-						const char *interface)
+GDBusProxy *g_dbus_client_get_proxy(GDBusClient *client, const char *path,
+							const char *interface)
 {
 	GList *list;
 
@@ -423,7 +423,7 @@ GDBusProxy *g_dbus_proxy_new(GDBusClient *client, const char *path,
 	if (client == NULL)
 		return NULL;
 
-	proxy = proxy_lookup(client, path, interface);
+	proxy = g_dbus_client_get_proxy(client, path, interface);
 	if (proxy)
 		return g_dbus_proxy_ref(proxy);
 
@@ -841,7 +841,7 @@ static void parse_properties(GDBusClient *client, const char *path,
 	if (g_str_equal(interface, DBUS_INTERFACE_PROPERTIES) == TRUE)
 		return;
 
-	proxy = proxy_lookup(client, path, interface);
+	proxy = g_dbus_client_get_proxy(client, path, interface);
 	if (proxy) {
 		update_properties(proxy, iter);
 		return;
diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 6f5a012..77bd069 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -360,6 +360,9 @@ gboolean g_dbus_client_set_proxy_handlers(GDBusClient *client,
 					GDBusPropertyFunction property_changed,
 					void *user_data);
 
+GDBusProxy *g_dbus_client_get_proxy(GDBusClient *client, const char *path,
+							const char *interface);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.0.1

[PATCH BlueZ 2/7] gdbus: Add g_dbus_proxy_get_client function

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This is convenient as some callbacks don't provide the client which
the proxy belongs.
---
 gdbus/client.c | 8 ++++++++
 gdbus/gdbus.h  | 1 +
 2 files changed, 9 insertions(+)

diff --git a/gdbus/client.c b/gdbus/client.c
index ea56023..3c5784b 100644
--- a/gdbus/client.c
+++ b/gdbus/client.c
@@ -478,6 +478,14 @@ const char *g_dbus_proxy_get_interface(GDBusProxy *proxy)
 	return proxy->interface;
 }
 
+GDBusClient *g_dbus_proxy_get_client(GDBusProxy *proxy)
+{
+	if (proxy == NULL)
+		return NULL;
+
+	return proxy->client;
+}
+
 gboolean g_dbus_proxy_get_property(GDBusProxy *proxy, const char *name,
                                                         DBusMessageIter *iter)
 {
diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 77bd069..83e41c7 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -313,6 +313,7 @@ void g_dbus_proxy_unref(GDBusProxy *proxy);
 
 const char *g_dbus_proxy_get_path(GDBusProxy *proxy);
 const char *g_dbus_proxy_get_interface(GDBusProxy *proxy);
+GDBusClient *g_dbus_proxy_get_client(GDBusProxy *proxy);
 
 gboolean g_dbus_proxy_get_property(GDBusProxy *proxy, const char *name,
 							DBusMessageIter *iter);
-- 
1.8.0.1

[PATCH BlueZ 3/7] unit: Add gdbus/client_get_dict_property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 unit/test-gdbus-client.c | 147 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)

diff --git a/unit/test-gdbus-client.c b/unit/test-gdbus-client.c
index 34c714e..dac0cb5 100644
--- a/unit/test-gdbus-client.c
+++ b/unit/test-gdbus-client.c
@@ -81,6 +81,8 @@ static struct context *create_context(void)
 	/* Avoid D-Bus library calling _exit() before next test finishes. */
 	dbus_connection_set_exit_on_disconnect(context->dbus_conn, FALSE);
 
+	g_dbus_attach_object_manager(context->dbus_conn);
+
 	return context;
 }
 
@@ -184,6 +186,148 @@ static void client_connect_disconnect(void)
 	destroy_context(context);
 }
 
+static void append_variant(DBusMessageIter *iter, int type, void *val)
+{
+	DBusMessageIter value;
+	char sig[2] = { type, '\0' };
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_VARIANT, sig, &value);
+
+	dbus_message_iter_append_basic(&value, type, val);
+
+	dbus_message_iter_close_container(iter, &value);
+}
+
+static void dict_append_entry(DBusMessageIter *dict, const char *key, int type,
+								void *val)
+{
+	DBusMessageIter entry;
+
+	if (type == DBUS_TYPE_STRING) {
+		const char *str = *((const char **) val);
+		if (str == NULL)
+			return;
+	}
+
+	dbus_message_iter_open_container(dict, DBUS_TYPE_DICT_ENTRY,
+							NULL, &entry);
+
+	dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING, &key);
+
+	append_variant(&entry, type, val);
+
+	dbus_message_iter_close_container(dict, &entry);
+}
+
+static gboolean get_dict(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	DBusMessageIter dict;
+	const char *string = "value";
+	dbus_bool_t boolean = TRUE;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+			DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+			DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_VARIANT_AS_STRING
+			DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
+
+	dict_append_entry(&dict, "String", DBUS_TYPE_STRING, &string);
+	dict_append_entry(&dict, "Boolean", DBUS_TYPE_BOOLEAN, &boolean);
+
+	dbus_message_iter_close_container(iter, &dict);
+
+	return TRUE;
+}
+
+static void proxy_get_dict(GDBusProxy *proxy, void *user_data)
+{
+	struct context *context = user_data;
+	DBusMessageIter iter, dict, var1, var2, entry1, entry2;
+	const char *string;
+	dbus_bool_t boolean;
+
+	if (g_test_verbose())
+		g_print("proxy %s found\n",
+					g_dbus_proxy_get_interface(proxy));
+
+	g_assert(g_dbus_proxy_get_property(proxy, "Dict", &iter));
+	g_assert(dbus_message_iter_get_arg_type(&iter) == DBUS_TYPE_ARRAY);
+
+	dbus_message_iter_recurse(&iter, &dict);
+	g_assert(dbus_message_iter_get_arg_type(&dict) ==
+							DBUS_TYPE_DICT_ENTRY);
+
+	dbus_message_iter_recurse(&dict, &entry1);
+	g_assert(dbus_message_iter_get_arg_type(&entry1) == DBUS_TYPE_STRING);
+
+	dbus_message_iter_get_basic(&entry1, &string);
+	g_assert(g_strcmp0(string, "String") == 0);
+
+	dbus_message_iter_next(&entry1);
+	g_assert(dbus_message_iter_get_arg_type(&entry1) == DBUS_TYPE_VARIANT);
+
+	dbus_message_iter_recurse(&entry1, &var1);
+	g_assert(dbus_message_iter_get_arg_type(&var1) == DBUS_TYPE_STRING);
+
+	dbus_message_iter_get_basic(&var1, &string);
+	g_assert(g_strcmp0(string, "value") == 0);
+
+	dbus_message_iter_next(&dict);
+	g_assert(dbus_message_iter_get_arg_type(&dict) ==
+							DBUS_TYPE_DICT_ENTRY);
+
+	dbus_message_iter_recurse(&dict, &entry2);
+	g_assert(dbus_message_iter_get_arg_type(&entry2) == DBUS_TYPE_STRING);
+
+	dbus_message_iter_get_basic(&entry2, &string);
+	g_assert(g_strcmp0(string, "Boolean") == 0);
+
+	dbus_message_iter_next(&entry2);
+	g_assert(dbus_message_iter_get_arg_type(&entry2) == DBUS_TYPE_VARIANT);
+
+	dbus_message_iter_recurse(&entry2, &var2);
+	g_assert(dbus_message_iter_get_arg_type(&var2) == DBUS_TYPE_BOOLEAN);
+
+	dbus_message_iter_get_basic(&var2, &boolean);
+	g_assert(boolean == TRUE);
+
+	dbus_message_iter_next(&dict);
+	g_assert(dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_INVALID);
+
+	g_main_loop_quit(context->main_loop);
+}
+
+static void client_get_dict_property(void)
+{
+	struct context *context = create_context();
+	static const GDBusPropertyTable dict_properties[] = {
+		{ "Dict", "a{sv}", get_dict },
+		{ },
+	};
+
+	if (context == NULL)
+		return;
+
+	g_dbus_register_interface(context->dbus_conn,
+				SERVICE_PATH, SERVICE_NAME,
+				methods, signals, dict_properties,
+				NULL, NULL);
+
+	context->dbus_client = g_dbus_client_new(context->dbus_conn,
+						SERVICE_NAME, SERVICE_PATH);
+
+	g_dbus_client_set_proxy_handlers(context->dbus_client, proxy_get_dict,
+						NULL, NULL,
+						context);
+
+	g_main_loop_run(context->main_loop);
+
+	g_dbus_unregister_interface(context->dbus_conn,
+					SERVICE_PATH, SERVICE_NAME);
+
+	destroy_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -193,5 +337,8 @@ int main(int argc, char *argv[])
 	g_test_add_func("/gdbus/client_connect_disconnect",
 						client_connect_disconnect);
 
+	g_test_add_func("/gdbus/client_get_dict_property",
+						client_get_dict_property);
+
 	return g_test_run();
 }
-- 
1.8.0.1

[PATCH BlueZ 4/7] unit: Add gdbus/client_get_string_property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 unit/test-gdbus-client.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/unit/test-gdbus-client.c b/unit/test-gdbus-client.c
index dac0cb5..bcdd811 100644
--- a/unit/test-gdbus-client.c
+++ b/unit/test-gdbus-client.c
@@ -328,6 +328,68 @@ static void client_get_dict_property(void)
 	destroy_context(context);
 }
 
+static void proxy_get_string(GDBusProxy *proxy, void *user_data)
+{
+	struct context *context = user_data;
+	DBusMessageIter iter;
+	const char *string;
+
+	if (g_test_verbose())
+		g_print("proxy %s found\n",
+					g_dbus_proxy_get_interface(proxy));
+
+	g_assert(g_dbus_proxy_get_property(proxy, "String", &iter));
+	g_assert(dbus_message_iter_get_arg_type(&iter) == DBUS_TYPE_STRING);
+
+	dbus_message_iter_get_basic(&iter, &string);
+	g_assert(g_strcmp0(string, "value") == 0);
+
+	g_dbus_client_unref(context->dbus_client);
+
+	g_main_loop_quit(context->main_loop);
+}
+
+static gboolean get_string(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	const char *string = "value";
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &string);
+
+	return TRUE;
+}
+
+static void client_get_string_property(void)
+{
+	struct context *context = create_context();
+	static const GDBusPropertyTable string_properties[] = {
+		{ "String", "s", get_string },
+		{ },
+	};
+
+	if (context == NULL)
+		return;
+
+	g_dbus_register_interface(context->dbus_conn,
+				SERVICE_PATH, SERVICE_NAME,
+				methods, signals, string_properties,
+				NULL, NULL);
+
+	context->dbus_client = g_dbus_client_new(context->dbus_conn,
+						SERVICE_NAME, SERVICE_PATH);
+
+	g_dbus_client_set_proxy_handlers(context->dbus_client, proxy_get_string,
+						NULL, NULL,
+						context);
+
+	g_main_loop_run(context->main_loop);
+
+	g_dbus_unregister_interface(context->dbus_conn,
+					SERVICE_PATH, SERVICE_NAME);
+
+	destroy_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -337,6 +399,9 @@ int main(int argc, char *argv[])
 	g_test_add_func("/gdbus/client_connect_disconnect",
 						client_connect_disconnect);
 
+	g_test_add_func("/gdbus/client_get_string_property",
+						client_get_string_property);
+
 	g_test_add_func("/gdbus/client_get_dict_property",
 						client_get_dict_property);
 
-- 
1.8.0.1

[PATCH BlueZ 5/7] unit: Add gdbus/client_get_boolean_property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 unit/test-gdbus-client.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/unit/test-gdbus-client.c b/unit/test-gdbus-client.c
index bcdd811..e26dd0c 100644
--- a/unit/test-gdbus-client.c
+++ b/unit/test-gdbus-client.c
@@ -390,6 +390,69 @@ static void client_get_string_property(void)
 	destroy_context(context);
 }
 
+static void proxy_get_boolean(GDBusProxy *proxy, void *user_data)
+{
+	struct context *context = user_data;
+	DBusMessageIter iter;
+	dbus_bool_t value;
+
+	if (g_test_verbose())
+		g_print("proxy %s found\n",
+					g_dbus_proxy_get_interface(proxy));
+
+	g_assert(g_dbus_proxy_get_property(proxy, "Boolean", &iter));
+	g_assert(dbus_message_iter_get_arg_type(&iter) == DBUS_TYPE_BOOLEAN);
+
+	dbus_message_iter_get_basic(&iter, &value);
+	g_assert(value == TRUE);
+
+	g_dbus_client_unref(context->dbus_client);
+
+	g_main_loop_quit(context->main_loop);
+}
+
+static gboolean get_boolean(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	dbus_bool_t value = TRUE;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &value);
+
+	return TRUE;
+}
+
+static void client_get_boolean_property(void)
+{
+	struct context *context = create_context();
+	static const GDBusPropertyTable boolean_properties[] = {
+		{ "Boolean", "b", get_boolean },
+		{ },
+	};
+
+	if (context == NULL)
+		return;
+
+	g_dbus_register_interface(context->dbus_conn,
+				SERVICE_PATH, SERVICE_NAME,
+				methods, signals, boolean_properties,
+				NULL, NULL);
+
+	context->dbus_client = g_dbus_client_new(context->dbus_conn,
+						SERVICE_NAME, SERVICE_PATH);
+
+	g_dbus_client_set_proxy_handlers(context->dbus_client,
+						proxy_get_boolean,
+						NULL, NULL,
+						context);
+
+	g_main_loop_run(context->main_loop);
+
+	g_dbus_unregister_interface(context->dbus_conn,
+					SERVICE_PATH, SERVICE_NAME);
+
+	destroy_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -402,6 +465,9 @@ int main(int argc, char *argv[])
 	g_test_add_func("/gdbus/client_get_string_property",
 						client_get_string_property);
 
+	g_test_add_func("/gdbus/client_get_boolean_property",
+						client_get_boolean_property);
+
 	g_test_add_func("/gdbus/client_get_dict_property",
 						client_get_dict_property);
 
-- 
1.8.0.1

[PATCH BlueZ 6/7] unit: Add gdbus/client_get_array_property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 unit/test-gdbus-client.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/unit/test-gdbus-client.c b/unit/test-gdbus-client.c
index e26dd0c..c3ae4b9 100644
--- a/unit/test-gdbus-client.c
+++ b/unit/test-gdbus-client.c
@@ -453,6 +453,83 @@ static void client_get_boolean_property(void)
 	destroy_context(context);
 }
 
+static void proxy_get_array(GDBusProxy *proxy, void *user_data)
+{
+	struct context *context = user_data;
+	DBusMessageIter iter, entry;
+	const char *value1, *value2;
+
+	if (g_test_verbose())
+		g_print("proxy %s found\n",
+					g_dbus_proxy_get_interface(proxy));
+
+	g_assert(g_dbus_proxy_get_property(proxy, "Array", &iter));
+	g_assert(dbus_message_iter_get_arg_type(&iter) == DBUS_TYPE_ARRAY);
+
+	dbus_message_iter_recurse(&iter, &entry);
+	g_assert(dbus_message_iter_get_arg_type(&entry) == DBUS_TYPE_STRING);
+
+	dbus_message_iter_get_basic(&entry, &value1);
+	g_assert(g_strcmp0(value1, "value1") == 0);
+
+	dbus_message_iter_next(&entry);
+	g_assert(dbus_message_iter_get_arg_type(&entry) == DBUS_TYPE_STRING);
+
+	dbus_message_iter_get_basic(&entry, &value2);
+	g_assert(g_strcmp0(value2, "value2") == 0);
+
+	g_dbus_client_unref(context->dbus_client);
+
+	g_main_loop_quit(context->main_loop);
+}
+
+static gboolean get_array(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	const char *value[2] = { "value1", "value2" };
+	DBusMessageIter array;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "s", &array);
+
+	dbus_message_iter_append_basic(&array, DBUS_TYPE_STRING, &value[0]);
+	dbus_message_iter_append_basic(&array, DBUS_TYPE_STRING, &value[1]);
+
+	dbus_message_iter_close_container(iter, &array);
+
+	return TRUE;
+}
+
+static void client_get_array_property(void)
+{
+	struct context *context = create_context();
+	static const GDBusPropertyTable array_properties[] = {
+		{ "Array", "as", get_array },
+		{ },
+	};
+
+	if (context == NULL)
+		return;
+
+	g_dbus_register_interface(context->dbus_conn,
+				SERVICE_PATH, SERVICE_NAME,
+				methods, signals, array_properties,
+				NULL, NULL);
+
+	context->dbus_client = g_dbus_client_new(context->dbus_conn,
+						SERVICE_NAME, SERVICE_PATH);
+
+	g_dbus_client_set_proxy_handlers(context->dbus_client, proxy_get_array,
+						NULL, NULL,
+						context);
+
+	g_main_loop_run(context->main_loop);
+
+	g_dbus_unregister_interface(context->dbus_conn,
+					SERVICE_PATH, SERVICE_NAME);
+
+	destroy_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -468,6 +545,9 @@ int main(int argc, char *argv[])
 	g_test_add_func("/gdbus/client_get_boolean_property",
 						client_get_boolean_property);
 
+	g_test_add_func("/gdbus/client_get_array_property",
+						client_get_array_property);
+
 	g_test_add_func("/gdbus/client_get_dict_property",
 						client_get_dict_property);
 
-- 
1.8.0.1

[PATCH BlueZ 7/7] unit: Add gdbus/client_get_uint64_property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 unit/test-gdbus-client.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/unit/test-gdbus-client.c b/unit/test-gdbus-client.c
index c3ae4b9..a777fe3 100644
--- a/unit/test-gdbus-client.c
+++ b/unit/test-gdbus-client.c
@@ -530,6 +530,69 @@ static void client_get_array_property(void)
 	destroy_context(context);
 }
 
+static void proxy_get_uint64(GDBusProxy *proxy, void *user_data)
+{
+	struct context *context = user_data;
+	DBusMessageIter iter;
+	guint64 value;
+
+	if (g_test_verbose())
+		g_print("proxy %s found\n",
+					g_dbus_proxy_get_interface(proxy));
+
+	g_assert(g_dbus_proxy_get_property(proxy, "Number", &iter));
+	g_assert(dbus_message_iter_get_arg_type(&iter) == DBUS_TYPE_UINT64);
+
+	dbus_message_iter_get_basic(&iter, &value);
+	g_assert(value == G_MAXUINT64);
+
+	g_dbus_client_unref(context->dbus_client);
+
+	g_main_loop_quit(context->main_loop);
+}
+
+static gboolean get_uint64(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	guint64 value = G_MAXUINT64;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT64, &value);
+
+	return TRUE;
+}
+
+static void client_get_uint64_property(void)
+{
+	struct context *context = create_context();
+	static const GDBusPropertyTable uint64_properties[] = {
+		{ "Number", "t", get_uint64 },
+		{ },
+	};
+
+	if (context == NULL)
+		return;
+
+	g_dbus_register_interface(context->dbus_conn,
+				SERVICE_PATH, SERVICE_NAME,
+				methods, signals, uint64_properties,
+				NULL, NULL);
+
+	context->dbus_client = g_dbus_client_new(context->dbus_conn,
+						SERVICE_NAME, SERVICE_PATH);
+
+	g_dbus_client_set_proxy_handlers(context->dbus_client,
+						proxy_get_uint64,
+						NULL, NULL,
+						context);
+
+	g_main_loop_run(context->main_loop);
+
+	g_dbus_unregister_interface(context->dbus_conn,
+					SERVICE_PATH, SERVICE_NAME);
+
+	destroy_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -545,6 +608,9 @@ int main(int argc, char *argv[])
 	g_test_add_func("/gdbus/client_get_boolean_property",
 						client_get_boolean_property);
 
+	g_test_add_func("/gdbus/client_get_uint64_property",
+						client_get_uint64_property);
+
 	g_test_add_func("/gdbus/client_get_array_property",
 						client_get_array_property);
 
-- 
1.8.0.1

Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy

[Marcel Holtmann]

Hi Luiz,

> g_dbus_client_get_proxy gives the possibilitity to just check if a
> proxy exist for the given path and interface pair instead of using
> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
> which is not always necessary.

why would we do that. You get the proxy via the client callbacks for
proxy created or proxy removed.

The proxy_new method is for dealing with services that do not have
ObjectManager support.

Regards

Marcel

Re: [PATCH BlueZ 2/7] gdbus: Add g_dbus_proxy_get_client function

[Marcel Holtmann]

Hi Luiz,

> This is convenient as some callbacks don't provide the client which
> the proxy belongs.

and I did not add that one on purpose. You should know where your proxy
belongs to and worst case hand it over via user_data.

Regards

Marcel

Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy

[Luiz Augusto von Dentz]

Hi Marcel,

On Fri, Jan 11, 2013 at 10:46 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> g_dbus_client_get_proxy gives the possibilitity to just check if a
>> proxy exist for the given path and interface pair instead of using
>> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
>> which is not always necessary.
>
> why would we do that. You get the proxy via the client callbacks for
> proxy created or proxy removed.

That way we the user don't have to maintain a list of found proxies,
such a list exist and is maintained by GDBusClient.

> The proxy_new method is for dealing with services that do not have
> ObjectManager support.

Yep, so in one way or the other you are already letting the user
application search in the list of proxies maintained by GDBusClient,
the only thing this does is to make the proxy_lookup public so the
client don't have to maintain its how list only to be able to look
back when a proxy point to another, btw the reason proxy_new is not
enough is because it may lead to extra calls when the user application
may just want to bail out if the proxy is not found.


--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/7] gdbus: Add g_dbus_proxy_get_client function

[Luiz Augusto von Dentz]

Hi Marcel,

On Fri, Jan 11, 2013 at 10:47 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> This is convenient as some callbacks don't provide the client which
>> the proxy belongs.
>
> and I did not add that one on purpose. You should know where your proxy
> belongs to and worst case hand it over via user_data.

I understand this as the user_data will always be the client pointer,
or a struct containing it, or you have to use a global variable, is
that what you really want? The use application has given the client
when registering the callbacks, so it is already passing it as
context. Now that being said, Im not saying the design utterly broken,
quite the contrary it work like a charm, it just could be more
convenient and that is what Im trying to achieve with this patch.

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy

[Marcel Holtmann]

Hi Luiz,

> >> g_dbus_client_get_proxy gives the possibilitity to just check if a
> >> proxy exist for the given path and interface pair instead of using
> >> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
> >> which is not always necessary.
> >
> > why would we do that. You get the proxy via the client callbacks for
> > proxy created or proxy removed.
> 
> That way we the user don't have to maintain a list of found proxies,
> such a list exist and is maintained by GDBusClient.

see how bluetoothctl does it. Pretty damn simple. So I am not convinced
that this is a good idea.

> > The proxy_new method is for dealing with services that do not have
> > ObjectManager support.
> 
> Yep, so in one way or the other you are already letting the user
> application search in the list of proxies maintained by GDBusClient,
> the only thing this does is to make the proxy_lookup public so the
> client don't have to maintain its how list only to be able to look
> back when a proxy point to another, btw the reason proxy_new is not
> enough is because it may lead to extra calls when the user application
> may just want to bail out if the proxy is not found.

Still not convinced. What kind of application would this. This seems to
be all half thought trough. I am really failing to see the real purpose
here. Either you go all the way with ObjectManager and do it the right
way or you don't have an object manager, then you need to manually
trigger the get all properties.

Regards

Marcel

Re: [PATCH BlueZ 2/7] gdbus: Add g_dbus_proxy_get_client function

[Marcel Holtmann]

Hi Luiz,

> >> This is convenient as some callbacks don't provide the client which
> >> the proxy belongs.
> >
> > and I did not add that one on purpose. You should know where your proxy
> > belongs to and worst case hand it over via user_data.
> 
> I understand this as the user_data will always be the client pointer,
> or a struct containing it, or you have to use a global variable, is
> that what you really want? The use application has given the client
> when registering the callbacks, so it is already passing it as
> context. Now that being said, Im not saying the design utterly broken,
> quite the contrary it work like a charm, it just could be more
> convenient and that is what Im trying to achieve with this patch.

it is a convenience for one special kind of quick-and-dirty hack that
normal application will never need. I have made these mistakes in the
past and rather have not sneaking up on me again. So lets not do this.

Regards

Marcel

Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy

[Luiz Augusto von Dentz]

Hi Marcel,

On Sun, Jan 13, 2013 at 11:09 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> >> g_dbus_client_get_proxy gives the possibilitity to just check if a
>> >> proxy exist for the given path and interface pair instead of using
>> >> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
>> >> which is not always necessary.
>> >
>> > why would we do that. You get the proxy via the client callbacks for
>> > proxy created or proxy removed.
>>
>> That way we the user don't have to maintain a list of found proxies,
>> such a list exist and is maintained by GDBusClient.
>
> see how bluetoothctl does it. Pretty damn simple. So I am not convinced
> that this is a good idea.
>
>> > The proxy_new method is for dealing with services that do not have
>> > ObjectManager support.
>>
>> Yep, so in one way or the other you are already letting the user
>> application search in the list of proxies maintained by GDBusClient,
>> the only thing this does is to make the proxy_lookup public so the
>> client don't have to maintain its how list only to be able to look
>> back when a proxy point to another, btw the reason proxy_new is not
>> enough is because it may lead to extra calls when the user application
>> may just want to bail out if the proxy is not found.
>
> Still not convinced. What kind of application would this. This seems to
> be all half thought trough. I am really failing to see the real purpose
> here. Either you go all the way with ObjectManager and do it the right
> way or you don't have an object manager, then you need to manually
> trigger the get all properties.

Im actually using these functions in an upcoming patch:

static void register_player(GDBusProxy *proxy)
{
...
client = g_dbus_proxy_get_client(proxy);

if (!g_dbus_proxy_get_property(proxy, "Device", &iter))
    return;

dbus_message_iter_get_basic(&iter, &path);

device = g_dbus_client_get_proxy(client, path, "org.bluez.Device1");
if (device == NULL)
    return;

if (!g_dbus_proxy_get_property(device, "Name", &iter))
    return;
...
static void proxy_added(GDBusProxy *proxy, void *user_data)
{
...
    } else if (!strcmp(interface, BLUEZ_MEDIA_PLAYER_INTERFACE)) {
         printf("Bluetooth Player %s found\n", g_dbus_proxy_get_path(proxy));
         register_player(proxy);
   }
...

With this I don't have to keep any list of found devices what so ever,
if I need a proxy I can just query it via g_dbus_client_get_proxy. Now
compare this to what bluetoothctl does:

static void proxy_added(GDBusProxy *proxy, void *user_data)
{
...
    if (!strcmp(interface, "org.bluez.Device1")) {
        if (device_is_child(proxy, default_ctrl) == TRUE) {
            dev_list = g_list_append(dev_list, proxy);

            print_device(proxy, "NEW");
    } else if (!strcmp(interface, "org.bluez.Adapter1")) {
            ctrl_list = g_list_append(ctrl_list, proxy);

            if (!default_ctrl)
                default_ctrl = proxy;

             print_adapter(proxy, "NEW");
...

So you are keeping the list of proxies while GDBusClient keeps them
too, now it maybe useful for bluetoothctl but for mpris-tool it is
completely useless, why would I keep a proxy of a device that doesn't
contain any player, specially while discovering devices this would
just populate with useless temporary devices.

Btw, with these functions we could actually have the following changes
to device_is_child:

diff --git a/client/main.c b/client/main.c
index 9a927a8..1cd6d10 100644
--- a/client/main.c
+++ b/client/main.c
@@ -224,8 +224,10 @@ static void print_uuids(GDBusProxy *proxy)

 static gboolean device_is_child(GDBusProxy *device, GDBusProxy *master)
 {
+       GDBusClient *client;
+       GDBusProxy *proxy;
        DBusMessageIter iter;
-       const char *adapter, *path;
+       const char *path;

        if (!master)
                return FALSE;
@@ -233,13 +235,13 @@ static gboolean device_is_child(GDBusProxy
*device, GDBusProxy *master)
        if (g_dbus_proxy_get_property(device, "Adapter", &iter) == FALSE)
                return FALSE;

-       dbus_message_iter_get_basic(&iter, &adapter);
-       path = g_dbus_proxy_get_path(master);
+       dbus_message_iter_get_basic(&iter, &path);

-       if (!strcmp(path, adapter))
-               return TRUE;
+       client = g_dbus_proxy_get_client(device);

-       return FALSE;
+       proxy = g_dbus_client_get_proxy(client, path, "org.bluez.Adapter1");
+
+       return proxy == master ? TRUE : FALSE;
 }

So we compare the proxies not the path alone.

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy

[Marcel Holtmann]

Hi Luiz,

> >> >> g_dbus_client_get_proxy gives the possibilitity to just check if a
> >> >> proxy exist for the given path and interface pair instead of using
> >> >> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
> >> >> which is not always necessary.
> >> >
> >> > why would we do that. You get the proxy via the client callbacks for
> >> > proxy created or proxy removed.
> >>
> >> That way we the user don't have to maintain a list of found proxies,
> >> such a list exist and is maintained by GDBusClient.
> >
> > see how bluetoothctl does it. Pretty damn simple. So I am not convinced
> > that this is a good idea.
> >
> >> > The proxy_new method is for dealing with services that do not have
> >> > ObjectManager support.
> >>
> >> Yep, so in one way or the other you are already letting the user
> >> application search in the list of proxies maintained by GDBusClient,
> >> the only thing this does is to make the proxy_lookup public so the
> >> client don't have to maintain its how list only to be able to look
> >> back when a proxy point to another, btw the reason proxy_new is not
> >> enough is because it may lead to extra calls when the user application
> >> may just want to bail out if the proxy is not found.
> >
> > Still not convinced. What kind of application would this. This seems to
> > be all half thought trough. I am really failing to see the real purpose
> > here. Either you go all the way with ObjectManager and do it the right
> > way or you don't have an object manager, then you need to manually
> > trigger the get all properties.
> 
> Im actually using these functions in an upcoming patch:
> 
> static void register_player(GDBusProxy *proxy)
> {
> ...
> client = g_dbus_proxy_get_client(proxy);
> 
> if (!g_dbus_proxy_get_property(proxy, "Device", &iter))
>     return;
> 
> dbus_message_iter_get_basic(&iter, &path);
> 
> device = g_dbus_client_get_proxy(client, path, "org.bluez.Device1");
> if (device == NULL)
>     return;
> 
> if (!g_dbus_proxy_get_property(device, "Name", &iter))
>     return;
> ...
> static void proxy_added(GDBusProxy *proxy, void *user_data)
> {
> ...
>     } else if (!strcmp(interface, BLUEZ_MEDIA_PLAYER_INTERFACE)) {
>          printf("Bluetooth Player %s found\n", g_dbus_proxy_get_path(proxy));
>          register_player(proxy);
>    }
> ...
> 
> With this I don't have to keep any list of found devices what so ever,
> if I need a proxy I can just query it via g_dbus_client_get_proxy. Now
> compare this to what bluetoothctl does:
> 
> static void proxy_added(GDBusProxy *proxy, void *user_data)
> {
> ...
>     if (!strcmp(interface, "org.bluez.Device1")) {
>         if (device_is_child(proxy, default_ctrl) == TRUE) {
>             dev_list = g_list_append(dev_list, proxy);
> 
>             print_device(proxy, "NEW");
>     } else if (!strcmp(interface, "org.bluez.Adapter1")) {
>             ctrl_list = g_list_append(ctrl_list, proxy);
> 
>             if (!default_ctrl)
>                 default_ctrl = proxy;
> 
>              print_adapter(proxy, "NEW");
> ...
> 
> So you are keeping the list of proxies while GDBusClient keeps them
> too, now it maybe useful for bluetoothctl but for mpris-tool it is
> completely useless, why would I keep a proxy of a device that doesn't
> contain any player, specially while discovering devices this would
> just populate with useless temporary devices.

bluetoothctl only keeps the list of proxies that contain an interface it
is interested in. And the same could be done for mpris-tool. If you get
an object path with a player interface, you remember it, otherwise you
just ignore it.

> Btw, with these functions we could actually have the following changes
> to device_is_child:
> 
> diff --git a/client/main.c b/client/main.c
> index 9a927a8..1cd6d10 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -224,8 +224,10 @@ static void print_uuids(GDBusProxy *proxy)
> 
>  static gboolean device_is_child(GDBusProxy *device, GDBusProxy *master)
>  {
> +       GDBusClient *client;
> +       GDBusProxy *proxy;
>         DBusMessageIter iter;
> -       const char *adapter, *path;
> +       const char *path;
> 
>         if (!master)
>                 return FALSE;
> @@ -233,13 +235,13 @@ static gboolean device_is_child(GDBusProxy
> *device, GDBusProxy *master)
>         if (g_dbus_proxy_get_property(device, "Adapter", &iter) == FALSE)
>                 return FALSE;
> 
> -       dbus_message_iter_get_basic(&iter, &adapter);
> -       path = g_dbus_proxy_get_path(master);
> +       dbus_message_iter_get_basic(&iter, &path);
> 
> -       if (!strcmp(path, adapter))
> -               return TRUE;
> +       client = g_dbus_proxy_get_client(device);

And this I have single client, I could just make this global. So I am
still not buying into this one.

> -       return FALSE;
> +       proxy = g_dbus_client_get_proxy(client, path, "org.bluez.Adapter1");
> +
> +       return proxy == master ? TRUE : FALSE;
>  }

Don't see how this makes it simpler code. Or ends up in less
instructions for that matter.

Regards

Marcel