On Mon, Jul 29, 2013 at 12:29 PM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Thu, 2013-07-25 at 00:31 -0300, Jo?o Paulo Rechi Vita wrote:
>> On Jul 18, 2013 12:31 PM, "Tanu Kaskinen" <tanu.kaskinen at linux.intel.com>
>> wrote:
>> >
>> > On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
>> > > +static void bluez5_transport_release(pa_bluetooth_transport *t) {
>> > > + DBusMessage *m;
>> > > + DBusError err;
>> > > +
>> > > + pa_assert(t);
>> > > + pa_assert(t->device);
>> > > + pa_assert(t->device->discovery);
>> > > +
>> > > + dbus_error_init(&err);
>> > > +
>> > > + if (t->state <= PA_BLUETOOTH_TRANSPORT_STATE_IDLE) {
>> > > + pa_log_info("Transport %s auto-released by BlueZ or already
>> released", t->path);
>> > > + return;
>> > > + }
>> > > +
>> > > + pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
>> BLUEZ_MEDIA_TRANSPORT_INTERFACE, "Release"));
>> > > +
>> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
>> m, -1, &err);
>> > > +
>> > > + if (dbus_error_is_set(&err)) {
>> > > + pa_log_error("Failed to release transport %s: %s", t->path,
>> err.message);
>> >
>> > In an earlier log message there is a mention of auto-releasing by BlueZ.
>> > I don't know exactly what it means, but it sounds like BlueZ can release
>> > the transport too, in which case there is a race condition: if BlueZ
>> > releases a transport at the same time when this code is running, our
>> > Release() call will be received when the transport is already released,
>> > which I presume will result in an error. If that is not a real error
>> > condition, perhaps the log level should be info instead of error.
>> >
>>
>> If the transport is auto-released by BlueZ is not an error condition, but
>> other errors can occur on release, so I think we should keep the log level
>> here as error. FWIW possible errors on release are "not authorized", if the
>> process which calls Release() is not the same which acquired it before, and
>> "in progress" if Release() has already been called by the same process but
>> not returned yet.
>
> OK, let's keep the log level as it is. My race scenario with
> auto-releasing was incorrect, but even if it was correct, the logic of
> downgrading the log error was bad. With my logic, we shouldn't print an
> error in those cases either where we send a message to an object that
> gets removed when our message is on its way. It would be nice not to
> print an error in those cases, if the object disappears as part of
> normal operation, but I don't think it's feasible to figure out when an
> object disappears as part of normal operation and when it's abnormal, so
> better print the error always (it's a rare occurrence anyway).
>
>
>> > > diff --git a/src/modules/bluetooth/bluez5-util.h
>> b/src/modules/bluetooth/bluez5-util.h
>> > > index a205b45..82953a7 100644
>> > > --- a/src/modules/bluetooth/bluez5-util.h
>> > > +++ b/src/modules/bluetooth/bluez5-util.h
>> > > @@ -24,14 +24,50 @@
>> > >
>> > > #include <pulsecore/core.h>
>> > >
>> > > +typedef struct pa_bluetooth_transport pa_bluetooth_transport;
>> > > typedef struct pa_bluetooth_device pa_bluetooth_device;
>> > > typedef struct pa_bluetooth_discovery pa_bluetooth_discovery;
>> > >
>> > > typedef enum pa_bluetooth_hook {
>> > > PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED, /* Call
>> data: pa_bluetooth_device */
>> > > + PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED, /* Call
>> data: pa_bluetooth_transport */
>> > > PA_BLUETOOTH_HOOK_MAX
>> > > } pa_bluetooth_hook_t;
>> > >
>> > > +typedef enum profile {
>> > > + PROFILE_A2DP_SINK,
>> > > + PROFILE_A2DP_SOURCE,
>> > > + PROFILE_OFF
>> > > +} pa_bluetooth_profile_t;
>> > > +#define PA_BLUETOOTH_PROFILE_COUNT PROFILE_OFF
>> > > +
>> > > +typedef enum pa_bluetooth_transport_state {
>> > > + PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED,
>> > > + PA_BLUETOOTH_TRANSPORT_STATE_IDLE,
>> > > + PA_BLUETOOTH_TRANSPORT_STATE_PLAYING
>> > > +} pa_bluetooth_transport_state_t;
>> > > +
>> > > +typedef int
>> (*pa_bluetooth_transport_acquire_cb)(pa_bluetooth_transport *t, bool
>> optional, size_t *imtu, size_t *omtu);
>> > > +typedef void
>> (*pa_bluetooth_transport_release_cb)(pa_bluetooth_transport *t);
>> >
>> > If these typedefs are only used in the pa_bluetooth_transport
>> > definition, I don't think they bring any benefit, and they also make it
>> > harder to check what the function signature is (the first place to look
>> > is the struct definition, and from there you have to go to the typedef -
>> > it would be easier if the signature was visible already in the struct
>> > definition).
>> >
>> > Hmm, now I see that the typedefs are used also in the transport_new()
>> > function parameters, where they certainly are very useful. However, I
>> > would prefer you to follow the usual subclassing pattern in PulseAudio:
>> > the backend calls foo_new() and then sets the callbacks and the userdata
>> > pointer (and then calls foo_put(), if such function exists).
>> >
>>
>> Can you point me for an example of this pattern that I could follow? It's
>> not clear to me from the text what exactly you mean.
>
> Well, have a look at how sinks, for example, are created.
> module-bluetooth-device calls pa_sink_new(), sets pa_sink.userdata and
> some callbacks, and then calls pa_sink_put(). Similarly, the a2dp
> transport code can call pa_bluez5_transport_new(), then set
> pa_bluez5_transport.userdata and the callbacks, and then, if necessary,
> call pa_bluez5_transport_put().
>
If I understood correctly your suggestion is to have acquire_cb,
release_cb and userdata set by the caller function instead of being
parameters of pa_bluetooth_transport_new(). I don't see how a _put()
function would fit here. But even with this pattern I prefer to keep
the typedefs instead of having the function signature in the struct.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
