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

[Tanu Kaskinen]

On Fri, 2018-03-16 at 15:33 +0200, Luiz Augusto von Dentz wrote:
> Hi Tanu,
> 
> On Thu, Mar 15, 2018 at 9:54 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > 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?"
> 
> Will add to the patch, I thought this was clear that we cannot send
> errors as signals which means if there is an error on Connect
> NewConnection may never be emitted leaving the card in connecting
> forever.

This wasn't clear to me.

By the way, you're probably talking about some error that happens after
a successful Connect call but before the NewConnection signal, but I
noticed that if Connect() itself fails, we don't set card->connecting
to false. Shouldn't we do that? (This problem existed already before
your patch.)

> > > +            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?
> 
> Perhaps because the card->codec stores the codec during NewConnection,
> at which point the transport may not be ready to resume after all we
> are the main thread no the IO, we could store it directly on the
> transport if it wasn't for this detail or if we could drop backward
> compatibility.

card->codec is only set in card_acquire(). card_acquire() is only
called by hf_audio_agent_transport_acquire(). This is what
hf_audio_agent_transport_acquire() does:

    if (!optional && card->fd < 0) {
        err = card_acquire(card);
        if (err < 0)
            return err;
    }

    ...

    t->codec = card->codec;

card->codec isn't used anywhere else. So first card_acquire() sets
card->codec and then it's immediately copied to t->codec.
card_acquire() could as well directly set t->codec, and card->codec
could be removed. I don't think there are concurrency issues related to
t->codec, but if there are, the code is buggy in any case, because t-
>codec is set from the same thread context as card->codec.

It's not clear to me what you meant with the backward compatibility
comment. Is NewConnection not used at all by ofono if we use Acquire?
If so, we shouldn't remove it right away, but I think it would be good
to have a comment somewhere in the code that reminds that the
NewConnection method is only used by ofono versions older than
<version>, released on <date>.

-- 
Tanu

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