2022-08-29 20:36:54

by Frédéric Danis

[permalink] [raw]
Subject: [PATCH BlueZ 0/2] profiles: Add remote endpoint path to SelectProperties

The SelectProperties method is only called on the central (initiator) device.
But there's no information related to the remote device for which is call is
done.
These commits allow the audio server to link this call methos to the
appropriate remote endpoint.

Frédéric Danis (2):
profiles: Add remote endpoint path to SelectProperties
doc: Add remote endpoint path to SelectProperties

doc/media-api.txt | 2 +-
profiles/audio/bap.c | 2 +-
profiles/audio/media.c | 3 +++
src/shared/bap.c | 2 ++
src/shared/bap.h | 2 ++
5 files changed, 9 insertions(+), 2 deletions(-)

--
2.25.1


2022-08-29 20:36:54

by Frédéric Danis

[permalink] [raw]
Subject: [PATCH BlueZ 2/2] doc: Add remote endpoint path to SelectProperties

---
doc/media-api.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/media-api.txt b/doc/media-api.txt
index 9cd211355..1866ecfcb 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -598,7 +598,7 @@ Methods void SetConfiguration(object transport, dict properties)
configuration since on success the configuration is
send back as parameter of SetConfiguration.

- dict SelectProperties(dict properties)
+ dict SelectProperties(object remote_endpoint, dict properties)

Select preferable properties from the supported
properties. Refer to SetConfiguration for the list of
--
2.25.1

2022-08-29 20:36:54

by Frédéric Danis

[permalink] [raw]
Subject: [PATCH BlueZ 1/2] profiles: Add remote endpoint path to SelectProperties

---
profiles/audio/bap.c | 2 +-
profiles/audio/media.c | 3 +++
src/shared/bap.c | 2 ++
src/shared/bap.h | 2 ++
4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index d388afe56..cf27ec0ae 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -613,7 +613,7 @@ static bool pac_found(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,

/* TODO: Cache LRU? */
if (btd_service_is_initiator(service))
- bt_bap_select(lpac, rpac, select_cb, ep);
+ bt_bap_select(lpac, rpac, ep->path, select_cb, ep);

return true;
}
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index ff3fa197b..8d777eedd 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -891,6 +891,7 @@ done:

static int pac_select(struct bt_bap_pac *pac, struct bt_bap_pac_qos *qos,
struct iovec *caps, struct iovec *metadata,
+ const char *remote_ep_path,
bt_bap_pac_select_t cb, void *cb_data, void *user_data)
{
struct media_endpoint *endpoint = user_data;
@@ -917,6 +918,8 @@ static int pac_select(struct bt_bap_pac *pac, struct bt_bap_pac_qos *qos,

dbus_message_iter_init_append(msg, &iter);

+ dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH, &remote_ep_path);
+
dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, "{sv}", &dict);

g_dbus_dict_append_basic_array(&dict, DBUS_TYPE_STRING, &key,
diff --git a/src/shared/bap.c b/src/shared/bap.c
index 8edc7b72e..691fec2fa 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -4058,6 +4058,7 @@ static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
}

int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
+ const char *remote_ep_path,
bt_bap_pac_select_t func, void *user_data)
{
if (!lpac || !rpac || !func)
@@ -4067,6 +4068,7 @@ int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
return -EOPNOTSUPP;

lpac->ops->select(lpac, &rpac->qos, rpac->data, rpac->metadata,
+ remote_ep_path,
func, user_data, lpac->user_data);

return 0;
diff --git a/src/shared/bap.h b/src/shared/bap.h
index ff4bac330..da5fe1431 100644
--- a/src/shared/bap.h
+++ b/src/shared/bap.h
@@ -122,6 +122,7 @@ struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
struct bt_bap_pac_ops {
int (*select) (struct bt_bap_pac *pac, struct bt_bap_pac_qos *qos,
struct iovec *caps, struct iovec *metadata,
+ const char *remote_ep_path,
bt_bap_pac_select_t cb, void *cb_data, void *user_data);
int (*config) (struct bt_bap_stream *stream, struct iovec *cfg,
struct bt_bap_qos *qos, bt_bap_pac_config_t cb,
@@ -188,6 +189,7 @@ int bt_bap_pac_get_codec(struct bt_bap_pac *pac, uint8_t *id,

/* Stream related functions */
int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
+ const char *remote_ep_path,
bt_bap_pac_select_t func, void *user_data);

struct bt_bap_stream *bt_bap_config(struct bt_bap *bap,
--
2.25.1

2022-08-29 21:45:40

by bluez.test.bot

[permalink] [raw]
Subject: RE: profiles: Add remote endpoint path to SelectProperties

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=672169

---Test result---

Test Summary:
CheckPatch FAIL 2.95 seconds
GitLint PASS 2.01 seconds
Prep - Setup ELL PASS 27.91 seconds
Build - Prep PASS 0.84 seconds
Build - Configure PASS 8.96 seconds
Build - Make PASS 1039.25 seconds
Make Check PASS 11.03 seconds
Make Check w/Valgrind PASS 276.22 seconds
Make Distcheck PASS 238.53 seconds
Build w/ext ELL - Configure PASS 8.61 seconds
Build w/ext ELL - Make PASS 82.44 seconds
Incremental Build w/ patches PASS 201.30 seconds
Scan Build WARNING 580.56 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[BlueZ,1/2] profiles: Add remote endpoint path to SelectProperties
WARNING:LONG_LINE: line length of 86 exceeds 80 columns
#99: FILE: profiles/audio/media.c:921:
+ dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH, &remote_ep_path);

/github/workspace/src/12958401.patch total: 0 errors, 1 warnings, 51 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12958401.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: Scan Build - WARNING
Desc: Run Scan Build with patches
Output:
*****************************************************************************
The bugs reported by the scan-build may or may not be caused by your patches.
Please check the list and fix the bugs if they are caused by your patch.
*****************************************************************************
profiles/audio/media.c:1453:6: warning: 8th function call argument is an uninitialized value
if (media_endpoint_create(adapter, sender, path, uuid, delay_reporting,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
profiles/audio/media.c:2999:3: warning: Use of memory after it is freed
release_endpoint(adapter->endpoints->data);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
profiles/audio/media.c:3002:3: warning: Use of memory after it is freed
media_player_destroy(adapter->players->data);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.




---
Regards,
Linux Bluetooth

2022-08-29 22:07:07

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 1/2] profiles: Add remote endpoint path to SelectProperties

Hi Frédéric,

On Mon, Aug 29, 2022 at 1:36 PM Frédéric Danis
<[email protected]> wrote:
>
> ---
> profiles/audio/bap.c | 2 +-
> profiles/audio/media.c | 3 +++
> src/shared/bap.c | 2 ++
> src/shared/bap.h | 2 ++
> 4 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index d388afe56..cf27ec0ae 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -613,7 +613,7 @@ static bool pac_found(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>
> /* TODO: Cache LRU? */
> if (btd_service_is_initiator(service))
> - bt_bap_select(lpac, rpac, select_cb, ep);
> + bt_bap_select(lpac, rpac, ep->path, select_cb, ep);
>
> return true;
> }
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index ff3fa197b..8d777eedd 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -891,6 +891,7 @@ done:
>
> static int pac_select(struct bt_bap_pac *pac, struct bt_bap_pac_qos *qos,
> struct iovec *caps, struct iovec *metadata,
> + const char *remote_ep_path,
> bt_bap_pac_select_t cb, void *cb_data, void *user_data)
> {
> struct media_endpoint *endpoint = user_data;
> @@ -917,6 +918,8 @@ static int pac_select(struct bt_bap_pac *pac, struct bt_bap_pac_qos *qos,
>
> dbus_message_iter_init_append(msg, &iter);
>
> + dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH, &remote_ep_path);
> +

If the endpoint object is not part of the dictionary then you will
have to fix client/player.c as well since the signature will be
changing, so I think it is better we include it as part of properties
itself.

> dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, "{sv}", &dict);
>
> g_dbus_dict_append_basic_array(&dict, DBUS_TYPE_STRING, &key,
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 8edc7b72e..691fec2fa 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -4058,6 +4058,7 @@ static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> }
>
> int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> + const char *remote_ep_path,
> bt_bap_pac_select_t func, void *user_data)
> {
> if (!lpac || !rpac || !func)
> @@ -4067,6 +4068,7 @@ int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> return -EOPNOTSUPP;
>
> lpac->ops->select(lpac, &rpac->qos, rpac->data, rpac->metadata,
> + remote_ep_path,
> func, user_data, lpac->user_data);
>
> return 0;
> diff --git a/src/shared/bap.h b/src/shared/bap.h
> index ff4bac330..da5fe1431 100644
> --- a/src/shared/bap.h
> +++ b/src/shared/bap.h
> @@ -122,6 +122,7 @@ struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
> struct bt_bap_pac_ops {
> int (*select) (struct bt_bap_pac *pac, struct bt_bap_pac_qos *qos,
> struct iovec *caps, struct iovec *metadata,
> + const char *remote_ep_path,

Hmm, Id avoid passing D-Bus specific path here since this API is
suppose to be generic and at some point we might want to create a unit
tests that don't involve the daemon, so instead I think it is better
that we pass the rpac and addition to the lpac and then perhaps have a
function which can attach custom user_data to the rpac e.g.: void
bt_bap_pac_set_user_data(void *); void *bt_bap_pac_get_user_data();

> bt_bap_pac_select_t cb, void *cb_data, void *user_data);
> int (*config) (struct bt_bap_stream *stream, struct iovec *cfg,
> struct bt_bap_qos *qos, bt_bap_pac_config_t cb,
> @@ -188,6 +189,7 @@ int bt_bap_pac_get_codec(struct bt_bap_pac *pac, uint8_t *id,
>
> /* Stream related functions */
> int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> + const char *remote_ep_path,
> bt_bap_pac_select_t func, void *user_data);
>
> struct bt_bap_stream *bt_bap_config(struct bt_bap *bap,
> --
> 2.25.1
>


--
Luiz Augusto von Dentz