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().
--
Tanu
