[pulseaudio-discuss] [PATCHv2 27/60] bluetooth: Create pa_bluetooth_transport for BlueZ 5 support

[João Paulo Rechi Vita]

On Tue, Aug 20, 2013 at 2:24 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Mon, 2013-08-19 at 13:44 -0300, João Paulo Rechi Vita wrote:
>> On Fri, Aug 16, 2013 at 12:32 PM, Tanu Kaskinen
>> <tanu.kaskinen at linux.intel.com> wrote:
>> > On Tue, 2013-08-13 at 01:54 -0300, jprvita at gmail.com wrote:
>> >> From: João Paulo Rechi Vita <jprvita at openbossa.org>
>> >>
>> >> Create the pa_bluetooth_transport structure to store information about
>> >> the bluetooth transport and utility functions to manipulate this
>> >> structure. The acquire() and release() operations are function pointers
>> >> in the pa_bluetooth_transport structure to make possible for different
>> >> transport backends to provide different implementations of these
>> >> operations. Thre is also a userdata field for the transport backend
>> >> provide data for the acquire/release functions.
>> >> ---
>> >>  src/modules/bluetooth/bluez5-util.c | 145 ++++++++++++++++++++++++++++++++++++
>> >>  src/modules/bluetooth/bluez5-util.h |  43 +++++++++++
>> >>  2 files changed, 188 insertions(+)
>> >>
>> >> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
>> >> index 1972a92..69eb759 100644
>> >> --- a/src/modules/bluetooth/bluez5-util.c
>> >> +++ b/src/modules/bluetooth/bluez5-util.c
>> >> @@ -36,6 +36,7 @@
>> >>  #include "bluez5-util.h"
>> >>
>> >>  #define BLUEZ_SERVICE "org.bluez"
>> >> +#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE ".MediaTransport1"
>> >>
>> >>  struct pa_bluetooth_discovery {
>> >>      PA_REFCNT_DECLARE;
>> >> @@ -45,8 +46,131 @@ struct pa_bluetooth_discovery {
>> >>      bool filter_added;
>> >>      pa_hook hooks[PA_BLUETOOTH_HOOK_MAX];
>> >>      pa_hashmap *devices;
>> >> +    pa_hashmap *transports;
>> >>  };
>> >>
>> >> +pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d, const char *owner, const char *path,
>> >> +                                                          pa_bluetooth_profile_t p, const uint8_t *config, size_t size) {
>> >
>> > Unaligned parameter list.
>> >
>> > There is some precedence for wrapping long parameter lists like this:
>> >
>> > pa_bluetooth_transport *pa_bluetooth_transport_new(
>> >         pa_bluetooth_device *d,
>> >         const char *owner,
>> >         const char *path,
>> >         pa_bluetooth_profile_t p,
>> >         const uint8_t *config,
>> >         size_t size) {
>> >
>>
>> Ok. Maybe we should add this to the official coding style guidelines
>> in the wiki, although I personally dislike having the function
>> parameters aligned in the same level as the function implementation.
>> My preference would be to have them aligned after the opening
>> parenthesis.
>
> The parameters in my suggestion aren't aligned in the same level as the
> function implementation - the indentation for the parameters is 8
> spaces, while the function implementation is indented by 4 spaces.
>
> I'll discuss with Arun and David about standardizing this or some other
> style.
>
>> >>  static void device_free(pa_bluetooth_device *d) {
>> >> +    unsigned i;
>> >> +
>> >>      pa_assert(d);
>> >>
>> >> +    for (i = 0; i < PA_BLUETOOTH_PROFILE_COUNT; i++) {
>> >> +        pa_bluetooth_transport *t;
>> >> +
>> >> +        if (!(t = d->transports[i]))
>> >> +            continue;
>> >> +
>> >> +        d->transports[i] = NULL;
>> >> +        transport_state_changed(t, PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
>> >> +        /* TODO: check if there is no risk of calling
>> >> +         * transport_connection_changed_cb() when the transport is already freed */
>> >> +        pa_bluetooth_transport_free(t);
>> >
>> > IMO pa_bluetooth_transport_free() should be called by the transport
>> > code, because the backend also creates the transport. The backend may
>> > need a callback for getting notified about the device going away. Or
>> > perhaps there should be pa_bluetooth_transport_kill(), with a kill()
>> > callback in the backend. This would be similar to how sink inputs are
>> > killed if the sink they're connected to goes away.
>> >
>> > If the backend is responsible for calling pa_bluetooth_transport_free(),
>> > the core should still ensure that t->device is set to NULL and that the
>> > transport is removed from discovery->transports (feel free to create
>> > pa_bluetooth_transport_unlink() for this purpose, if you want).
>> >
>>
>> I'll answer this comment in the next message in this thread due to the
>> additional comment in that message.
>>
>> > As for the TODO item, I don't know what it means. There is no
>> > transport_connection_changed_cb() function.
>> >
>>
>> transport_connection_changed_cb() is the callback in
>> module-bluez5-device for the hook
>> PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED, which is fired by the
>> transport_state_changed() call right before the TODO line. Depending
>> on the way hook callbacks are called by the mainloop t may be already
>> freed by the pa_bluetooth_transport_free() call after the TODO line.
>
> Ok, so you mean transport_state_changed_cb(), not
> transport_connection_changed_cb(). If I understood correctly, you worry
> that the hook callbacks might get called asynchronously. Don't worry,
> they're always synchronous.
>

Yes, there was a naming issue here, sorry. The code path I'm worried
about is that transport_state_changed() fires
PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED which triggers a call to
transport_state_changed_cb() in module-bluez5-device. But right after
transport_state_changed() returns here, pa_bluetooth_transport_free()
is called, freeing the transport object. If the handlers of fired
events are called immediately this flow is fine, but if they are added
to a queue and called on a successive mainloop iteration, then this
may hit the assertion pa_assert(t) in the beginning of
transport_state_changed_cb().

Looking at the implementation of pa_hook_fire() the code path seems safe.

-- 
João Paulo Rechi Vita
http://about.me/jprvita