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

[Tanu Kaskinen]

On Fri, 2018-03-09 at 14:12 +0200, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
> 
> Attempt to use Acquire method if available.

Quoting my previous review:

"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 | 92 +++++++++++++++++++++++++++--------
>  1 file changed, 72 insertions(+), 20 deletions(-)
> 
> diff --git a/src/modules/bluetooth/backend-ofono.c b/src/modules/bluetooth/backend-ofono.c
> index 2c51497f3..49906560e 100644
> --- a/src/modules/bluetooth/backend-ofono.c
> +++ b/src/modules/bluetooth/backend-ofono.c
> @@ -150,35 +150,87 @@ static int socket_accept(int sock)
>      return 0;
>  }
>  
> -static int hf_audio_agent_transport_acquire(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
> -    struct hf_audio_card *card = t->userdata;
> -    int err;
> +static DBusMessage *card_send(struct hf_audio_card *card, const char *method, DBusError *err)
> +{
> +    pa_bluetooth_transport *t = card->transport;
> +    DBusMessage *m, *r;
>  
> -    pa_assert(card);
> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", method));
> +    r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), m, -1, err);
> +    dbus_message_unref(m);
>  
> -    if (!optional && card->fd < 0) {
> -        DBusMessage *m, *r;
> -        DBusError derr;
> +    return r;
> +}
>  
> -        if (card->connecting)
> -            return -EAGAIN;
> +static int card_acquire(struct hf_audio_card *card) {
> +    int fd;
> +    uint8_t codec;
> +    DBusMessage *r;
> +    DBusError err;
>  
> -        card->connecting = true;
> +    if (card->connecting)
> +        return -EAGAIN;
>  
> -        dbus_error_init(&derr);
> -        pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", "Connect"));
> -        r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), m, -1, &derr);
> -        dbus_message_unref(m);
> -        m = NULL;
> +    /* Try acquiring the stream first */
> +    dbus_error_init(&err);
> +    r = card_send(card, "Acquire", &err);
>  
> -        if (!r)
> +    if (!r) {
> +        if (!pa_streq(err.name, DBUS_ERROR_UNKNOWN_METHOD)) {
> +            pa_log_error("Failed to acquire %s: %s", err.name, err.message);
> +            dbus_error_free(&err);
>              return -1;
> -
> +        }
> +    } else if ((dbus_message_get_args(r, NULL, DBUS_TYPE_UNIX_FD, &fd,
> +                                      DBUS_TYPE_BYTE, &codec,
> +                                      DBUS_TYPE_INVALID) == true)) {
> +        dbus_message_unref(r);
> +        if (card->codec != HFP_AUDIO_CODEC_CVSD) {

You have to check codec, not card->codec.

> +            pa_log_error("Invalid codec: %u", card->codec);
> +            /* shutdown to make sure connection is dropped immediately */
> +            shutdown(card->fd, SHUT_RDWR);
> +            close(card->fd);
> +            card->fd = -1;
> +        }
> +        return card->fd;

Have you tested this patch? To me this looks like this can never work,
because on success card->fd is not updated (nor card->codec).

By the way, why is the codec stored in both hf_audio_card and
pa_bluetooth_transport? Wouldn't it be enough to store it in
pa_bluetooth_transport?

> +    } else {
> +        pa_log_error("Unable to acquire");
>          dbus_message_unref(r);
> -        r = NULL;
> +        return -1;
> +    }
> +
> +    dbus_error_free(&err);
>  
> -        if (card->connecting)
> -            return -EAGAIN;
> +    /* Fallback to Connect as this might be an old version of ofono */
> +    card->connecting = true;
> +
> +    dbus_error_init(&err);
> +    card_send(card, "Connect", &err);

You forgot to assign the card_send() return value to r.

-- 
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk