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

[Tanu Kaskinen]

On Thu, 2017-05-25 at 11:36 +0300, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
> 
> Attempt to use Acquire method if available.

After reading the commit message, I don't know what problem this patch
tries to fix. Please write a more informative commit message. Also,
which oFono version introduced the Acquire method?

> ---
>  src/modules/bluetooth/backend-ofono.c | 59 +++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 16 deletions(-)
> 
> diff --git a/src/modules/bluetooth/backend-ofono.c b/src/modules/bluetooth/backend-ofono.c
> index 6e9a366..d098402 100644
> --- a/src/modules/bluetooth/backend-ofono.c
> +++ b/src/modules/bluetooth/backend-ofono.c
> @@ -150,6 +150,46 @@ static int socket_accept(int sock)
>      return 0;
>  }
>  
> +static int card_acquire(struct hf_audio_card *card) {
> +    pa_bluetooth_transport *t = card->transport;
> +    DBusMessage *m, *r;
> +    DBusError err;
> +
> +    if (card->connecting)
> +        return -EAGAIN;
> +
> +    /* Try acquiring the stream first */
> +    dbus_error_init(&err);

err is never freed.

> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", "Acquire"));

m is never unreffed.

> +    r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), m, -1, &err);

r is never unreffed.

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

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

> +                         DBUS_TYPE_INVALID) == true)) {
> +        return card->fd;
> +    } else
> +        return -1;

An error message should be logged.

> +
> +    /* Fallback to Connect as this might be an old version of ofono */
> +    card->connecting = true;
> +
> +    dbus_error_init(&err);

err is never freed.

> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", "Connect"));

m is never unreffed.

> +    r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), m, -1, &err);

r is never unreffed.

> +    if (!r)
> +        return -1;

An error message should be logged.

-- 
Tanu

https://www.patreon.com/tanuk