[pulseaudio-discuss] [PATCH 1/3] bluetooth: ofono: Use Acquire method if available

[Luiz Augusto von Dentz]

Hi Tanu,

On Thu, Mar 15, 2018 at 9:54 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Fri, 2018-03-09 at 14:12 +0200, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
>>
>> Attempt to use Acquire method if available.
>
> Quoting my previous review:
>
> "After reading the commit message, I don't know what problem this patch
> tries to fix. Please write a more informative commit message. Also,
> which oFono version introduced the Acquire method?"

Will add to the patch, I thought this was clear that we cannot send
errors as signals which means if there is an error on Connect
NewConnection may never be emitted leaving the card in connecting
forever.

>
>> ---
>>  src/modules/bluetooth/backend-ofono.c | 92 +++++++++++++++++++++++++++--------
>>  1 file changed, 72 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/backend-ofono.c b/src/modules/bluetooth/backend-ofono.c
>> index 2c51497f3..49906560e 100644
>> --- a/src/modules/bluetooth/backend-ofono.c
>> +++ b/src/modules/bluetooth/backend-ofono.c
>> @@ -150,35 +150,87 @@ static int socket_accept(int sock)
>>      return 0;
>>  }
>>
>> -static int hf_audio_agent_transport_acquire(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
>> -    struct hf_audio_card *card = t->userdata;
>> -    int err;
>> +static DBusMessage *card_send(struct hf_audio_card *card, const char *method, DBusError *err)
>> +{
>> +    pa_bluetooth_transport *t = card->transport;
>> +    DBusMessage *m, *r;
>>
>> -    pa_assert(card);
>> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", method));
>> +    r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), m, -1, err);
>> +    dbus_message_unref(m);
>>
>> -    if (!optional && card->fd < 0) {
>> -        DBusMessage *m, *r;
>> -        DBusError derr;
>> +    return r;
>> +}
>>
>> -        if (card->connecting)
>> -            return -EAGAIN;
>> +static int card_acquire(struct hf_audio_card *card) {
>> +    int fd;
>> +    uint8_t codec;
>> +    DBusMessage *r;
>> +    DBusError err;
>>
>> -        card->connecting = true;
>> +    if (card->connecting)
>> +        return -EAGAIN;
>>
>> -        dbus_error_init(&derr);
>> -        pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", "Connect"));
>> -        r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), m, -1, &derr);
>> -        dbus_message_unref(m);
>> -        m = NULL;
>> +    /* Try acquiring the stream first */
>> +    dbus_error_init(&err);
>> +    r = card_send(card, "Acquire", &err);
>>
>> -        if (!r)
>> +    if (!r) {
>> +        if (!pa_streq(err.name, DBUS_ERROR_UNKNOWN_METHOD)) {
>> +            pa_log_error("Failed to acquire %s: %s", err.name, err.message);
>> +            dbus_error_free(&err);
>>              return -1;
>> -
>> +        }
>> +    } else if ((dbus_message_get_args(r, NULL, DBUS_TYPE_UNIX_FD, &fd,
>> +                                      DBUS_TYPE_BYTE, &codec,
>> +                                      DBUS_TYPE_INVALID) == true)) {
>> +        dbus_message_unref(r);
>> +        if (card->codec != HFP_AUDIO_CODEC_CVSD) {
>
> You have to check codec, not card->codec.

Fixed on v2.

>> +            pa_log_error("Invalid codec: %u", card->codec);
>> +            /* shutdown to make sure connection is dropped immediately */
>> +            shutdown(card->fd, SHUT_RDWR);
>> +            close(card->fd);
>> +            card->fd = -1;
>> +        }
>> +        return card->fd;
>
> Have you tested this patch? To me this looks like this can never work,
> because on success card->fd is not updated (nor card->codec).
>
> By the way, why is the codec stored in both hf_audio_card and
> pa_bluetooth_transport? Wouldn't it be enough to store it in
> pa_bluetooth_transport?

Perhaps because the card->codec stores the codec during NewConnection,
at which point the transport may not be ready to resume after all we
are the main thread no the IO, we could store it directly on the
transport if it wasn't for this detail or if we could drop backward
compatibility.

>> +    } else {
>> +        pa_log_error("Unable to acquire");
>>          dbus_message_unref(r);
>> -        r = NULL;
>> +        return -1;
>> +    }
>> +
>> +    dbus_error_free(&err);
>>
>> -        if (card->connecting)
>> -            return -EAGAIN;
>> +    /* Fallback to Connect as this might be an old version of ofono */
>> +    card->connecting = true;
>> +
>> +    dbus_error_init(&err);
>> +    card_send(card, "Connect", &err);
>
> You forgot to assign the card_send() return value to r.

Fixed on v2.

>
> --
> Tanu
>
> https://liberapay.com/tanuk
> https://www.patreon.com/tanuk



-- 
Luiz Augusto von Dentz