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

Mon Jul 17 17:44:42 UTC 2017

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.

-- 
Tanu

https://www.patreon.com/tanuk