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

[Tanu Kaskinen]

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:
> > > +    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.

The documentation doesn't say that the ownership is transferred, and I
also had a look at the libdbus code, and it looks to me that indeed no
such transfer occurs.

> > > +    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.

-- 
Tanu

https://www.patreon.com/tanuk