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:
>> > > + 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.
Nevermind, it seems my memory is failing me.
>> > > + 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.
> --
> Tanu
>
> https://www.patreon.com/tanuk
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
--
Luiz Augusto von Dentz
