On 17.07.2017 19:44, Tanu Kaskinen wrote:
> On Sun, 2017-07-16 at 14:35 +0200, Georg Chini wrote:
>> 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.
> You have a point - I can't point to any specific thing that will
> definitely break. However, the IO thread has already been torn down
> when the SET_TRANSPORT_PLAYING message is processed, and I'm not sure
> if setting the transport state to PLAYING is safe in that situation.
> pa_bluetooth_transport_set_state() will fire some hooks, and I didn't
> trace down what happens in those hooks. It seems safer to tear down the
> transport while the IO thread is still running.
>
After another look I understand even less ...
Doesn't pa_thread_mq_done() process the final messages for the I/O
thread and not for the main thread? The messages we are talking
about are messages from the I/O thread to the main thread and are
therefore processed in the main thread. This can't happen while the
main thread is executing stop_thread(), so from that perspective
it should not matter where in stop_thread() the transport is set to
NULL (unless my assumption concerning pa_thread_mq_done() is
wrong).
On the other hand I think it should only be set to NULL after all I/O
thread messages have been processed, which is after
pa_thread_mq_done(), so I still believe releasing the transport after
the call is correct.
Now however I wonder if there is the possibility that there are still
pending BLUETOOTH_MESSAGE_SET_TRANSPORT_PLAYING
messages after stop_thread() has been called. This is should be no
problem if pa_bluetooth_transport_set_state() just returns if the
transport is NULL (currently it asserts).
Do you still think I should move the transport release before
pa_thread_mq_done()?
