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
