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
