[PATCH BlueZ 1/6] gdbus: Introduce G_DBUS_METHOD_FLAG_EXPERIMENTAL

[PATCH BlueZ 1/6] gdbus: Introduce G_DBUS_METHOD_FLAG_EXPERIMENTAL

[Luiz Augusto von Dentz]

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

This flag can be used to mark methods as experimental, the marked
methods with this flag can be enabled by setting the environment variable
GDBUS_EXPERIMENTAL=1
---
 gdbus/gdbus.h  | 21 ++++++++++++++++++---
 gdbus/object.c | 17 +++++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 0e5c012..64f5d62 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -89,9 +89,10 @@ typedef void (* GDBusSecurityFunction) (DBusConnection *connection,
 						GDBusPendingReply pending);
 
 enum GDBusMethodFlags {
-	G_DBUS_METHOD_FLAG_DEPRECATED = (1 << 0),
-	G_DBUS_METHOD_FLAG_NOREPLY    = (1 << 1),
-	G_DBUS_METHOD_FLAG_ASYNC      = (1 << 2),
+	G_DBUS_METHOD_FLAG_DEPRECATED	= (1 << 0),
+	G_DBUS_METHOD_FLAG_NOREPLY	= (1 << 1),
+	G_DBUS_METHOD_FLAG_ASYNC	= (1 << 2),
+	G_DBUS_METHOD_FLAG_EXPERIMENTAL	= (1 << 3),
 };
 
 enum GDBusSignalFlags {
@@ -173,6 +174,20 @@ struct GDBusSecurityTable {
 	.function = _function, \
 	.flags = G_DBUS_METHOD_FLAG_ASYNC | G_DBUS_METHOD_FLAG_DEPRECATED
 
+#define GDBUS_EXPERIMENTAL_METHOD(_name, _in_args, _out_args, _function) \
+	.name = _name, \
+	.in_args = _in_args, \
+	.out_args = _out_args, \
+	.function = _function, \
+	.flags = G_DBUS_METHOD_FLAG_EXPERIMENTAL
+
+#define GDBUS_EXPERIMENTAL_ASYNC_METHOD(_name, _in_args, _out_args, _function) \
+	.name = _name, \
+	.in_args = _in_args, \
+	.out_args = _out_args, \
+	.function = _function, \
+	.flags = G_DBUS_METHOD_FLAG_ASYNC | G_DBUS_METHOD_FLAG_EXPERIMENTAL
+
 #define GDBUS_NOREPLY_METHOD(_name, _in_args, _out_args, _function) \
 	.name = _name, \
 	.in_args = _in_args, \
diff --git a/gdbus/object.c b/gdbus/object.c
index 776d35e..180d48c 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -129,6 +129,14 @@ static void generate_interface_xml(GString *gstr, struct interface_data *iface)
 						G_DBUS_METHOD_FLAG_DEPRECATED;
 		gboolean noreply = method->flags &
 						G_DBUS_METHOD_FLAG_NOREPLY;
+		gboolean experimental = method->flags &
+					G_DBUS_METHOD_FLAG_EXPERIMENTAL;
+
+		if (experimental) {
+			const char *env = g_getenv("GDBUS_EXPERIMENTAL");
+			if (g_strcmp0(env, "1") != 0)
+				return;
+		}
 
 		if (!deprecated && !noreply &&
 				!(method->in_args && method->in_args->name) &&
@@ -1022,10 +1030,19 @@ static DBusHandlerResult generic_message(DBusConnection *connection,
 
 	for (method = iface->methods; method &&
 			method->name && method->function; method++) {
+		gboolean experimental = method->flags &
+					G_DBUS_METHOD_FLAG_EXPERIMENTAL;
+
 		if (dbus_message_is_method_call(message, iface->name,
 							method->name) == FALSE)
 			continue;
 
+		if (experimental) {
+			const char *env = g_getenv("GDBUS_EXPERIMENTAL");
+			if (g_strcmp0(env, "1") != 0)
+				return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+		}
+
 		if (g_dbus_args_have_signature(method->in_args,
 							message) == FALSE)
 			continue;
-- 
1.7.11.7

[PATCH BlueZ 2/6] media: Enable RegisterPlayer and UnregisterPlayer methods as experimental

[Luiz Augusto von Dentz]

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

---
 profiles/audio/media.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index f728460..e4206e3 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -868,7 +868,6 @@ static DBusMessage *unregister_endpoint(DBusConnection *conn, DBusMessage *msg,
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
-#if 0
 static struct media_player *media_adapter_find_player(
 						struct media_adapter *adapter,
 						const char *sender,
@@ -1533,7 +1532,6 @@ static DBusMessage *unregister_player(DBusConnection *conn, DBusMessage *msg,
 
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
-#endif
 
 static const GDBusMethodTable media_methods[] = {
 	{ GDBUS_METHOD("RegisterEndpoint",
@@ -1541,14 +1539,12 @@ static const GDBusMethodTable media_methods[] = {
 		NULL, register_endpoint) },
 	{ GDBUS_METHOD("UnregisterEndpoint",
 		GDBUS_ARGS({ "endpoint", "o" }), NULL, unregister_endpoint) },
-#if 0
-	{ GDBUS_METHOD("RegisterPlayer",
+	{ GDBUS_EXPERIMENTAL_METHOD("RegisterPlayer",
 		GDBUS_ARGS({ "player", "o" }, { "properties", "a{sv}" },
 						{ "metadata", "a{sv}" }),
 		NULL, register_player) },
-	{ GDBUS_METHOD("UnregisterPlayer",
+	{ GDBUS_EXPERIMENTAL_METHOD("UnregisterPlayer",
 		GDBUS_ARGS({ "player", "o" }), NULL, unregister_player) },
-#endif
 	{ },
 };
 
@@ -1559,10 +1555,8 @@ static void path_free(void *data)
 	while (adapter->endpoints)
 		release_endpoint(adapter->endpoints->data);
 
-#if 0
 	while (adapter->players)
 		media_player_destroy(adapter->players->data);
-#endif
 
 	adapters = g_slist_remove(adapters, adapter);
 
-- 
1.7.11.7

[PATCH BlueZ 3/6] gdbus: Introduce GDBusInterfaceFlags

[Luiz Augusto von Dentz]

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

The flags should be passed in g_dbus_register_interface_with_flags,
currently there only one flag which is G_DBUS_INTERFACE_FLAG_EXPERIMENTAL
which works similarly to G_DBUS_METHOD_FLAG_EXPERIMENTAL but for the
whole interface.
---
 gdbus/gdbus.h  | 13 +++++++++++++
 gdbus/object.c | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 64f5d62..f49d9f0 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -31,6 +31,7 @@ extern "C" {
 #include <dbus/dbus.h>
 #include <glib.h>
 
+typedef enum GDBusInterfaceFlags GDBusInterfaceFlags;
 typedef enum GDBusMethodFlags GDBusMethodFlags;
 typedef enum GDBusSignalFlags GDBusSignalFlags;
 typedef enum GDBusPropertyFlags GDBusPropertyFlags;
@@ -88,6 +89,10 @@ typedef void (* GDBusSecurityFunction) (DBusConnection *connection,
 						gboolean interaction,
 						GDBusPendingReply pending);
 
+enum GDBusInterfaceFlags {
+	G_DBUS_INTERFACE_FLAG_EXPERIMENTAL = (1 << 0),
+};
+
 enum GDBusMethodFlags {
 	G_DBUS_METHOD_FLAG_DEPRECATED	= (1 << 0),
 	G_DBUS_METHOD_FLAG_NOREPLY	= (1 << 1),
@@ -211,6 +216,14 @@ gboolean g_dbus_register_interface(DBusConnection *connection,
 					const GDBusPropertyTable *properties,
 					void *user_data,
 					GDBusDestroyFunction destroy);
+gboolean g_dbus_register_interface_with_flags(DBusConnection *connection,
+					const char *path, const char *name,
+					GDBusInterfaceFlags flags,
+					const GDBusMethodTable *methods,
+					const GDBusSignalTable *signals,
+					const GDBusPropertyTable *properties,
+					void *user_data,
+					GDBusDestroyFunction destroy);
 gboolean g_dbus_unregister_interface(DBusConnection *connection,
 					const char *path, const char *name);
 
diff --git a/gdbus/object.c b/gdbus/object.c
index 180d48c..510763a 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -1350,6 +1350,26 @@ gboolean g_dbus_register_interface(DBusConnection *connection,
 	return TRUE;
 }
 
+gboolean g_dbus_register_interface_with_flags(DBusConnection *connection,
+					const char *path, const char *name,
+					GDBusInterfaceFlags flags,
+					const GDBusMethodTable *methods,
+					const GDBusSignalTable *signals,
+					const GDBusPropertyTable *properties,
+					void *user_data,
+					GDBusDestroyFunction destroy)
+{
+	if (flags & G_DBUS_INTERFACE_FLAG_EXPERIMENTAL) {
+		const char *env = g_getenv("GDBUS_EXPERIMENTAL");
+		if (g_strcmp0(env, "1") != 0)
+			return FALSE;
+	}
+
+	return g_dbus_register_interface(connection, path, name, methods,
+						signals, properties, user_data,
+						destroy);
+}
+
 gboolean g_dbus_unregister_interface(DBusConnection *connection,
 					const char *path, const char *name)
 {
-- 
1.7.11.7

[PATCH BlueZ 4/6] gdbus: Call check_signals when sending signals with g_dbus_send_message

[Luiz Augusto von Dentz]

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

If message passed to g_dbus_send_message is a signal verify if it is a
valid and there really exists an interface with respective signal name.
---
 gdbus/object.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdbus/object.c b/gdbus/object.c
index 510763a..6951171 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -1475,6 +1475,15 @@ gboolean g_dbus_send_message(DBusConnection *connection, DBusMessage *message)
 
 	if (dbus_message_get_type(message) == DBUS_MESSAGE_TYPE_METHOD_CALL)
 		dbus_message_set_no_reply(message, TRUE);
+	else {
+		const char *path = dbus_message_get_path(message);
+		const char *interface = dbus_message_get_interface(message);
+		const char *name = dbus_message_get_member(message);
+		const GDBusArgInfo *args;
+
+		if (!check_signal(connection, path, interface, name, &args))
+			return FALSE;
+	}
 
 	result = dbus_connection_send(connection, message, NULL);
 
-- 
1.7.11.7

[PATCH BlueZ 5/6] player: Enable MediaPlayer1 interface as experimental

[Luiz Augusto von Dentz]

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

This enable MediaPlayer1 when GDBUS_EXPERIMENTAL=1
---
 profiles/audio/player.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/profiles/audio/player.c b/profiles/audio/player.c
index 005d0d1..60c7a8a 100644
--- a/profiles/audio/player.c
+++ b/profiles/audio/player.c
@@ -298,9 +298,9 @@ struct media_player *media_player_controller_create(const char *path)
 							g_free, g_free);
 	mp->progress = g_timer_new();
 
-#if 0
-	if (!g_dbus_register_interface(btd_get_dbus_connection(),
+	if (!g_dbus_register_interface_with_flags(btd_get_dbus_connection(),
 					mp->path, MEDIA_PLAYER_INTERFACE,
+					G_DBUS_INTERFACE_FLAG_EXPERIMENTAL,
 					media_player_methods,
 					media_player_signals,
 					media_player_properties, mp, NULL)) {
@@ -308,7 +308,7 @@ struct media_player *media_player_controller_create(const char *path)
 		media_player_destroy(mp);
 		return NULL;
 	}
-#endif
+
 	DBG("%s", mp->path);
 
 	return mp;
@@ -410,7 +410,6 @@ void media_player_set_status(struct media_player *mp, const char *status)
 
 static gboolean process_metadata_changed(void *user_data)
 {
-#if 0
 	struct media_player *mp = user_data;
 	DBusMessage *signal;
 	DBusMessageIter iter, dict;
@@ -439,7 +438,7 @@ static gboolean process_metadata_changed(void *user_data)
 	dbus_message_iter_close_container(&iter, &dict);
 
 	g_dbus_send_message(btd_get_dbus_connection(), signal);
-#endif
+
 	return FALSE;
 }
 
-- 
1.7.11.7

[PATCH BlueZ 6/6] AVRCP: Fix not checking for media_player_controller_create

[Luiz Augusto von Dentz]

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

Now that the MediaPlayer1 interface is experimental the interface
registration may fail which return NULL, in that case there is no
point on register to any notifications.
---
 profiles/audio/avrcp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 4e3d31d..ce070cd 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -1987,6 +1987,11 @@ static gboolean avrcp_get_capabilities_resp(struct avctp *conn,
 
 	count = pdu->params[1];
 
+	path = device_get_path(session->dev->btd_dev);
+	mp = media_player_controller_create(path);
+	if (mp == NULL)
+		return FALSE;
+
 	for (; count > 0; count--) {
 		uint8_t event = pdu->params[1 + count];
 
@@ -2001,8 +2006,6 @@ static gboolean avrcp_get_capabilities_resp(struct avctp *conn,
 		}
 	}
 
-	path = device_get_path(session->dev->btd_dev);
-	mp = media_player_controller_create(path);
 	media_player_set_callbacks(mp, &ct_cbs, player);
 	player->user_data = mp;
 	player->destroy = (GDestroyNotify) media_player_destroy;
-- 
1.7.11.7

Re: [PATCH BlueZ 3/6] gdbus: Introduce GDBusInterfaceFlags

[Marcel Holtmann]

Hi Luiz,

> The flags should be passed in g_dbus_register_interface_with_flags,
> currently there only one flag which is G_DBUS_INTERFACE_FLAG_EXPERIMENTAL
> which works similarly to G_DBUS_METHOD_FLAG_EXPERIMENTAL but for the
> whole interface.
> ---
>  gdbus/gdbus.h  | 13 +++++++++++++
>  gdbus/object.c | 20 ++++++++++++++++++++
>  2 files changed, 33 insertions(+)

can we not just check if the interface contains any non experimental
method, signals and properties and base that decision around that?

I am not really in favor of introducing a version with flags.

Regards

Marcel

Re: [PATCH BlueZ 4/6] gdbus: Call check_signals when sending signals with g_dbus_send_message

[Marcel Holtmann]

Hi Luiz,

> If message passed to g_dbus_send_message is a signal verify if it is a
> valid and there really exists an interface with respective signal name.
> ---
>  gdbus/object.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/gdbus/object.c b/gdbus/object.c
> index 510763a..6951171 100644
> --- a/gdbus/object.c
> +++ b/gdbus/object.c
> @@ -1475,6 +1475,15 @@ gboolean g_dbus_send_message(DBusConnection *connection, DBusMessage *message)
>  
>  	if (dbus_message_get_type(message) == DBUS_MESSAGE_TYPE_METHOD_CALL)
>  		dbus_message_set_no_reply(message, TRUE);
> +	else {
> +		const char *path = dbus_message_get_path(message);
> +		const char *interface = dbus_message_get_interface(message);
> +		const char *name = dbus_message_get_member(message);
> +		const GDBusArgInfo *args;
> +
> +		if (!check_signal(connection, path, interface, name, &args))
> +			return FALSE;
> +	}

we have more than just method calls and signals. This could be also used
used for sending async method returns.

>  
>  	result = dbus_connection_send(connection, message, NULL);
>  

Regards

Marcel

Re: [PATCH BlueZ 3/6] gdbus: Introduce GDBusInterfaceFlags

[Luiz Augusto von Dentz]

Hi Marcel,

On Sun, Dec 23, 2012 at 6:09 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> The flags should be passed in g_dbus_register_interface_with_flags,
>> currently there only one flag which is G_DBUS_INTERFACE_FLAG_EXPERIMENTAL
>> which works similarly to G_DBUS_METHOD_FLAG_EXPERIMENTAL but for the
>> whole interface.
>> ---
>>  gdbus/gdbus.h  | 13 +++++++++++++
>>  gdbus/object.c | 20 ++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>
> can we not just check if the interface contains any non experimental
> method, signals and properties and base that decision around that?

It can work, but we would have to iterate on every single item, so in
terms of code is quite a bit more complex so I favored adding a new
function even though we might replace it in the long run.

> I am not really in favor of introducing a version with flags.

The final solution I was thinking is passing another interface
containing the table as this tend to be quite static e.g.:

struct GDBusInteface {
  const char *name;
  GDBusInterfaceFlags flags;
  const GDBusMethodTable *methods;
  const GDBusSignalTable *signals;
  const GDBusPropertiesTable *properties;
};

Obviously this would break g_dbus_register_interface and generate
quite a bit more changes to the daemon code.


--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 3/6] gdbus: Introduce GDBusInterfaceFlags

[Marcel Holtmann]

Hi Luiz,

> >> The flags should be passed in g_dbus_register_interface_with_flags,
> >> currently there only one flag which is G_DBUS_INTERFACE_FLAG_EXPERIMENTAL
> >> which works similarly to G_DBUS_METHOD_FLAG_EXPERIMENTAL but for the
> >> whole interface.
> >> ---
> >>  gdbus/gdbus.h  | 13 +++++++++++++
> >>  gdbus/object.c | 20 ++++++++++++++++++++
> >>  2 files changed, 33 insertions(+)
> >
> > can we not just check if the interface contains any non experimental
> > method, signals and properties and base that decision around that?
> 
> It can work, but we would have to iterate on every single item, so in
> terms of code is quite a bit more complex so I favored adding a new
> function even though we might replace it in the long run.

we have to iterate over every single item in the success case anyway, so
why bother trying to optimize this.

> > I am not really in favor of introducing a version with flags.
> 
> The final solution I was thinking is passing another interface
> containing the table as this tend to be quite static e.g.:

If this is not the final solution you are heading for, then lets not
even bother introducing it. Lets go with the the simple approach that
does not introduce any new functions.

It has the advantage that you only need to change one place. Otherwise
you need to check the method structs and the calling register function.

Regards

Marcel

Re: [PATCH BlueZ 4/6] gdbus: Call check_signals when sending signals with g_dbus_send_message

[Luiz Augusto von Dentz]

Hi Marcel,

On Sun, Dec 23, 2012 at 6:10 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> If message passed to g_dbus_send_message is a signal verify if it is a
>> valid and there really exists an interface with respective signal name.
>> ---
>>  gdbus/object.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/gdbus/object.c b/gdbus/object.c
>> index 510763a..6951171 100644
>> --- a/gdbus/object.c
>> +++ b/gdbus/object.c
>> @@ -1475,6 +1475,15 @@ gboolean g_dbus_send_message(DBusConnection *connection, DBusMessage *message)
>>
>>       if (dbus_message_get_type(message) == DBUS_MESSAGE_TYPE_METHOD_CALL)
>>               dbus_message_set_no_reply(message, TRUE);
>> +     else {
>> +             const char *path = dbus_message_get_path(message);
>> +             const char *interface = dbus_message_get_interface(message);
>> +             const char *name = dbus_message_get_member(message);
>> +             const GDBusArgInfo *args;
>> +
>> +             if (!check_signal(connection, path, interface, name, &args))
>> +                     return FALSE;
>> +     }
>
> we have more than just method calls and signals. This could be also used
> used for sending async method returns.

I guess I should do else if and check for signal type then, or you
have a better idea?

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 4/6] gdbus: Call check_signals when sending signals with g_dbus_send_message

[Marcel Holtmann]

Hi Luiz,

> >> If message passed to g_dbus_send_message is a signal verify if it is a
> >> valid and there really exists an interface with respective signal name.
> >> ---
> >>  gdbus/object.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/gdbus/object.c b/gdbus/object.c
> >> index 510763a..6951171 100644
> >> --- a/gdbus/object.c
> >> +++ b/gdbus/object.c
> >> @@ -1475,6 +1475,15 @@ gboolean g_dbus_send_message(DBusConnection *connection, DBusMessage *message)
> >>
> >>       if (dbus_message_get_type(message) == DBUS_MESSAGE_TYPE_METHOD_CALL)
> >>               dbus_message_set_no_reply(message, TRUE);
> >> +     else {
> >> +             const char *path = dbus_message_get_path(message);
> >> +             const char *interface = dbus_message_get_interface(message);
> >> +             const char *name = dbus_message_get_member(message);
> >> +             const GDBusArgInfo *args;
> >> +
> >> +             if (!check_signal(connection, path, interface, name, &args))
> >> +                     return FALSE;
> >> +     }
> >
> > we have more than just method calls and signals. This could be also used
> > used for sending async method returns.
> 
> I guess I should do else if and check for signal type then, or you
> have a better idea?

just do the else if part here. You could use a switch statement, but
then you can not have local variables. So it does not really help.

I am still wondering why we are setting no reply flag when this is a
method call. That makes no sense to me. Must be missing something.

Regards

Marcel