Re: [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed

[Yun-hao Chung <howardchung@google.com>]

I agree this is a trick for CrOS and probably not suitable for
upstreaming. If we want to allow/disallow profiles without
reprobing/removing them, here is what we need to do:
For each profile in profiles/, reject the connection if its UUID is
not allowed. Note that checking the UUID in btd_request_authorization
is not enough since some profiles like profiles/health/mcap.c don't
call btd_request_authorization.
The same check will need to be added in src/profiles.c as well so that
we can also manage external profiles.
Does that make sense?

On Fri, Jul 9, 2021 at 1:49 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Howard,
>
> On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
> >
> > When A2DP source profile is removed from adapter, a2dp_server and
> > everything inside the object will be removed, which also releases all
> > MediaEndpoints and MediaPlayer. When A2DP source profile is re-added,
> > although a2dp_server will be created, clients are not able to know they
> > can register their endpoints and player by then.
> >
> > This patch handles this case by unregistering Media1 interface
> > when we remove a2dp_server, and register it back when a2dp_source is
> > allowed.
>
> This sounds more like a bug fix for a regression introduced by the
> very set, so Id recommend fixup the original patch that introduced the
> problem, also Im afraid there could other instances like this perhaps
> it would have been better to propagate the allow/block to the profiles
> that way they don't have to be reprobed, I also have my doubts clients
> would react properly to Media1 disappearing and appearing again so Id
> leave it up if there is any endpoint/player registered to avoid having
> them to re-register.
>
> >
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > ---
> > perform following steps
> > 1. SetServiceAllowList to empty list
> > 2. pair with an LE headset, then turn off the headset
> > 3. SetServiceAllowList to "0x1234"
> > 4. SetServiceAllowList to empty list
> > 5. turn on the headset and check if it is reconnected.
> >
> >  profiles/audio/a2dp.c  | 2 ++
> >  profiles/audio/avrcp.c | 3 +++
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > index d31ed845cbe7..26d4f365207e 100644
> > --- a/profiles/audio/a2dp.c
> > +++ b/profiles/audio/a2dp.c
> > @@ -3275,6 +3275,7 @@ static int a2dp_source_server_probe(struct btd_profile *p,
> >  {
> >         struct a2dp_server *server;
> >
> > +       media_register(adapter);
> >         DBG("path %s", adapter_get_path(adapter));
> >
> >         server = find_server(servers, adapter);
> > @@ -3315,6 +3316,7 @@ static void a2dp_source_server_remove(struct btd_profile *p,
> >                 return;
> >
> >         a2dp_server_unregister(server);
> > +       media_unregister(adapter);
> >  }
> >
> >  static int a2dp_sink_server_probe(struct btd_profile *p,
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index ccf34b2207a9..997a5be9a0f4 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -4751,6 +4751,8 @@ static void avrcp_controller_server_remove(struct btd_profile *p,
> >
> >         if (server->tg_record_id == 0)
> >                 avrcp_server_unregister(server);
> > +
> > +       media_unregister(adapter);
> >  }
> >
> >  static int avrcp_controller_server_probe(struct btd_profile *p,
> > @@ -4761,6 +4763,7 @@ static int avrcp_controller_server_probe(struct btd_profile *p,
> >
> >         DBG("path %s", adapter_get_path(adapter));
> >
> > +       media_register(adapter);
> >         server = find_server(servers, adapter);
> >         if (server != NULL)
> >                 goto done;
> > --
> > 2.32.0.93.g670b81a890-goog
> >
>
>
> --
> Luiz Augusto von Dentz