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

[Georg Chini]

On 04.07.2017 15:38, Tanu Kaskinen wrote:
> On Mon, 2017-07-03 at 21:01 +0300, Luiz Augusto von Dentz wrote:
>> Hi Georg,
>>
>> On Mon, Jul 3, 2017 at 8:32 PM, Georg Chini <georg at chini.tk> wrote:
>>> On 03.07.2017 17:51, Luiz Augusto von Dentz wrote:
>>>> Hi Tanu,
>>>>
>>>> On Mon, Jul 3, 2017 at 5:05 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
>>>>> On Mon, 2017-07-03 at 16:09 +0300, Luiz Augusto von Dentz wrote:
>>>>>> 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:
>>>>>>>> +    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.
>>>>> Is a separate patch forthcoming? Without such patch it looks like
>>>>> you're introducing a regression here.
>>>> Don't follow your logic, regression to what? _None_ of the the
>>>> backends do set the transport state from the io thread, this is not
>>>> changing anything either with Connect or Acquire the behavior is
>>>> exactly the same in this regard. Im not saying we should not fix it,
>>>> but considering the current behavior is to wait for NewConnection
>>>> forever that is probably a much worse offender then having out of sync
>>>> transport state.
>>>>
>>>> A2DP also does a similar thing, though it return the fd in place it
>>>> only changes the transport state when D-Bus indicate so:
>>>>
>>>>               if (pa_streq(key, "State")) {
>>>>                   pa_bluetooth_transport_state_t state;
>>>>
>>>>                   if (transport_state_from_string(value, &state) < 0) {
>>>>                       pa_log_error("Invalid state received: %s", value);
>>>>                       return;
>>>>                   }
>>>>
>>>>                   pa_bluetooth_transport_set_state(t, state);
>>>>
>>>> Unfortunately the Connect + NewConnection is racy, which should be
>>>> clear by now, this stayed for as long I remembered the ofono backend
>>>> exists, so I fixing things because they are not very reliable,
>>>> including ofono which got patched as well.
>>>>
>>>>
>>>>
>>> Hi, I don't understand the discussion. I think we don't want to set the
>>> state
>>> from the I/O thread. The transport state is set correctly from the main
>>> thread in all cases except for the AG role when using the native
>>> backend. The patch Tanu is referring to is here:
>>>
>>> https://patchwork.freedesktop.org/patch/155661/
> Oh, so you did send a patch for this already. Sorry, I don't know how I
> missed it.
>
>> Great, that should work regardless of the backend. Btw is there a
>> chance the BLUETOOTH_MESSAGE_SET_TRANSPORT_PLAYING set but in the
>> meantime we disconnect/hup? It seems to me this is still racy, though
>> we can probably ensure setting to playing is valid by checking if
>> transport is active, but if the profile changes in the meantime this
>> might not work since the transport may have changed.
> It looks racy indeed, so some check should be added as you say. When
> shutting down or changing the profile, stop_thread() is always called.
> stop_thread() calls pa_thread_mq_done(), and this is where queued
> messages are processed. The transport hasn't been released yet at this
> point, but I think the transport release can be moved to happen before
> pa_thread_mq_done(), then you can check if u->transport is NULL.
>
Mh, after looking at the code, I don't see the race condition. Let's
assume we went through transport_acquire() and sent a
BLUETOOTH_MESSAGE_SET_TRANSPORT_PLAYING message. This
means we successfully acquired the transport. Now something
happens that leads to a thread shutdown before the message is
processed. This can either be a profile change or the remote end
unexpectedly closing the connection. In all cases stop_thread()
will be called. In stop_thread() the outstanding message will be
processed and the transport set to playing in pa_thread_mq_done().
This does not really matter, because the transport is released
immediately after this through transport_release(). It just reflects -
with a little delay - what happened in reality.

In my opinion we would only have a race condition if it was possible
that the transport was released before the message was processed
but I do not see how this could happen.

But maybe I just did not see the point (again), so please correct me
if I'm wrong.