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.
Reviewed-by: Miao-chen Chou <[email protected]>
---
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
Hi Howard,
On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <[email protected]> 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 <[email protected]>
> ---
> 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
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
<[email protected]> wrote:
>
> Hi Howard,
>
> On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <[email protected]> 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 <[email protected]>
> > ---
> > 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
Hi Howard,
On Mon, Jul 12, 2021 at 1:17 AM Yun-hao Chung <[email protected]> wrote:
>
> 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?
Yep, afaik when Ive implemented the service plugin I did the
blocking/allowing at the service level, not sure if you want to
replicate that.
> On Fri, Jul 9, 2021 at 1:49 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Howard,
> >
> > On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
--
Luiz Augusto von Dentz