[pulseaudio-discuss] [PATCH v4 3/8] bluetooth: ofono: Detect if Connect has been called

[Georg Chini]

On 03.05.2017 19:18, Luiz Augusto von Dentz wrote:
> Hi Georg,
>
> On Sat, Apr 29, 2017 at 4:01 PM, Georg Chini <georg at chini.tk> wrote:
>> On 29.04.2017 13:28, Georg Chini wrote:
>>> On 26.04.2017 14:19, Luiz Augusto von Dentz wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
>>>>
>>>> This detects if profile has already been called and we are waiting
>>>> the response.
>>>> ---
>>>>    src/modules/bluetooth/backend-ofono.c | 19 ++++++++++++++++---
>>>>    1 file changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/modules/bluetooth/backend-ofono.c
>>>> b/src/modules/bluetooth/backend-ofono.c
>>>> index 3fbf153..a847ad0 100644
>>>> --- a/src/modules/bluetooth/backend-ofono.c
>>>> +++ b/src/modules/bluetooth/backend-ofono.c
>>>> @@ -65,6 +65,7 @@ struct hf_audio_card {
>>>>        char *remote_address;
>>>>        char *local_address;
>>>>    +    bool connecting;
>>>>        int fd;
>>>>        uint8_t codec;
>>>>    @@ -156,12 +157,22 @@ static int
>>>> hf_audio_agent_transport_acquire(pa_bluetooth_transport *t, bool opti
>>>>        pa_assert(card);
>>>>          if (!optional && card->fd < 0) {
>>>> -        DBusMessage *m;
>>>> +        DBusMessage *m, *r;
>>>> +        DBusError derr;
>>>>    +        if (card->connecting)
>>>> +            return -1;
>>>> +
>>>> +        card->connecting = true;
>>>> +
>>>> +        dbus_error_init(&derr);
>>>>            pa_assert_se(m = dbus_message_new_method_call(t->owner,
>>>> t->path, "org.ofono.HandsfreeAudioCard", "Connect"));
>>>> -
>>>> pa_assert_se(dbus_connection_send(pa_dbus_connection_get(card->backend->connection),
>>>> m, NULL));
>>>> +        r =
>>>> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection),
>>>> m, -1, &derr);
>>>> +        if (!r)
>>>> +            return -1;
>>>>    -        return -1;
>>>> +        if (card->connecting)
>>>> +            return -1;
>>>>        }
>>>>          /* The correct block size should take into account the SCO MTU
>>>> from
>>>> @@ -535,6 +546,8 @@ static DBusMessage
>>>> *hf_audio_agent_new_connection(DBusConnection *c, DBusMessage
>>>>          card = pa_hashmap_get(backend->cards, path);
>>>>    +    card->connecting = false;
>>>
>>> Should card->connecting not be set to false immediately after entering
>>> hf_audio_agent_new_connection()? If you set it here, the card will never
>>> be able to connect again if the sender was wrong or
>>> dbus_message_get_args() fails. Or do you expect there will be another
>>> call to hf_audio_agent_new_connection() in these cases.
>> Sorry, I did not see that the card is not known before that point, so that
>> you can't set the flag before.
>> But nevertheless the question remains what to do if one of the
>> two errors occurs. Maybe you could do the dbus_message_get_args()
>> first to identify the card. I don't know enough about the DBUS API,
>> if only one argument is wrong in dbus_message_get_args(), will the other
>> values be set anyway? If yes you could even try after a failure
>> if the path is set.
>
> This is the earliest we can know that the NewConnection is for the
> card connecting since there could be multiple cards connecting we do
> need dbus_message_get_args to complete, and no I don't think it can
> parse partially as the signature doesn't match, anyway that would be
> something completely wrong with ofono or something is taking its name
> and pretending to be it, so I guess we are doing the right thing here,
> though we probably need to add a card->connection = false to
> hf_audio_agent_transport_release in case it never resumes.
>
>
>
Yes, you are right. It does not help much to swap the calls. So I'm OK
with the patch.