Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210708062314.245754-1-howardchung@google.com>
 <20210708142059.Bluez.v1.6.I123189fa93aad2c2d1be645d38dbc494aa15563c@changeid>
 <CABBYNZ+_hYCZX=QMbrYALK6QwVnxcT4-XM-aaFvNHxC06fR2vQ@mail.gmail.com>
In-Reply-To: <CABBYNZ+_hYCZX=QMbrYALK6QwVnxcT4-XM-aaFvNHxC06fR2vQ@mail.gmail.com>
From:   Yun-hao Chung <howardchung@google.com>
Date:   Mon, 12 Jul 2021 16:16:55 +0800
Message-ID: <CAPHZWUfQSECsHDB4AB2OfhTeRra9JoiwK_gqxysD1nrqtknyyw@mail.gmail.com>
Subject: Re: [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp
 source disallowed
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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