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

[Luiz Augusto von Dentz]

Hi Tanu,

On Mon, Jun 19, 2017 at 4:29 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Thu, 2017-05-25 at 11:36 +0300, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
>>
>> Attempt to use Acquire method if available.
>
> 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?
>
>> ---
>>  src/modules/bluetooth/backend-ofono.c | 59 +++++++++++++++++++++++++----------
>>  1 file changed, 43 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/backend-ofono.c b/src/modules/bluetooth/backend-ofono.c
>> index 6e9a366..d098402 100644
>> --- a/src/modules/bluetooth/backend-ofono.c
>> +++ b/src/modules/bluetooth/backend-ofono.c
>> @@ -150,6 +150,46 @@ static int socket_accept(int sock)
>>      return 0;
>>  }
>>
>> +static int card_acquire(struct hf_audio_card *card) {
>> +    pa_bluetooth_transport *t = card->transport;
>> +    DBusMessage *m, *r;
>> +    DBusError err;
>> +
>> +    if (card->connecting)
>> +        return -EAGAIN;
>> +
>> +    /* Try acquiring the stream first */
>> +    dbus_error_init(&err);
>
> err is never freed.
>
>> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", "Acquire"));
>
> m is never unreffed.

And it is never done with dbus_connection_send_with_reply_and_block
since that uses a pending call that takes ownership of the message.

>> +    r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), m, -1, &err);
>
> r is never unreffed.

Will fix it.

>> +    if (!r) {
>> +        if (!pa_streq(err.name, DBUS_ERROR_UNKNOWN_METHOD)) {
>> +            pa_log_error("Failed to acquire %s: %s", err.name, err.message);
>> +            return -1;
>> +        }
>> +    } else if ((dbus_message_get_args(r, NULL,
>> +                         DBUS_TYPE_UNIX_FD, &card->fd,
>> +                         DBUS_TYPE_BYTE, &card->codec,
>
> I don't know how dbus_message_get_args() works, but it seems likely
> that it can fail between setting card->fd and card->codec. We don't
> want to set card->fd if the operation fails, so I think we should read
> the values into local variables, and only update the card if the
> operation is successful.

Will add separate variables just to be safe.

> The codec value should be checked (it has to be CVSD).
>
> hf_audio_agent_new_connection() sets the transport state to PLAYING,
> but that's not done here. This was pointed out by Georg as well. In the
> previous discussion, he said that he'll send a patch related to this,
> but that hasn't happened...

hf_audio_agent_new_connection is called for the main thread thus why
it can change the transport state, on the IO thread this would cause a
crash, in fact up to now all backends including A2DP do not set the
transport state from acquire callback directly for this very reason
which is why I decided to leave it be like this since perhaps we want
the caller to directly set the state which might make sense to change
in a separate patch.

>> +                         DBUS_TYPE_INVALID) == true)) {
>> +        return card->fd;
>> +    } else
>> +        return -1;
>
> An error message should be logged.

Will add.

>> +
>> +    /* Fallback to Connect as this might be an old version of ofono */
>> +    card->connecting = true;
>> +
>> +    dbus_error_init(&err);
>
> err is never freed.

Will fix.

>> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", "Connect"));
>
> m is never unreffed.

Ditto.

>> +    r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), m, -1, &err);
>
> r is never unreffed.

Will fix.

>> +    if (!r)
>> +        return -1;
>
> An error message should be logged.
>
> --
> Tanu
>
> https://www.patreon.com/tanuk



-- 
Luiz Augusto von Dentz