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
