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
