[pulseaudio-discuss] [RFC v0 07/15] bluetooth: BlueZ 5 interface rename to org.bluez.MediaEndpoint1

[Tanu Kaskinen]

On Wed, 2012-12-19 at 13:58 +0100, Mikel Astiz wrote:
> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
> 
> Use the new interface name if BlueZ 5 has been detected.
> ---
>  src/modules/bluetooth/bluetooth-util.c | 52 +++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
> index 67b3240..0d18c4d 100644
> --- a/src/modules/bluetooth/bluetooth-util.c
> +++ b/src/modules/bluetooth/bluetooth-util.c
> @@ -61,6 +61,30 @@
>      " </interface>"                                                     \
>      "</node>"
>  
> +#define BLUEZ_5_ENDPOINT_INTROSPECT_XML                                 \

I think "MEDIA_ENDPOINT_1_INTROSPECT_XML" would be a better name. The
reason is that the introspection data isn't necessarily tied to the
bluez version number - bluez 5 may introduce MediaEndpoint2, or bluez 6
may still use MediaEndpoint1.

> +    DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE                           \
> +    "<node>"                                                            \
> +    " <interface name=\"org.bluez.MediaEndpoint1\">"                    \
> +    "  <method name=\"SetConfiguration\">"                              \
> +    "   <arg name=\"transport\" direction=\"in\" type=\"o\"/>"          \
> +    "   <arg name=\"configuration\" direction=\"in\" type=\"ay\"/>"     \
> +    "  </method>"                                                       \
> +    "  <method name=\"SelectConfiguration\">"                           \
> +    "   <arg name=\"capabilities\" direction=\"in\" type=\"ay\"/>"      \
> +    "   <arg name=\"configuration\" direction=\"out\" type=\"ay\"/>"    \
> +    "  </method>"                                                       \
> +    "  <method name=\"ClearConfiguration\">"                            \
> +    "  </method>"                                                       \
> +    "  <method name=\"Release\">"                                       \
> +    "  </method>"                                                       \
> +    " </interface>"                                                     \
> +    " <interface name=\"org.freedesktop.DBus.Introspectable\">"         \
> +    "  <method name=\"Introspect\">"                                    \
> +    "   <arg name=\"data\" type=\"s\" direction=\"out\"/>"              \
> +    "  </method>"                                                       \
> +    " </interface>"                                                     \
> +    "</node>"
> +
>  typedef enum pa_bluez_version {
>      BLUEZ_VERSION_UNKNOWN,
>      BLUEZ_VERSION_4,
> @@ -1526,7 +1550,7 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>      dbus_message_iter_get_basic(&args, &path);

Not a problem of this patch, but it seems that
endpoint_set_configuration() doesn't check that the received message
actually contains the path argument.

>  
>      if (pa_hashmap_get(y->transports, path)) {
> -        pa_log("org.bluez.MediaEndpoint.SetConfiguration: Transport %s is already configured.", path);
> +        pa_log("Endpoint SetConfiguration: Transport %s is already configured.", path);
>          goto fail;
>      }
>  
> @@ -1614,9 +1638,8 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>      return r;
>  
>  fail:
> -    pa_log("org.bluez.MediaEndpoint.SetConfiguration: invalid arguments");
> -    pa_assert_se(r = (dbus_message_new_error(m, "org.bluez.MediaEndpoint.Error.InvalidArguments",
> -                                                        "Unable to set configuration")));
> +    pa_log("Endpoint SetConfiguration: invalid arguments");
> +    pa_assert_se(r = (dbus_message_new_error(m, "org.bluez.Error.InvalidArguments", "Unable to set configuration")));

While you're modifying this line, you could also remove the redundant
parentheses around the dbus_message_new_error() call.

>      return r;
>  }
>  
> @@ -1630,7 +1653,7 @@ static DBusMessage *endpoint_clear_configuration(DBusConnection *c, DBusMessage
>      dbus_error_init(&e);
>  
>      if (!dbus_message_get_args(m, &e, DBUS_TYPE_OBJECT_PATH, &path, DBUS_TYPE_INVALID)) {
> -        pa_log("org.bluez.MediaEndpoint.ClearConfiguration: %s", e.message);
> +        pa_log("Endpoint ClearConfiguration: %s", e.message);
>          dbus_error_free(&e);
>          goto fail;
>      }
> @@ -1655,8 +1678,7 @@ static DBusMessage *endpoint_clear_configuration(DBusConnection *c, DBusMessage
>      return r;
>  
>  fail:
> -    pa_assert_se(r = (dbus_message_new_error(m, "org.bluez.MediaEndpoint.Error.InvalidArguments",
> -                                                        "Unable to clear configuration")));
> +    pa_assert_se(r = (dbus_message_new_error(m, "org.bluez.Error.InvalidArguments", "Unable to clear configuration")));

Extra parentheses here too.

>      return r;
>  }
>  
> @@ -1726,7 +1748,7 @@ static DBusMessage *endpoint_select_configuration(DBusConnection *c, DBusMessage
>      dbus_error_init(&e);
>  
>      if (!dbus_message_get_args(m, &e, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE, &cap, &size, DBUS_TYPE_INVALID)) {
> -        pa_log("org.bluez.MediaEndpoint.SelectConfiguration: %s", e.message);
> +        pa_log("Endpoint SelectConfiguration: %s", e.message);
>          dbus_error_free(&e);
>          goto fail;
>      }
> @@ -1823,8 +1845,7 @@ done:
>      return r;
>  
>  fail:
> -    pa_assert_se(r = (dbus_message_new_error(m, "org.bluez.MediaEndpoint.Error.InvalidArguments",
> -                                                        "Unable to select configuration")));
> +    pa_assert_se(r = (dbus_message_new_error(m, "org.bluez.Error.InvalidArguments", "Unable to select configuration")));

Extra parentheses.

-- 
Tanu