[BlueZ RFC 0/5] gdbus: Add support for DBus.Properties interface

[BlueZ RFC 0/5] gdbus: Add support for DBus.Properties interface

[Lucas De Marchi]

Hey,

This series adds DBus.Properties interface to gdbus - it's still missing the
PropertiesChanged signal, but we'd like to share early this code to get feedback.

See http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties

The intention is to replace the GetProperties method per-interface and use this
interface per-object instead. It adds some nice things like getting properties
separately, too.

This is just the infrastructure needed in gdbus. Henrique (that's working with
me on this) will send soon his changes on some of the interfaces so you can take
a look how it will be after these patches. After that we can work on finishing the
conversion for the other interfaces.


If you are going to test this series, it is intended to be applied on top of my
previous series improving gdbus introspection.


Lucas De Marchi (5):
  gdbus: add skeleton of DBus.Properties interface
  gdbus: implement DBus.Properties.Get method
  gdbus: implement DBus.Properties.GetAll method
  gdbus: implement DBus.Properties.Set method
  gdbus: add properties into interface introspection

 gdbus/gdbus.h  |   21 +++++-
 gdbus/object.c |  224 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 243 insertions(+), 2 deletions(-)

-- 
1.7.10

[BlueZ RFC 1/5] gdbus: add skeleton of DBus.Properties interface

[Lucas De Marchi]

This interface is responsible for handling properties of all objects in
a given path. Right now it only registers itself, doing nothing useful.
A conversion to this new layout will be done by subsequent patches.

org.freedesktop.org.DBus.Properties spec can be found at
http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties
---
 gdbus/object.c |   34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/gdbus/object.c b/gdbus/object.c
index 540d1fc..31113e0 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -554,6 +554,36 @@ static const GDBusMethodTable introspect_methods[] = {
 	{ }
 };
 
+static DBusMessage *properties_get(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	return NULL;
+}
+
+static DBusMessage *properties_get_all(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	return NULL;
+}
+
+static DBusMessage *properties_set(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	return NULL;
+}
+
+static const GDBusMethodTable properties_methods[] = {
+	{ "Get", "s[interface_name]s[property_name]", "v[value]", properties_get },
+	{ "Set", "s[interface_name]s[property_name]v[value]", "", properties_set },
+	{ "GetAll", "s[interface_name]", "a{sv}[props]", properties_get_all },
+	{ }
+};
+
+static const GDBusSignalTable properties_signals[] = {
+	{ "PropertiesChanged", "s[interface_name]a{sv}[changed_properties]as[invalidated_properties]" },
+	{ }
+};
+
 static char *undecorate_signature(const char *src)
 {
 	GString *dst = g_string_new(NULL);
@@ -679,6 +709,9 @@ static struct generic_data *object_path_ref(DBusConnection *connection,
 	add_interface(data, DBUS_INTERFACE_INTROSPECTABLE,
 			introspect_methods, NULL, NULL, data, NULL);
 
+	add_interface(data, DBUS_INTERFACE_PROPERTIES, properties_methods,
+					properties_signals, NULL, data, NULL);
+
 	return data;
 }
 
@@ -721,6 +754,7 @@ static void object_path_unref(DBusConnection *connection, const char *path)
 		return;
 
 	remove_interface(data, DBUS_INTERFACE_INTROSPECTABLE);
+	remove_interface(data, DBUS_INTERFACE_PROPERTIES);
 
 	invalidate_parent_data(connection, path);
 
-- 
1.7.10

[BlueZ RFC 2/5] gdbus: implement DBus.Properties.Get method

[Lucas De Marchi]

---
 gdbus/gdbus.h  |   15 +++++++++++--
 gdbus/object.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index a5843e0..359e8a0 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -55,6 +55,15 @@ typedef void (* GDBusDestroyFunction) (void *user_data);
 typedef DBusMessage * (* GDBusMethodFunction) (DBusConnection *connection,
 					DBusMessage *message, void *user_data);
 
+typedef struct GDBusPropertyTable GDBusPropertyTable;
+typedef gboolean (*GDBusPropertyGetter)(DBusConnection *connection,
+					DBusMessage *message,
+					const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data);
+
+typedef gboolean (*GDBusPropertyExists)(const GDBusPropertyTable *property,
+								void *data);
+
 typedef guint32 GDBusPendingReply;
 
 typedef void (* GDBusSecurityFunction) (DBusConnection *connection,
@@ -97,11 +106,13 @@ typedef struct {
 	GDBusSignalFlags flags;
 } GDBusSignalTable;
 
-typedef struct {
+struct GDBusPropertyTable {
 	const char *name;
 	const char *type;
 	GDBusPropertyFlags flags;
-} GDBusPropertyTable;
+	GDBusPropertyGetter get;
+	GDBusPropertyExists exists;
+};
 
 typedef struct {
 	unsigned int privilege;
diff --git a/gdbus/object.c b/gdbus/object.c
index 31113e0..0259f46 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -554,10 +554,74 @@ static const GDBusMethodTable introspect_methods[] = {
 	{ }
 };
 
+static inline const GDBusPropertyTable *find_property(const GDBusPropertyTable *properties,
+							const char *property_name)
+{
+	const GDBusPropertyTable *p;
+
+	for (p = properties; p && p->name; p++) {
+		if (strcmp(property_name, p->name) == 0)
+			return p;
+	}
+
+	return NULL;
+}
+
 static DBusMessage *properties_get(DBusConnection *connection,
 					DBusMessage *message, void *user_data)
 {
-	return NULL;
+	struct generic_data *data = user_data;
+	struct interface_data *iface;
+	const GDBusPropertyTable *property;
+	const char *interface_name, *property_name;
+	DBusMessageIter iter, value;
+	DBusMessage *reply;
+
+	if (!dbus_message_get_args(message, NULL,
+					DBUS_TYPE_STRING, &interface_name,
+					DBUS_TYPE_STRING, &property_name,
+					DBUS_TYPE_INVALID))
+		return NULL;
+
+	iface = find_interface(data->interfaces, interface_name);
+	if (iface == NULL)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+				"No such interface '%s'", interface_name);
+
+	property = find_property(iface->properties, property_name);
+	if (property == NULL)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+				"No such property '%s'", property_name);
+
+	if (property->exists != NULL &&
+			!property->exists(property, iface->user_data))
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+						"No such property '%s'",
+						property_name);
+
+	if (property->get == NULL)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+					"Property '%s' is not readable",
+					property_name);
+
+	reply = dbus_message_new_method_return(message);
+	if (reply == NULL)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
+						property->type, &value);
+
+	if (!property->get(connection, message, property, &value,
+							iface->user_data)) {
+		dbus_message_iter_abandon_container(&iter, &value);
+		dbus_message_unref(reply);
+		return NULL;
+	}
+
+	dbus_message_iter_close_container(&iter, &value);
+
+	return reply;
 }
 
 static DBusMessage *properties_get_all(DBusConnection *connection,
-- 
1.7.10

[BlueZ RFC 3/5] gdbus: implement DBus.Properties.GetAll method

[Lucas De Marchi]

---
 gdbus/object.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index 0259f46..5a1db15 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -627,7 +627,63 @@ static DBusMessage *properties_get(DBusConnection *connection,
 static DBusMessage *properties_get_all(DBusConnection *connection,
 					DBusMessage *message, void *user_data)
 {
-	return NULL;
+	struct generic_data *data = user_data;
+	struct interface_data *iface;
+	const GDBusPropertyTable *p;
+	const char *interface_name;
+	DBusMessageIter iter, dict;
+	DBusMessage *reply;
+
+	if (!dbus_message_get_args(message, NULL,
+					DBUS_TYPE_STRING, &interface_name,
+					DBUS_TYPE_INVALID))
+		return NULL;
+
+	iface = find_interface(data->interfaces, interface_name);
+	if (iface == NULL)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+				"No such interface '%s'", interface_name);
+
+	reply = dbus_message_new_method_return(message);
+	if (reply == NULL)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+	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);
+
+	for (p = iface->properties; p && p->name; p++) {
+		DBusMessageIter entry, value;
+
+		if (p->get == NULL)
+			continue;
+
+		if (p->exists != NULL && !p->exists(p, iface->user_data))
+			continue;
+
+		dbus_message_iter_open_container(&dict, DBUS_TYPE_DICT_ENTRY,
+								NULL, &entry);
+		dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
+								p->name);
+		dbus_message_iter_open_container(&entry, DBUS_TYPE_VARIANT,
+							p->type, &value);
+
+		if (!p->get(connection, message, p, &value, iface->user_data)) {
+			dbus_message_iter_abandon_container(&entry, &value);
+			dbus_message_iter_abandon_container(&dict, &entry);
+			dbus_message_unref(reply);
+			return NULL;
+		}
+
+		dbus_message_iter_close_container(&entry, &value);
+		dbus_message_iter_close_container(&dict, &entry);
+	}
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
 }
 
 static DBusMessage *properties_set(DBusConnection *connection,
-- 
1.7.10

[BlueZ RFC 4/5] gdbus: implement DBus.Properties.Set method

[Lucas De Marchi]

---
 gdbus/gdbus.h  |    6 ++++++
 gdbus/object.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 359e8a0..6fde16d 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -61,6 +61,11 @@ typedef gboolean (*GDBusPropertyGetter)(DBusConnection *connection,
 					const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *data);
 
+typedef DBusMessage *(*GDBusPropertySetter)(DBusConnection *connection,
+					DBusMessage *message,
+					const GDBusPropertyTable *property,
+					DBusMessageIter *value, void *data);
+
 typedef gboolean (*GDBusPropertyExists)(const GDBusPropertyTable *property,
 								void *data);
 
@@ -111,6 +116,7 @@ struct GDBusPropertyTable {
 	const char *type;
 	GDBusPropertyFlags flags;
 	GDBusPropertyGetter get;
+	GDBusPropertySetter set;
 	GDBusPropertyExists exists;
 };
 
diff --git a/gdbus/object.c b/gdbus/object.c
index 5a1db15..ff5380a 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -689,7 +689,61 @@ static DBusMessage *properties_get_all(DBusConnection *connection,
 static DBusMessage *properties_set(DBusConnection *connection,
 					DBusMessage *message, void *user_data)
 {
-	return NULL;
+	struct generic_data *data = user_data;
+	DBusMessageIter iter, sub;
+	struct interface_data *iface;
+	const GDBusPropertyTable *property;
+	const char *property_name, *interface_name;
+
+	if (!dbus_message_iter_init(message, &iter))
+		return NULL;
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+					"Invalid argument type: '%c'",
+					dbus_message_iter_get_arg_type(&iter));
+
+	dbus_message_iter_get_basic(&iter, &property_name);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+					"Invalid argument type: '%c'",
+					dbus_message_iter_get_arg_type(&iter));
+
+	dbus_message_iter_get_basic(&iter, &interface_name);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+					"Invalid argument type: '%c'",
+					dbus_message_iter_get_arg_type(&iter));
+
+	dbus_message_iter_recurse(&iter, &sub);
+
+	iface = find_interface(data->interfaces, interface_name);
+	if (iface == NULL)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+				"No such interface '%s'", interface_name);
+
+	property = find_property(iface->properties, property_name);
+	if (property == NULL)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+				"No such property '%s'", property_name);
+
+	if (property->set == NULL)
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+					"Property '%s' is not writable",
+					property_name);
+
+	if (property->exists != NULL &&
+			!property->exists(property, iface->user_data))
+		return g_dbus_create_error(message, DBUS_ERROR_INVALID_ARGS,
+						"No such property '%s'",
+						property_name);
+
+	return property->set(connection, message, property, &sub,
+							iface->user_data);
 }
 
 static const GDBusMethodTable properties_methods[] = {
-- 
1.7.10

[BlueZ RFC 5/5] gdbus: add properties into interface introspection

[Lucas De Marchi]

---
 gdbus/object.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gdbus/object.c b/gdbus/object.c
index ff5380a..701352c 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -158,6 +158,7 @@ static void generate_interface_xml(GString *gstr, struct interface_data *iface)
 {
 	const GDBusMethodTable *method;
 	const GDBusSignalTable *signal;
+	const GDBusPropertyTable *property;
 
 	for (method = iface->methods; method && method->name; method++) {
 		gboolean deprecated = method->flags &
@@ -205,6 +206,21 @@ static void generate_interface_xml(GString *gstr, struct interface_data *iface)
 			g_string_append_printf(gstr, "\t\t</signal>\n");
 		}
 	}
+
+	for (property = iface->properties; property && property->name; property++) {
+		gboolean deprecated = property->flags &
+					G_DBUS_PROPERTY_FLAG_DEPRECATED;
+
+		g_string_append_printf(gstr, "\t\t<property name=\"%s\" type=\"%s\" access=\"%s%s\"",
+						property->name, property->type,
+						property->get ? "read" : "",
+						property->set ? "write" : "");
+
+		if (!deprecated)
+			g_string_append_printf(gstr, "/>\n");
+		else
+			g_string_append_printf(gstr, ">\n\t\t\t<annotation name=\"org.freedesktop.DBus.Deprecated\" value=\"true\"/>\n\t\t</property>\n");
+	}
 }
 
 static void generate_introspection_xml(DBusConnection *conn,
-- 
1.7.10

Re: [BlueZ RFC 3/5] gdbus: implement DBus.Properties.GetAll method

[Luiz Augusto von Dentz]

Hi Lucas,

On Sat, Apr 28, 2012 at 12:25 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> +               if (p->exists != NULL && !p->exists(p, iface->user_data))
> +                       continue;
> +
> +               dbus_message_iter_open_container(&dict, DBUS_TYPE_DICT_ENTRY,
> +                                                               NULL, &entry);
> +               dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
> +                                                               p->name);
> +               dbus_message_iter_open_container(&entry, DBUS_TYPE_VARIANT,
> +                                                       p->type, &value);
> +
> +               if (!p->get(connection, message, p, &value, iface->user_data)) {
> +                       dbus_message_iter_abandon_container(&entry, &value);
> +                       dbus_message_iter_abandon_container(&dict, &entry);
> +                       dbus_message_unref(reply);
> +                       return NULL;
> +               }

I would do this a little different, make the callback return the value
(void *) so you don't even need to create any container if it returns
NULL, if the value is set then you create the container and use the
registered type to add the value to the container, obviously this is
limited to values that doesn't require another container but IMO that
should not be the case of any of our properties.


-- 
Luiz Augusto von Dentz

Re: [BlueZ RFC 3/5] gdbus: implement DBus.Properties.GetAll method

[Lucas De Marchi]

On Sun, Apr 29, 2012 at 8:22 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lucas,
>
> On Sat, Apr 28, 2012 at 12:25 AM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> +               if (p->exists != NULL && !p->exists(p, iface->user_data))
>> +                       continue;
>> +
>> +               dbus_message_iter_open_container(&dict, DBUS_TYPE_DICT_ENTRY,
>> +                                                               NULL, &entry);
>> +               dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
>> +                                                               p->name);
>> +               dbus_message_iter_open_container(&entry, DBUS_TYPE_VARIANT,
>> +                                                       p->type, &value);
>> +
>> +               if (!p->get(connection, message, p, &value, iface->user_data)) {
>> +                       dbus_message_iter_abandon_container(&entry, &value);
>> +                       dbus_message_iter_abandon_container(&dict, &entry);
>> +                       dbus_message_unref(reply);
>> +                       return NULL;
>> +               }
>
> I would do this a little different, make the callback return the value
> (void *) so you don't even need to create any container if it returns
> NULL, if the value is set then you create the container and use the
> registered type to add the value to the container, obviously this is
> limited to values that doesn't require another container but IMO that
> should not be the case of any of our properties.

Yeah, I thought about that. But as you said it's limited to the basic
types. See the example Henrique sent for "ao". And there's the more
difficult one: a property that is a dictionary.

To cover these more complicated cases, we would need to check if it's
a property with basic signature and act differently based on that.


Lucas De Marchi