[PATCH v3 1/6] a2dp: Fix crash when endpoint respond with an error

[PATCH v3 1/6] a2dp: Fix crash when endpoint respond with an error

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If endpoint respond with an error the callback will be called with size
set to -1 not 0.
---
 profiles/audio/a2dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index d0676b577..74ada3bbe 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -2418,7 +2418,7 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
 	struct avdtp_media_codec_capability *codec;
 	int err;
 
-	if (size) {
+	if (size >= 0) {
 		caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
 		goto done;
 	}
-- 
2.20.1

[PATCH v3 2/6] a2dp: Try aborting when switching endpoints

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If ongoing stream cannot be closed try aborting since the setup may
still be ongoing.
---
 profiles/audio/a2dp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 74ada3bbe..d0913b1ec 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -1257,6 +1257,11 @@ static void abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	if (!setup)
 		return;
 
+	if (setup->reconfigure) {
+		g_timeout_add(RECONFIGURE_TIMEOUT, a2dp_reconfigure, setup);
+		return;
+	}
+
 	setup_unref(setup);
 }
 
@@ -1642,8 +1647,12 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
 
 			err = avdtp_close(chan->session, tmp->stream, FALSE);
 			if (err < 0) {
-				error("avdtp_close: %s", strerror(-err));
-				goto fail;
+				err = avdtp_abort(chan->session, tmp->stream);
+				if (err < 0) {
+					error("avdtp_abort: %s",
+							strerror(-err));
+					goto fail;
+				}
 			}
 
 			setup->reconfigure = TRUE;
-- 
2.20.1

[PATCH v3 3/6] a2dp: Update last used on open indication

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This updates the last used endpoint also when receiving an open
request from the remote end.
---
 profiles/audio/a2dp.c | 55 +++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index d0913b1ec..2ce7c06cb 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -836,32 +836,6 @@ static void getconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 		DBG("Source %p: Set_Configuration_Cfm", sep);
 }
 
-static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
-				struct avdtp_stream *stream, uint8_t *err,
-				void *user_data)
-{
-	struct a2dp_sep *a2dp_sep = user_data;
-	struct a2dp_setup *setup;
-
-	if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK)
-		DBG("Sink %p: Open_Ind", sep);
-	else
-		DBG("Source %p: Open_Ind", sep);
-
-	setup = a2dp_setup_get(session);
-	if (!setup)
-		return FALSE;
-
-	setup->stream = stream;
-
-	if (setup->reconfigure)
-		setup->reconfigure = FALSE;
-
-	finalize_config(setup);
-
-	return TRUE;
-}
-
 static bool match_remote_sep(const void *data, const void *user_data)
 {
 	const struct a2dp_remote_sep *sep = data;
@@ -916,6 +890,35 @@ static void update_last_used(struct a2dp_channel *chan,
 	store_last_used(chan, rsep);
 }
 
+static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
+				struct avdtp_stream *stream, uint8_t *err,
+				void *user_data)
+{
+	struct a2dp_sep *a2dp_sep = user_data;
+	struct a2dp_setup *setup;
+
+	if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK)
+		DBG("Sink %p: Open_Ind", sep);
+	else
+		DBG("Source %p: Open_Ind", sep);
+
+	setup = a2dp_setup_get(session);
+	if (!setup)
+		return FALSE;
+
+	setup->stream = stream;
+
+	if (!err && setup->chan)
+		update_last_used(setup->chan, stream);
+
+	if (setup->reconfigure)
+		setup->reconfigure = FALSE;
+
+	finalize_config(setup);
+
+	return TRUE;
+}
+
 static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 			struct avdtp_stream *stream, struct avdtp_error *err,
 			void *user_data)
-- 
2.20.1

[PATCH v3 4/6] a2dp: Fix reconfiguring when there multiple devices connected

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

When there are multiple devices connected streams need to be matched
with the sessions they belong.
---
 profiles/audio/a2dp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 2ce7c06cb..f89105a8d 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -1648,6 +1648,10 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
 							tmp->user_data)))
 				return -EPERM;
 
+			/* Check if stream is for the channel */
+			if (!avdtp_has_stream(chan->session, tmp->stream))
+				continue;
+
 			err = avdtp_close(chan->session, tmp->stream, FALSE);
 			if (err < 0) {
 				err = avdtp_abort(chan->session, tmp->stream);
-- 
2.20.1

[PATCH v3 5/6] a2dp: Fix useless statement

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Checking for NULL path doesn't really matter since NULL is returned
anyway.
---
 profiles/audio/a2dp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index f89105a8d..b54c50315 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -2998,8 +2998,7 @@ struct btd_device *a2dp_setup_get_device(struct a2dp_setup *setup)
 const char *a2dp_setup_remote_path(struct a2dp_setup *setup)
 {
 	if (setup->rsep) {
-		if (setup->rsep->path)
-			return setup->rsep->path;
+		return setup->rsep->path;
 	}
 
 	return NULL;
-- 
2.20.1

[PATCH v3 6/6] a2dp: Reword LastUsed

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

In addition to storing the remote endpoint also store the local one as
well so we it can properly be restore.
---
 profiles/audio/a2dp.c  | 147 +++++++++++++++++++++++++++++++----------
 profiles/audio/avdtp.c |   5 ++
 profiles/audio/avdtp.h |   1 +
 3 files changed, 119 insertions(+), 34 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index b54c50315..046193688 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -139,6 +139,11 @@ struct a2dp_remote_sep {
 	struct avdtp_remote_sep *sep;
 };
 
+struct a2dp_last_used {
+	struct a2dp_sep *lsep;
+	struct a2dp_remote_sep *rsep;
+};
+
 struct a2dp_channel {
 	struct a2dp_server *server;
 	struct btd_device *device;
@@ -148,7 +153,7 @@ struct a2dp_channel {
 	unsigned int auth_id;
 	struct avdtp *session;
 	struct queue *seps;
-	struct a2dp_remote_sep *last_used;
+	struct a2dp_last_used *last_used;
 };
 
 static GSList *servers = NULL;
@@ -214,6 +219,8 @@ static void setup_free(struct a2dp_setup *s)
 		g_io_channel_unref(s->io);
 	}
 
+	queue_destroy(s->eps, NULL);
+
 	setups = g_slist_remove(setups, s);
 	if (s->session)
 		avdtp_unref(s->session);
@@ -844,13 +851,13 @@ static bool match_remote_sep(const void *data, const void *user_data)
 	return sep->sep == rsep;
 }
 
-static void store_last_used(struct a2dp_channel *chan,
-					struct avdtp_remote_sep *rsep)
+static void store_last_used(struct a2dp_channel *chan, uint8_t lseid,
+							uint8_t rseid)
 {
 	GKeyFile *key_file;
 	char filename[PATH_MAX];
 	char dst_addr[18];
-	char value[4];
+	char value[6];
 	char *data;
 	size_t len = 0;
 
@@ -862,7 +869,7 @@ static void store_last_used(struct a2dp_channel *chan,
 	key_file = g_key_file_new();
 	g_key_file_load_from_file(key_file, filename, 0, NULL);
 
-	sprintf(value, "%02hhx", avdtp_get_seid(rsep));
+	sprintf(value, "%02hhx:%02hhx", lseid, rseid);
 
 	g_key_file_set_string(key_file, "Endpoints", "LastUsed", value);
 
@@ -873,21 +880,38 @@ static void store_last_used(struct a2dp_channel *chan,
 	g_key_file_free(key_file);
 }
 
-static void update_last_used(struct a2dp_channel *chan,
-						struct avdtp_stream *stream)
+static void add_last_used(struct a2dp_channel *chan, struct a2dp_sep *lsep,
+				struct a2dp_remote_sep *rsep)
+{
+	if (!chan->last_used)
+		chan->last_used = new0(struct a2dp_last_used, 1);
+
+	chan->last_used->lsep = lsep;
+	chan->last_used->rsep = rsep;
+}
+
+static void update_last_used(struct a2dp_channel *chan, struct a2dp_sep *lsep,
+					struct avdtp_stream *stream)
 {
 	struct avdtp_remote_sep *rsep;
 	struct a2dp_remote_sep *sep;
 
 	rsep = avdtp_stream_get_remote_sep(stream);
-
-	/* Update last used */
 	sep = queue_find(chan->seps, match_remote_sep, rsep);
-	if (chan->last_used == sep)
+	if (!sep) {
+		error("Unable to find remote SEP");
 		return;
+	}
+
+	/* Check if already stored then skip */
+	if (chan->last_used && (chan->last_used->lsep == lsep &&
+				chan->last_used->rsep == sep))
+		return;
+
+	add_last_used(chan, lsep, sep);
 
-	chan->last_used = sep;
-	store_last_used(chan, rsep);
+	store_last_used(chan, avdtp_sep_get_seid(lsep->lsep),
+					avdtp_get_seid(rsep));
 }
 
 static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
@@ -909,7 +933,7 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 	setup->stream = stream;
 
 	if (!err && setup->chan)
-		update_last_used(setup->chan, stream);
+		update_last_used(setup->chan, a2dp_sep, stream);
 
 	if (setup->reconfigure)
 		setup->reconfigure = FALSE;
@@ -944,7 +968,7 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 		if (setup->start)
 			finalize_resume(setup);
 	} else if (setup->chan)
-		update_last_used(setup->chan, stream);
+		update_last_used(setup->chan, a2dp_sep, stream);
 
 	finalize_config(setup);
 
@@ -1483,6 +1507,7 @@ static void channel_free(void *data)
 	avdtp_remove_state_cb(chan->state_id);
 
 	queue_destroy(chan->seps, remove_remote_sep);
+	free(chan->last_used);
 	g_free(chan);
 }
 
@@ -1631,6 +1656,9 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
 	setup->sep = lsep;
 	setup->rsep = rsep;
 
+	g_slist_free_full(setup->caps, g_free);
+	setup->caps = NULL;
+
 	caps_add_codec(&setup->caps, setup->sep->codec, caps, size);
 
 	l = avdtp_get_type(rsep->sep) == AVDTP_SEP_TYPE_SINK ?
@@ -1871,11 +1899,37 @@ static bool match_seid(const void *data, const void *user_data)
 	return avdtp_get_seid(sep->sep) == *seid;
 }
 
+static int match_sep(const void *data, const void *user_data)
+{
+	struct a2dp_sep *sep = (void *) data;
+	const uint8_t *seid = user_data;
+
+	return *seid - avdtp_sep_get_seid(sep->lsep);
+}
+
+static struct a2dp_sep *find_sep_by_seid(struct a2dp_server *server,
+							uint8_t seid)
+{
+	GSList *l;
+
+	l = g_slist_find_custom(server->sources, &seid, match_sep);
+	if (l)
+		return l->data;
+
+	l = g_slist_find_custom(server->sinks, &seid, match_sep);
+	if (l)
+		return l->data;
+
+	return NULL;
+}
+
 static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 								char **seids)
 {
-	struct avdtp_remote_sep *sep;
-	uint8_t seid;
+	struct a2dp_sep *lsep;
+	struct a2dp_remote_sep *sep;
+	struct avdtp_remote_sep *rsep;
+	uint8_t lseid, rseid;
 	char *value;
 
 	if (!seids)
@@ -1884,12 +1938,12 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 	for (; *seids; seids++) {
 		uint8_t type;
 		uint8_t codec;
+		GSList *l = NULL;
 		char caps[256];
 		uint8_t data[128];
 		int i, size;
-		GSList *l = NULL;
 
-		if (sscanf(*seids, "%02hhx", &seid) != 1)
+		if (sscanf(*seids, "%02hhx", &rseid) != 1)
 			continue;
 
 		value = g_key_file_get_string(key_file, "Endpoints", *seids,
@@ -1899,7 +1953,7 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 
 		if (sscanf(value, "%02hhx:%02hhx:%s", &type, &codec,
 								caps) != 3) {
-			warn("Unable to load Endpoint: seid %u", seid);
+			warn("Unable to load Endpoint: seid %u", rseid);
 			g_free(value);
 			continue;
 		}
@@ -1908,7 +1962,8 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 			uint8_t *tmp = data + i / 2;
 
 			if (sscanf(caps + i, "%02hhx", tmp) != 1) {
-				warn("Unable to load Endpoint: seid %u", seid);
+				warn("Unable to load Endpoint: seid %u", rseid);
+				g_free(value);
 				break;
 			}
 		}
@@ -1920,23 +1975,39 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 
 		caps_add_codec(&l, codec, data, size / 2);
 
-		sep = avdtp_register_remote_sep(chan->session, seid, type, l);
-		if (!sep) {
-			warn("Unable to register Endpoint: seid %u", seid);
+		rsep = avdtp_register_remote_sep(chan->session, rseid, type, l);
+		if (!rsep) {
+			warn("Unable to register Endpoint: seid %u", rseid);
 			continue;
 		}
 
-		register_remote_sep(sep, chan);
+		register_remote_sep(rsep, chan);
 	}
 
 	value = g_key_file_get_string(key_file, "Endpoints", "LastUsed", NULL);
 	if (!value)
 		return;
 
-	if (sscanf(value, "%02hhx", &seid) != 1)
+	if (sscanf(value, "%02hhx:%02hhx", &lseid, &rseid) != 2) {
+		warn("Unable to load LastUsed");
+		return;
+	}
+
+	g_free(value);
+
+	lsep = find_sep_by_seid(chan->server, lseid);
+	if (!lsep) {
+		warn("Unable to load LastUsed: lseid %u not found", lseid);
 		return;
+	}
 
-	chan->last_used = queue_find(chan->seps, match_seid, &seid);
+	sep = queue_find(chan->seps, match_seid, &rseid);
+	if (!sep) {
+		warn("Unable to load LastUsed: rseid %u not found", rseid);
+		return;
+	}
+
+	add_last_used(chan, lsep, sep);
 }
 
 static void load_remote_seps(struct a2dp_channel *chan)
@@ -2442,7 +2513,6 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
 	setup->sep = queue_pop_head(setup->eps);
 	if (!setup->sep) {
 		error("Unable to select a valid configuration");
-		queue_destroy(setup->eps, NULL);
 		goto done;
 	}
 
@@ -2453,7 +2523,7 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
 	err = setup->sep->endpoint->select_configuration(setup->sep,
 					codec->data,
 					service->length - sizeof(*codec),
-					setup_ref(setup),
+					setup,
 					select_cb, setup->sep->user_data);
 	if (err == 0)
 		return;
@@ -2493,11 +2563,11 @@ static struct queue *a2dp_find_eps(struct avdtp *session, GSList *list,
 			seps = queue_new();
 
 		/* Prepend last used so it is preferred over others */
-		if (chan->last_used && chan->last_used->sep == rsep)
+		if (chan->last_used && (chan->last_used->lsep == sep &&
+					chan->last_used->rsep->sep == rsep))
 			queue_push_head(seps, sep);
 		else
 			queue_push_tail(seps, sep);
-
 	}
 
 	return seps;
@@ -2566,11 +2636,18 @@ static void store_remote_seps(struct a2dp_channel *chan)
 	key_file = g_key_file_new();
 	g_key_file_load_from_file(key_file, filename, 0, NULL);
 
+	data = g_key_file_get_string(key_file, "Endpoints", "LastUsed", NULL);
+
 	/* Remove current endpoints since it might have changed */
 	g_key_file_remove_group(key_file, "Endpoints", NULL);
 
 	queue_foreach(chan->seps, store_remote_sep, key_file);
 
+	if (data) {
+		g_key_file_set_string(key_file, "Endpoints", "LastUsed", data);
+		g_free(data);
+	}
+
 	data = g_key_file_to_data(key_file, &length, NULL);
 	g_file_set_contents(filename, data, length, NULL);
 
@@ -2656,10 +2733,12 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
 	codec = (struct avdtp_media_codec_capability *) service->data;
 
 	err = setup->sep->endpoint->select_configuration(setup->sep,
-					codec->data,
-					service->length - sizeof(*codec),
-					setup_ref(setup),
-					select_cb, setup->sep->user_data);
+							codec->data,
+							service->length -
+							sizeof(*codec),
+							setup_ref(setup),
+							select_cb,
+							setup->sep->user_data);
 	if (err == 0)
 		return cb_data->id;
 
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index f4c9a2ed8..91b1e4b96 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -3710,6 +3710,11 @@ avdtp_state_t avdtp_sep_get_state(struct avdtp_local_sep *sep)
 	return sep->state;
 }
 
+uint8_t avdtp_sep_get_seid(struct avdtp_local_sep *sep)
+{
+	return sep->info.seid;
+}
+
 struct btd_adapter *avdtp_get_adapter(struct avdtp *session)
 {
 	return device_get_adapter(session->device);
diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
index b03ca9030..ad2cb9bcb 100644
--- a/profiles/audio/avdtp.h
+++ b/profiles/audio/avdtp.h
@@ -296,6 +296,7 @@ struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
 int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep);
 
 avdtp_state_t avdtp_sep_get_state(struct avdtp_local_sep *sep);
+uint8_t avdtp_sep_get_seid(struct avdtp_local_sep *sep);
 
 void avdtp_error_init(struct avdtp_error *err, uint8_t type, int id);
 const char *avdtp_strerror(struct avdtp_error *err);
-- 
2.20.1

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, May 6, 2019 at 3:43 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> In addition to storing the remote endpoint also store the local one as
> well so we it can properly be restore.
> ---
>  profiles/audio/a2dp.c  | 147 +++++++++++++++++++++++++++++++----------
>  profiles/audio/avdtp.c |   5 ++
>  profiles/audio/avdtp.h |   1 +
>  3 files changed, 119 insertions(+), 34 deletions(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index b54c50315..046193688 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -139,6 +139,11 @@ struct a2dp_remote_sep {
>         struct avdtp_remote_sep *sep;
>  };
>
> +struct a2dp_last_used {
> +       struct a2dp_sep *lsep;
> +       struct a2dp_remote_sep *rsep;
> +};
> +
>  struct a2dp_channel {
>         struct a2dp_server *server;
>         struct btd_device *device;
> @@ -148,7 +153,7 @@ struct a2dp_channel {
>         unsigned int auth_id;
>         struct avdtp *session;
>         struct queue *seps;
> -       struct a2dp_remote_sep *last_used;
> +       struct a2dp_last_used *last_used;
>  };
>
>  static GSList *servers = NULL;
> @@ -214,6 +219,8 @@ static void setup_free(struct a2dp_setup *s)
>                 g_io_channel_unref(s->io);
>         }
>
> +       queue_destroy(s->eps, NULL);
> +
>         setups = g_slist_remove(setups, s);
>         if (s->session)
>                 avdtp_unref(s->session);
> @@ -844,13 +851,13 @@ static bool match_remote_sep(const void *data, const void *user_data)
>         return sep->sep == rsep;
>  }
>
> -static void store_last_used(struct a2dp_channel *chan,
> -                                       struct avdtp_remote_sep *rsep)
> +static void store_last_used(struct a2dp_channel *chan, uint8_t lseid,
> +                                                       uint8_t rseid)
>  {
>         GKeyFile *key_file;
>         char filename[PATH_MAX];
>         char dst_addr[18];
> -       char value[4];
> +       char value[6];
>         char *data;
>         size_t len = 0;
>
> @@ -862,7 +869,7 @@ static void store_last_used(struct a2dp_channel *chan,
>         key_file = g_key_file_new();
>         g_key_file_load_from_file(key_file, filename, 0, NULL);
>
> -       sprintf(value, "%02hhx", avdtp_get_seid(rsep));
> +       sprintf(value, "%02hhx:%02hhx", lseid, rseid);
>
>         g_key_file_set_string(key_file, "Endpoints", "LastUsed", value);
>
> @@ -873,21 +880,38 @@ static void store_last_used(struct a2dp_channel *chan,
>         g_key_file_free(key_file);
>  }
>
> -static void update_last_used(struct a2dp_channel *chan,
> -                                               struct avdtp_stream *stream)
> +static void add_last_used(struct a2dp_channel *chan, struct a2dp_sep *lsep,
> +                               struct a2dp_remote_sep *rsep)
> +{
> +       if (!chan->last_used)
> +               chan->last_used = new0(struct a2dp_last_used, 1);
> +
> +       chan->last_used->lsep = lsep;
> +       chan->last_used->rsep = rsep;
> +}
> +
> +static void update_last_used(struct a2dp_channel *chan, struct a2dp_sep *lsep,
> +                                       struct avdtp_stream *stream)
>  {
>         struct avdtp_remote_sep *rsep;
>         struct a2dp_remote_sep *sep;
>
>         rsep = avdtp_stream_get_remote_sep(stream);
> -
> -       /* Update last used */
>         sep = queue_find(chan->seps, match_remote_sep, rsep);
> -       if (chan->last_used == sep)
> +       if (!sep) {
> +               error("Unable to find remote SEP");
>                 return;
> +       }
> +
> +       /* Check if already stored then skip */
> +       if (chan->last_used && (chan->last_used->lsep == lsep &&
> +                               chan->last_used->rsep == sep))
> +               return;
> +
> +       add_last_used(chan, lsep, sep);
>
> -       chan->last_used = sep;
> -       store_last_used(chan, rsep);
> +       store_last_used(chan, avdtp_sep_get_seid(lsep->lsep),
> +                                       avdtp_get_seid(rsep));
>  }
>
>  static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
> @@ -909,7 +933,7 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
>         setup->stream = stream;
>
>         if (!err && setup->chan)
> -               update_last_used(setup->chan, stream);
> +               update_last_used(setup->chan, a2dp_sep, stream);
>
>         if (setup->reconfigure)
>                 setup->reconfigure = FALSE;
> @@ -944,7 +968,7 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
>                 if (setup->start)
>                         finalize_resume(setup);
>         } else if (setup->chan)
> -               update_last_used(setup->chan, stream);
> +               update_last_used(setup->chan, a2dp_sep, stream);
>
>         finalize_config(setup);
>
> @@ -1483,6 +1507,7 @@ static void channel_free(void *data)
>         avdtp_remove_state_cb(chan->state_id);
>
>         queue_destroy(chan->seps, remove_remote_sep);
> +       free(chan->last_used);
>         g_free(chan);
>  }
>
> @@ -1631,6 +1656,9 @@ static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
>         setup->sep = lsep;
>         setup->rsep = rsep;
>
> +       g_slist_free_full(setup->caps, g_free);
> +       setup->caps = NULL;
> +
>         caps_add_codec(&setup->caps, setup->sep->codec, caps, size);
>
>         l = avdtp_get_type(rsep->sep) == AVDTP_SEP_TYPE_SINK ?
> @@ -1871,11 +1899,37 @@ static bool match_seid(const void *data, const void *user_data)
>         return avdtp_get_seid(sep->sep) == *seid;
>  }
>
> +static int match_sep(const void *data, const void *user_data)
> +{
> +       struct a2dp_sep *sep = (void *) data;
> +       const uint8_t *seid = user_data;
> +
> +       return *seid - avdtp_sep_get_seid(sep->lsep);
> +}
> +
> +static struct a2dp_sep *find_sep_by_seid(struct a2dp_server *server,
> +                                                       uint8_t seid)
> +{
> +       GSList *l;
> +
> +       l = g_slist_find_custom(server->sources, &seid, match_sep);
> +       if (l)
> +               return l->data;
> +
> +       l = g_slist_find_custom(server->sinks, &seid, match_sep);
> +       if (l)
> +               return l->data;
> +
> +       return NULL;
> +}
> +
>  static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
>                                                                 char **seids)
>  {
> -       struct avdtp_remote_sep *sep;
> -       uint8_t seid;
> +       struct a2dp_sep *lsep;
> +       struct a2dp_remote_sep *sep;
> +       struct avdtp_remote_sep *rsep;
> +       uint8_t lseid, rseid;
>         char *value;
>
>         if (!seids)
> @@ -1884,12 +1938,12 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
>         for (; *seids; seids++) {
>                 uint8_t type;
>                 uint8_t codec;
> +               GSList *l = NULL;
>                 char caps[256];
>                 uint8_t data[128];
>                 int i, size;
> -               GSList *l = NULL;
>
> -               if (sscanf(*seids, "%02hhx", &seid) != 1)
> +               if (sscanf(*seids, "%02hhx", &rseid) != 1)
>                         continue;
>
>                 value = g_key_file_get_string(key_file, "Endpoints", *seids,
> @@ -1899,7 +1953,7 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
>
>                 if (sscanf(value, "%02hhx:%02hhx:%s", &type, &codec,
>                                                                 caps) != 3) {
> -                       warn("Unable to load Endpoint: seid %u", seid);
> +                       warn("Unable to load Endpoint: seid %u", rseid);
>                         g_free(value);
>                         continue;
>                 }
> @@ -1908,7 +1962,8 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
>                         uint8_t *tmp = data + i / 2;
>
>                         if (sscanf(caps + i, "%02hhx", tmp) != 1) {
> -                               warn("Unable to load Endpoint: seid %u", seid);
> +                               warn("Unable to load Endpoint: seid %u", rseid);
> +                               g_free(value);
>                                 break;
>                         }
>                 }
> @@ -1920,23 +1975,39 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
>
>                 caps_add_codec(&l, codec, data, size / 2);
>
> -               sep = avdtp_register_remote_sep(chan->session, seid, type, l);
> -               if (!sep) {
> -                       warn("Unable to register Endpoint: seid %u", seid);
> +               rsep = avdtp_register_remote_sep(chan->session, rseid, type, l);
> +               if (!rsep) {
> +                       warn("Unable to register Endpoint: seid %u", rseid);
>                         continue;
>                 }
>
> -               register_remote_sep(sep, chan);
> +               register_remote_sep(rsep, chan);
>         }
>
>         value = g_key_file_get_string(key_file, "Endpoints", "LastUsed", NULL);
>         if (!value)
>                 return;
>
> -       if (sscanf(value, "%02hhx", &seid) != 1)
> +       if (sscanf(value, "%02hhx:%02hhx", &lseid, &rseid) != 2) {
> +               warn("Unable to load LastUsed");
> +               return;
> +       }
> +
> +       g_free(value);
> +
> +       lsep = find_sep_by_seid(chan->server, lseid);
> +       if (!lsep) {
> +               warn("Unable to load LastUsed: lseid %u not found", lseid);
>                 return;
> +       }
>
> -       chan->last_used = queue_find(chan->seps, match_seid, &seid);
> +       sep = queue_find(chan->seps, match_seid, &rseid);
> +       if (!sep) {
> +               warn("Unable to load LastUsed: rseid %u not found", rseid);
> +               return;
> +       }
> +
> +       add_last_used(chan, lsep, sep);
>  }
>
>  static void load_remote_seps(struct a2dp_channel *chan)
> @@ -2442,7 +2513,6 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
>         setup->sep = queue_pop_head(setup->eps);
>         if (!setup->sep) {
>                 error("Unable to select a valid configuration");
> -               queue_destroy(setup->eps, NULL);
>                 goto done;
>         }
>
> @@ -2453,7 +2523,7 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
>         err = setup->sep->endpoint->select_configuration(setup->sep,
>                                         codec->data,
>                                         service->length - sizeof(*codec),
> -                                       setup_ref(setup),
> +                                       setup,
>                                         select_cb, setup->sep->user_data);
>         if (err == 0)
>                 return;
> @@ -2493,11 +2563,11 @@ static struct queue *a2dp_find_eps(struct avdtp *session, GSList *list,
>                         seps = queue_new();
>
>                 /* Prepend last used so it is preferred over others */
> -               if (chan->last_used && chan->last_used->sep == rsep)
> +               if (chan->last_used && (chan->last_used->lsep == sep &&
> +                                       chan->last_used->rsep->sep == rsep))
>                         queue_push_head(seps, sep);
>                 else
>                         queue_push_tail(seps, sep);
> -
>         }
>
>         return seps;
> @@ -2566,11 +2636,18 @@ static void store_remote_seps(struct a2dp_channel *chan)
>         key_file = g_key_file_new();
>         g_key_file_load_from_file(key_file, filename, 0, NULL);
>
> +       data = g_key_file_get_string(key_file, "Endpoints", "LastUsed", NULL);
> +
>         /* Remove current endpoints since it might have changed */
>         g_key_file_remove_group(key_file, "Endpoints", NULL);
>
>         queue_foreach(chan->seps, store_remote_sep, key_file);
>
> +       if (data) {
> +               g_key_file_set_string(key_file, "Endpoints", "LastUsed", data);
> +               g_free(data);
> +       }
> +
>         data = g_key_file_to_data(key_file, &length, NULL);
>         g_file_set_contents(filename, data, length, NULL);
>
> @@ -2656,10 +2733,12 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
>         codec = (struct avdtp_media_codec_capability *) service->data;
>
>         err = setup->sep->endpoint->select_configuration(setup->sep,
> -                                       codec->data,
> -                                       service->length - sizeof(*codec),
> -                                       setup_ref(setup),
> -                                       select_cb, setup->sep->user_data);
> +                                                       codec->data,
> +                                                       service->length -
> +                                                       sizeof(*codec),
> +                                                       setup_ref(setup),
> +                                                       select_cb,
> +                                                       setup->sep->user_data);
>         if (err == 0)
>                 return cb_data->id;
>
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index f4c9a2ed8..91b1e4b96 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -3710,6 +3710,11 @@ avdtp_state_t avdtp_sep_get_state(struct avdtp_local_sep *sep)
>         return sep->state;
>  }
>
> +uint8_t avdtp_sep_get_seid(struct avdtp_local_sep *sep)
> +{
> +       return sep->info.seid;
> +}
> +
>  struct btd_adapter *avdtp_get_adapter(struct avdtp *session)
>  {
>         return device_get_adapter(session->device);
> diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
> index b03ca9030..ad2cb9bcb 100644
> --- a/profiles/audio/avdtp.h
> +++ b/profiles/audio/avdtp.h
> @@ -296,6 +296,7 @@ struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
>  int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep);
>
>  avdtp_state_t avdtp_sep_get_state(struct avdtp_local_sep *sep);
> +uint8_t avdtp_sep_get_seid(struct avdtp_local_sep *sep);
>
>  void avdtp_error_init(struct avdtp_error *err, uint8_t type, int id);
>  const char *avdtp_strerror(struct avdtp_error *err);
> --
> 2.20.1

I hope this fixes the problems you have been seeing, it should at
least take care of the issues with wrong order of SelectConfiguration
and restoring the exact same endpoint used last time.


-- 
Luiz Augusto von Dentz

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On Monday 06 May 2019 15:46:03 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> I hope this fixes the problems you have been seeing, it should at
> least take care of the issues with wrong order of SelectConfiguration
> and restoring the exact same endpoint used last time.

Hi Luiz! Great, this should make bluez to be more deterministic.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On Monday 06 May 2019 15:02:25 Pali Rohár wrote:
> On Monday 06 May 2019 15:46:03 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> > 
> > I hope this fixes the problems you have been seeing, it should at
> > least take care of the issues with wrong order of SelectConfiguration
> > and restoring the exact same endpoint used last time.
> 
> Hi Luiz! Great, this should make bluez to be more deterministic.

Hi! Now I looked at it and in cache file I see:

LastUsed=04:01

What would happen when A2DP application (e.g. pulseaudio) register DBus
endpoints in different order? Or e.g. do not register some endpoints due
to missing codec librayr (aptX).

I guess that in this case LastUsed information stops working...

Should not be there stored rather dbus endpoint name identifier?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Luiz Augusto von Dentz]

Hi Pali,

On Tue, May 7, 2019 at 11:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Monday 06 May 2019 15:02:25 Pali Rohár wrote:
> > On Monday 06 May 2019 15:46:03 Luiz Augusto von Dentz wrote:
> > > Hi Pali,
> > >
> > > I hope this fixes the problems you have been seeing, it should at
> > > least take care of the issues with wrong order of SelectConfiguration
> > > and restoring the exact same endpoint used last time.
> >
> > Hi Luiz! Great, this should make bluez to be more deterministic.
>
> Hi! Now I looked at it and in cache file I see:
>
> LastUsed=04:01
>
> What would happen when A2DP application (e.g. pulseaudio) register DBus
> endpoints in different order? Or e.g. do not register some endpoints due
> to missing codec librayr (aptX).
>
> I guess that in this case LastUsed information stops working...

It would most likely fail at SelectConfiguration and then try with the
other endpoints.

>
> Should not be there stored rather dbus endpoint name identifier?

Originally I tried to avoid having the local endpoints because of this
problem, but now that SelectConfiguration can fail it shouldn't really
be a problem. At least with the seid if you have a system that didn't
changed the order or number of endpoints it will keep working as
expected, if we choose to encode the D-Bus connection, etc, as soon as
PA is restarted, the system is rebooted, etc, the D-Bus connection may
have changed making the stored values in LastUsed invalid. Anyway
chances are the LastUsed is only invalidated if you update PA, in
which case there could be new endpoints or a change in their order and
package can also provide a script to clear the LastUsed if that
happens, but then again LastUsed setting does not actually store the
configuration just the endpoint so SelectConfiguration has control
over the configuration.

-- 
Luiz Augusto von Dentz

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 3820 bytes --]

On Tuesday 07 May 2019 13:13:17 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Tue, May 7, 2019 at 11:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > On Monday 06 May 2019 15:02:25 Pali Rohár wrote:
> > > On Monday 06 May 2019 15:46:03 Luiz Augusto von Dentz wrote:
> > > > Hi Pali,
> > > >
> > > > I hope this fixes the problems you have been seeing, it should at
> > > > least take care of the issues with wrong order of SelectConfiguration
> > > > and restoring the exact same endpoint used last time.
> > >
> > > Hi Luiz! Great, this should make bluez to be more deterministic.
> >
> > Hi! Now I looked at it and in cache file I see:
> >
> > LastUsed=04:01
> >
> > What would happen when A2DP application (e.g. pulseaudio) register DBus
> > endpoints in different order? Or e.g. do not register some endpoints due
> > to missing codec librayr (aptX).
> >
> > I guess that in this case LastUsed information stops working...
> 
> It would most likely fail at SelectConfiguration and then try with the
> other endpoints.

Yes, that is truth. My point is just about validity of LastUsed value.

> >
> > Should not be there stored rather dbus endpoint name identifier?
> 
> Originally I tried to avoid having the local endpoints because of this
> problem, but now that SelectConfiguration can fail it shouldn't really
> be a problem. At least with the seid if you have a system that didn't
> changed the order or number of endpoints it will keep working as
> expected,

But this may happen. And such thing is allowed. Any application,
including unprivileged can register own new endpoint to bluez. It is not
specific to pulseaudio. And in my opinion central bluetooth daemon which
expose such functionality should be robust and be prepared that
application on "other side of dbus IPC" does not have to be well
behaved.

> if we choose to encode the D-Bus connection, etc, as soon as
> PA is restarted, the system is rebooted, etc, the D-Bus connection may
> have changed making the stored values in LastUsed invalid.

That is truth.

My idea was to encode just dbus path of local dbus endpoint. We can say
(in IPC API) that client application should preserve dbus path for one
same endpoint between dbus daemon / computer restarts. Because otherwise
functionality of "choose last used endpoint" would not work.

This is less strict requirement and current one (that registration order
of all existing applications in system must be same across reboots),
less error prone and still should be easily implemented.

> Anyway
> chances are the LastUsed is only invalidated if you update PA, in

Or once we include support for dynamic codec loading (based on encoder
library presence in system), installation of any irrelevant application
may bring a new supported codec and therefore a new endpoint. So codecs
can become in any order...

Or another example, when different application (not PA) register also
some endpoint.

> which case there could be new endpoints or a change in their order and
> package can also provide a script to clear the LastUsed if that
> happens, but then again LastUsed setting does not actually store the
> configuration just the endpoint so SelectConfiguration has control
> over the configuration.

Yes, whole thing is only about initial codec selection. In the worst
case user would have pre-selected different codec as which was last
used.

But I think that storing dbus path of endpoint as described above should
be more reliable solution.

Applications (e.g. PA) talking to bluetooth daemon does not register or
request directly SEID. Instead they register their dbus path and dbus
connection and bluetooth daemon later allocates SEID for that path.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Pali Rohár]

On Tuesday 07 May 2019 20:57:40 Pali Rohár wrote:
> On Tuesday 07 May 2019 13:13:17 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> > 
> > On Tue, May 7, 2019 at 11:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > On Monday 06 May 2019 15:02:25 Pali Rohár wrote:
> > > > On Monday 06 May 2019 15:46:03 Luiz Augusto von Dentz wrote:
> > > > > Hi Pali,
> > > > >
> > > > > I hope this fixes the problems you have been seeing, it should at
> > > > > least take care of the issues with wrong order of SelectConfiguration
> > > > > and restoring the exact same endpoint used last time.
> > > >
> > > > Hi Luiz! Great, this should make bluez to be more deterministic.
> > >
> > > Hi! Now I looked at it and in cache file I see:
> > >
> > > LastUsed=04:01
> > >
> > > What would happen when A2DP application (e.g. pulseaudio) register DBus
> > > endpoints in different order? Or e.g. do not register some endpoints due
> > > to missing codec librayr (aptX).
> > >
> > > I guess that in this case LastUsed information stops working...
> > 
> > It would most likely fail at SelectConfiguration and then try with the
> > other endpoints.
> 
> Yes, that is truth. My point is just about validity of LastUsed value.
> 
> > >
> > > Should not be there stored rather dbus endpoint name identifier?
> > 
> > Originally I tried to avoid having the local endpoints because of this
> > problem, but now that SelectConfiguration can fail it shouldn't really
> > be a problem. At least with the seid if you have a system that didn't
> > changed the order or number of endpoints it will keep working as
> > expected,
> 
> But this may happen. And such thing is allowed. Any application,
> including unprivileged can register own new endpoint to bluez. It is not
> specific to pulseaudio. And in my opinion central bluetooth daemon which
> expose such functionality should be robust and be prepared that
> application on "other side of dbus IPC" does not have to be well
> behaved.
> 
> > if we choose to encode the D-Bus connection, etc, as soon as
> > PA is restarted, the system is rebooted, etc, the D-Bus connection may
> > have changed making the stored values in LastUsed invalid.
> 
> That is truth.
> 
> My idea was to encode just dbus path of local dbus endpoint. We can say
> (in IPC API) that client application should preserve dbus path for one
> same endpoint between dbus daemon / computer restarts. Because otherwise
> functionality of "choose last used endpoint" would not work.
> 
> This is less strict requirement and current one (that registration order
> of all existing applications in system must be same across reboots),
> less error prone and still should be easily implemented.

So what about this my idea? Is there anything wrong with such thing?

> > Anyway
> > chances are the LastUsed is only invalidated if you update PA, in
> 
> Or once we include support for dynamic codec loading (based on encoder
> library presence in system), installation of any irrelevant application
> may bring a new supported codec and therefore a new endpoint. So codecs
> can become in any order...
> 
> Or another example, when different application (not PA) register also
> some endpoint.
> 
> > which case there could be new endpoints or a change in their order and
> > package can also provide a script to clear the LastUsed if that
> > happens, but then again LastUsed setting does not actually store the
> > configuration just the endpoint so SelectConfiguration has control
> > over the configuration.
> 
> Yes, whole thing is only about initial codec selection. In the worst
> case user would have pre-selected different codec as which was last
> used.
> 
> But I think that storing dbus path of endpoint as described above should
> be more reliable solution.
> 
> Applications (e.g. PA) talking to bluetooth daemon does not register or
> request directly SEID. Instead they register their dbus path and dbus
> connection and bluetooth daemon later allocates SEID for that path.
> 

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Luiz Augusto von Dentz]

Hi Pali,

On Fri, Jun 7, 2019 at 4:04 PM Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Tuesday 07 May 2019 20:57:40 Pali Rohár wrote:
> > On Tuesday 07 May 2019 13:13:17 Luiz Augusto von Dentz wrote:
> > > Hi Pali,
> > >
> > > On Tue, May 7, 2019 at 11:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > >
> > > > On Monday 06 May 2019 15:02:25 Pali Rohár wrote:
> > > > > On Monday 06 May 2019 15:46:03 Luiz Augusto von Dentz wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > I hope this fixes the problems you have been seeing, it should at
> > > > > > least take care of the issues with wrong order of SelectConfiguration
> > > > > > and restoring the exact same endpoint used last time.
> > > > >
> > > > > Hi Luiz! Great, this should make bluez to be more deterministic.
> > > >
> > > > Hi! Now I looked at it and in cache file I see:
> > > >
> > > > LastUsed=04:01
> > > >
> > > > What would happen when A2DP application (e.g. pulseaudio) register DBus
> > > > endpoints in different order? Or e.g. do not register some endpoints due
> > > > to missing codec librayr (aptX).
> > > >
> > > > I guess that in this case LastUsed information stops working...
> > >
> > > It would most likely fail at SelectConfiguration and then try with the
> > > other endpoints.
> >
> > Yes, that is truth. My point is just about validity of LastUsed value.
> >
> > > >
> > > > Should not be there stored rather dbus endpoint name identifier?
> > >
> > > Originally I tried to avoid having the local endpoints because of this
> > > problem, but now that SelectConfiguration can fail it shouldn't really
> > > be a problem. At least with the seid if you have a system that didn't
> > > changed the order or number of endpoints it will keep working as
> > > expected,
> >
> > But this may happen. And such thing is allowed. Any application,
> > including unprivileged can register own new endpoint to bluez. It is not
> > specific to pulseaudio. And in my opinion central bluetooth daemon which
> > expose such functionality should be robust and be prepared that
> > application on "other side of dbus IPC" does not have to be well
> > behaved.
> >
> > > if we choose to encode the D-Bus connection, etc, as soon as
> > > PA is restarted, the system is rebooted, etc, the D-Bus connection may
> > > have changed making the stored values in LastUsed invalid.
> >
> > That is truth.
> >
> > My idea was to encode just dbus path of local dbus endpoint. We can say
> > (in IPC API) that client application should preserve dbus path for one
> > same endpoint between dbus daemon / computer restarts. Because otherwise
> > functionality of "choose last used endpoint" would not work.
> >
> > This is less strict requirement and current one (that registration order
> > of all existing applications in system must be same across reboots),
> > less error prone and still should be easily implemented.
>
> So what about this my idea? Is there anything wrong with such thing?

That makes the assumption that the path doesn't ever change which is
something we never required for the endpoints, besides can be clashes
if the same path is used in which case the stored value would be
ambiguous, so I prefer to just have the endpoint id, if we are so
eager to detect endpoint changes we can save some hash/unique id
generated but I think it is a bit overkill if this is working as it
currently is and we can always fallback if we cannot recover the last
used, anyway this is not even supported by the spec since it has no
guarantees regarding peers endpoints uniqueness.

> > > Anyway
> > > chances are the LastUsed is only invalidated if you update PA, in
> >
> > Or once we include support for dynamic codec loading (based on encoder
> > library presence in system), installation of any irrelevant application
> > may bring a new supported codec and therefore a new endpoint. So codecs
> > can become in any order...
> >
> > Or another example, when different application (not PA) register also
> > some endpoint.
> >
> > > which case there could be new endpoints or a change in their order and
> > > package can also provide a script to clear the LastUsed if that
> > > happens, but then again LastUsed setting does not actually store the
> > > configuration just the endpoint so SelectConfiguration has control
> > > over the configuration.
> >
> > Yes, whole thing is only about initial codec selection. In the worst
> > case user would have pre-selected different codec as which was last
> > used.
> >
> > But I think that storing dbus path of endpoint as described above should
> > be more reliable solution.
> >
> > Applications (e.g. PA) talking to bluetooth daemon does not register or
> > request directly SEID. Instead they register their dbus path and dbus
> > connection and bluetooth daemon later allocates SEID for that path.
> >
>
> --
> Pali Rohár
> pali.rohar@gmail.com



-- 
Luiz Augusto von Dentz

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Pali Rohár]

On Friday 07 June 2019 18:18:04 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Fri, Jun 7, 2019 at 4:04 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > On Tuesday 07 May 2019 20:57:40 Pali Rohár wrote:
> > > On Tuesday 07 May 2019 13:13:17 Luiz Augusto von Dentz wrote:
> > > > Hi Pali,
> > > >
> > > > On Tue, May 7, 2019 at 11:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > >
> > > > > On Monday 06 May 2019 15:02:25 Pali Rohár wrote:
> > > > > > On Monday 06 May 2019 15:46:03 Luiz Augusto von Dentz wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > I hope this fixes the problems you have been seeing, it should at
> > > > > > > least take care of the issues with wrong order of SelectConfiguration
> > > > > > > and restoring the exact same endpoint used last time.
> > > > > >
> > > > > > Hi Luiz! Great, this should make bluez to be more deterministic.
> > > > >
> > > > > Hi! Now I looked at it and in cache file I see:
> > > > >
> > > > > LastUsed=04:01
> > > > >
> > > > > What would happen when A2DP application (e.g. pulseaudio) register DBus
> > > > > endpoints in different order? Or e.g. do not register some endpoints due
> > > > > to missing codec librayr (aptX).
> > > > >
> > > > > I guess that in this case LastUsed information stops working...
> > > >
> > > > It would most likely fail at SelectConfiguration and then try with the
> > > > other endpoints.
> > >
> > > Yes, that is truth. My point is just about validity of LastUsed value.
> > >
> > > > >
> > > > > Should not be there stored rather dbus endpoint name identifier?
> > > >
> > > > Originally I tried to avoid having the local endpoints because of this
> > > > problem, but now that SelectConfiguration can fail it shouldn't really
> > > > be a problem. At least with the seid if you have a system that didn't
> > > > changed the order or number of endpoints it will keep working as
> > > > expected,
> > >
> > > But this may happen. And such thing is allowed. Any application,
> > > including unprivileged can register own new endpoint to bluez. It is not
> > > specific to pulseaudio. And in my opinion central bluetooth daemon which
> > > expose such functionality should be robust and be prepared that
> > > application on "other side of dbus IPC" does not have to be well
> > > behaved.
> > >
> > > > if we choose to encode the D-Bus connection, etc, as soon as
> > > > PA is restarted, the system is rebooted, etc, the D-Bus connection may
> > > > have changed making the stored values in LastUsed invalid.
> > >
> > > That is truth.
> > >
> > > My idea was to encode just dbus path of local dbus endpoint. We can say
> > > (in IPC API) that client application should preserve dbus path for one
> > > same endpoint between dbus daemon / computer restarts. Because otherwise
> > > functionality of "choose last used endpoint" would not work.
> > >
> > > This is less strict requirement and current one (that registration order
> > > of all existing applications in system must be same across reboots),
> > > less error prone and still should be easily implemented.
> >
> > So what about this my idea? Is there anything wrong with such thing?
> 
> That makes the assumption that the path doesn't ever change which is
> something we never required for the endpoints, besides can be clashes
> if the same path is used in which case the stored value would be
> ambiguous, so I prefer to just have the endpoint id, if we are so
> eager to detect endpoint changes we can save some hash/unique id
> generated but I think it is a bit overkill if this is working as it
> currently is and we can always fallback if we cannot recover the last
> used, anyway this is not even supported by the spec since it has no
> guarantees regarding peers endpoints uniqueness.

My main objection against endpoint id is that it is less stable then
application dbus path. Endpoint id depends on ordering which is defined
by all application which registers to dbus. And so it does not depend on
application itself. On the other hand endpoint dbus path is fully under
application control, so independently of other applications, dbus path
can be stable -- if application make it stable.

> > > > Anyway
> > > > chances are the LastUsed is only invalidated if you update PA, in
> > >
> > > Or once we include support for dynamic codec loading (based on encoder
> > > library presence in system), installation of any irrelevant application
> > > may bring a new supported codec and therefore a new endpoint. So codecs
> > > can become in any order...
> > >
> > > Or another example, when different application (not PA) register also
> > > some endpoint.
> > >
> > > > which case there could be new endpoints or a change in their order and
> > > > package can also provide a script to clear the LastUsed if that
> > > > happens, but then again LastUsed setting does not actually store the
> > > > configuration just the endpoint so SelectConfiguration has control
> > > > over the configuration.
> > >
> > > Yes, whole thing is only about initial codec selection. In the worst
> > > case user would have pre-selected different codec as which was last
> > > used.
> > >
> > > But I think that storing dbus path of endpoint as described above should
> > > be more reliable solution.
> > >
> > > Applications (e.g. PA) talking to bluetooth daemon does not register or
> > > request directly SEID. Instead they register their dbus path and dbus
> > > connection and bluetooth daemon later allocates SEID for that path.
> > >
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v3 6/6] a2dp: Reword LastUsed

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Jun 10, 2019 at 3:35 PM Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Friday 07 June 2019 18:18:04 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> >
> > On Fri, Jun 7, 2019 at 4:04 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > On Tuesday 07 May 2019 20:57:40 Pali Rohár wrote:
> > > > On Tuesday 07 May 2019 13:13:17 Luiz Augusto von Dentz wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Tue, May 7, 2019 at 11:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > >
> > > > > > On Monday 06 May 2019 15:02:25 Pali Rohár wrote:
> > > > > > > On Monday 06 May 2019 15:46:03 Luiz Augusto von Dentz wrote:
> > > > > > > > Hi Pali,
> > > > > > > >
> > > > > > > > I hope this fixes the problems you have been seeing, it should at
> > > > > > > > least take care of the issues with wrong order of SelectConfiguration
> > > > > > > > and restoring the exact same endpoint used last time.
> > > > > > >
> > > > > > > Hi Luiz! Great, this should make bluez to be more deterministic.
> > > > > >
> > > > > > Hi! Now I looked at it and in cache file I see:
> > > > > >
> > > > > > LastUsed=04:01
> > > > > >
> > > > > > What would happen when A2DP application (e.g. pulseaudio) register DBus
> > > > > > endpoints in different order? Or e.g. do not register some endpoints due
> > > > > > to missing codec librayr (aptX).
> > > > > >
> > > > > > I guess that in this case LastUsed information stops working...
> > > > >
> > > > > It would most likely fail at SelectConfiguration and then try with the
> > > > > other endpoints.
> > > >
> > > > Yes, that is truth. My point is just about validity of LastUsed value.
> > > >
> > > > > >
> > > > > > Should not be there stored rather dbus endpoint name identifier?
> > > > >
> > > > > Originally I tried to avoid having the local endpoints because of this
> > > > > problem, but now that SelectConfiguration can fail it shouldn't really
> > > > > be a problem. At least with the seid if you have a system that didn't
> > > > > changed the order or number of endpoints it will keep working as
> > > > > expected,
> > > >
> > > > But this may happen. And such thing is allowed. Any application,
> > > > including unprivileged can register own new endpoint to bluez. It is not
> > > > specific to pulseaudio. And in my opinion central bluetooth daemon which
> > > > expose such functionality should be robust and be prepared that
> > > > application on "other side of dbus IPC" does not have to be well
> > > > behaved.
> > > >
> > > > > if we choose to encode the D-Bus connection, etc, as soon as
> > > > > PA is restarted, the system is rebooted, etc, the D-Bus connection may
> > > > > have changed making the stored values in LastUsed invalid.
> > > >
> > > > That is truth.
> > > >
> > > > My idea was to encode just dbus path of local dbus endpoint. We can say
> > > > (in IPC API) that client application should preserve dbus path for one
> > > > same endpoint between dbus daemon / computer restarts. Because otherwise
> > > > functionality of "choose last used endpoint" would not work.
> > > >
> > > > This is less strict requirement and current one (that registration order
> > > > of all existing applications in system must be same across reboots),
> > > > less error prone and still should be easily implemented.
> > >
> > > So what about this my idea? Is there anything wrong with such thing?
> >
> > That makes the assumption that the path doesn't ever change which is
> > something we never required for the endpoints, besides can be clashes
> > if the same path is used in which case the stored value would be
> > ambiguous, so I prefer to just have the endpoint id, if we are so
> > eager to detect endpoint changes we can save some hash/unique id
> > generated but I think it is a bit overkill if this is working as it
> > currently is and we can always fallback if we cannot recover the last
> > used, anyway this is not even supported by the spec since it has no
> > guarantees regarding peers endpoints uniqueness.
>
> My main objection against endpoint id is that it is less stable then
> application dbus path. Endpoint id depends on ordering which is defined
> by all application which registers to dbus. And so it does not depend on
> application itself. On the other hand endpoint dbus path is fully under
> application control, so independently of other applications, dbus path
> can be stable -- if application make it stable.

Yes, it may be more stable but it is also not unique so in the event 2
endpoints happen to have the same path we would have to disambiguate
them somehow and because of that I don't think we should be using
that. Im not sure why this would be a big deal since we should call
SelectConfiguration it is not like we would end up in a situation
where the stream would fail to setup since if that fails we fallback
to the priority ordering, if the user felt something out of order it
was either the system has changed (PA update, etc) or the device has
changed (new firmware, etc).

> > > > > Anyway
> > > > > chances are the LastUsed is only invalidated if you update PA, in
> > > >
> > > > Or once we include support for dynamic codec loading (based on encoder
> > > > library presence in system), installation of any irrelevant application
> > > > may bring a new supported codec and therefore a new endpoint. So codecs
> > > > can become in any order...
> > > >
> > > > Or another example, when different application (not PA) register also
> > > > some endpoint.
> > > >
> > > > > which case there could be new endpoints or a change in their order and
> > > > > package can also provide a script to clear the LastUsed if that
> > > > > happens, but then again LastUsed setting does not actually store the
> > > > > configuration just the endpoint so SelectConfiguration has control
> > > > > over the configuration.
> > > >
> > > > Yes, whole thing is only about initial codec selection. In the worst
> > > > case user would have pre-selected different codec as which was last
> > > > used.
> > > >
> > > > But I think that storing dbus path of endpoint as described above should
> > > > be more reliable solution.
> > > >
> > > > Applications (e.g. PA) talking to bluetooth daemon does not register or
> > > > request directly SEID. Instead they register their dbus path and dbus
> > > > connection and bluetooth daemon later allocates SEID for that path.
> > > >
> > >
> > > --
> > > Pali Rohár
> > > pali.rohar@gmail.com
>
> --
> Pali Rohár
> pali.rohar@gmail.com



-- 
Luiz Augusto von Dentz

Re: [PATCH v3 1/6] a2dp: Fix crash when endpoint respond with an error

[Luiz Augusto von Dentz]

Hi,

On Mon, May 6, 2019 at 3:43 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> If endpoint respond with an error the callback will be called with size
> set to -1 not 0.
> ---
>  profiles/audio/a2dp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index d0676b577..74ada3bbe 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -2418,7 +2418,7 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
>         struct avdtp_media_codec_capability *codec;
>         int err;
>
> -       if (size) {
> +       if (size >= 0) {
>                 caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
>                 goto done;
>         }
> --
> 2.20.1

Applied.

-- 
Luiz Augusto von Dentz