* [PATCH BlueZ 2/2] test-mcp: add tests for long value reading
2025-12-16 19:46 [PATCH BlueZ 1/2] shared/mcp: emit MCS error if value changes during long read Pauli Virtanen
@ 2025-12-16 19:46 ` Pauli Virtanen
2025-12-16 20:40 ` [BlueZ,1/2] shared/mcp: emit MCS error if value changes during long read bluez.test.bot
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Pauli Virtanen @ 2025-12-16 19:46 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
Add server tests for handling the Value Changed During Read Long error.
Add similar client tests.
---
src/shared/mcp.c | 5 ++
src/shared/mcp.h | 1 +
unit/test-mcp.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 189 insertions(+)
diff --git a/src/shared/mcp.c b/src/shared/mcp.c
index 9f8952af1..a25d7b5a2 100644
--- a/src/shared/mcp.c
+++ b/src/shared/mcp.c
@@ -2118,3 +2118,8 @@ bool bt_mcp_add_listener(struct bt_mcp *mcp, uint8_t ccid,
queue_push_tail(service->listeners, listener);
return true;
}
+
+struct bt_gatt_client *bt_mcp_test_util_get_client(struct bt_mcp *mcp)
+{
+ return mcp->client;
+}
diff --git a/src/shared/mcp.h b/src/shared/mcp.h
index 937afb6d3..8179c0098 100644
--- a/src/shared/mcp.h
+++ b/src/shared/mcp.h
@@ -167,3 +167,4 @@ void bt_mcs_unregister_all(struct gatt_db *db);
/* For tests: */
void bt_mcs_test_util_reset_ccid(void);
+struct bt_gatt_client *bt_mcp_test_util_get_client(struct bt_mcp *mcp);
diff --git a/unit/test-mcp.c b/unit/test-mcp.c
index b05630185..7d922bb83 100644
--- a/unit/test-mcp.c
+++ b/unit/test-mcp.c
@@ -586,6 +586,31 @@ static void test_client(const void *user_data)
IOV_DATA(0x0a, HND(hnd)), \
IOV_DATA(0x0b, value)
+/* ATT: Read Blob Request (0x0c)
+ * ATT: Read Blob Response (0x0d)
+ */
+
+#define READ_BLOB_CHRC(hnd, off, value...) \
+ IOV_DATA(0x0c, HND(hnd), (off) & 0xff, (off) >> 8), \
+ IOV_DATA(0x0d, value)
+
+#define READ_BLOB_ERR_CHRC(hnd, off, err) \
+ IOV_DATA(0x0c, HND(hnd), (off) & 0xff, (off) >> 8), \
+ IOV_DATA(0x01, 0x0c, HND(hnd), err)
+
+#define BLOB_DATA_MTU_1(v) \
+ v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, \
+ v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, \
+ v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, \
+ v, v, v, v, v, v, v, v, v, v, v, v, v, v, v
+
+#define BLOB_DATA_MTU_3(v) \
+ v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, \
+ v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, \
+ v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, v, \
+ v, v, v, v, v, v, v, v, v, v, v, v, v
+
+
/* ATT: Write Command (0x52) len 2
*/
@@ -1369,11 +1394,100 @@ const struct test_config cfg_cl_bluez_1_reread = {
.gmcs = false,
};
+#define CL_BLUEZ_2_REREAD \
+ NOTIFY_CHRC(TRACK_CHG), \
+ READ_CHRC(TRACK_TITLE, BLOB_DATA_MTU_1('y')), \
+ /* Value change during long read */ \
+ READ_BLOB_ERR_CHRC(TRACK_TITLE, 0x40 - 1, 0x80), \
+ READ_CHRC(TRACK_DUR, 0xff, 0xff, 0xff, 0xff), \
+ READ_CHRC(TRACK_POS, 0xff, 0xff, 0xff, 0xff), \
+ READ_CHRC(PLAY_SPEED, 0x00), \
+ READ_CHRC(SEEK_SPEED, 0x00), \
+ READ_CHRC(PLAY_ORDER, 0x04), \
+ READ_CHRC(PLAY_ORDER_SUPP, 0x18, 0x00), \
+ READ_CHRC(CP_SUPP, SPLIT_INT32(0x01)), \
+ /* Track change notification -> reread */ \
+ NOTIFY_CHRC(TRACK_CHG), \
+ READ_CHRC(TRACK_TITLE, BLOB_DATA_MTU_1('x')), \
+ READ_BLOB_CHRC(TRACK_TITLE, 0x40 - 1, 'x', 'x', 'x'), \
+ READ_CHRC(TRACK_DUR, 0xff, 0xff, 0xff, 0xff), \
+ READ_CHRC(TRACK_POS, 0xff, 0xff, 0xff, 0xff), \
+ READ_CHRC(PLAY_SPEED, 0x00), \
+ READ_CHRC(SEEK_SPEED, 0x00), \
+ READ_CHRC(PLAY_ORDER, 0x04), \
+ READ_CHRC(PLAY_ORDER_SUPP, 0x18, 0x00), \
+ READ_CHRC(CP_SUPP, SPLIT_INT32(0x01)), \
+ IOV_NULL
+
+#define CL_BLUEZ_3_REREAD \
+ NOTIFY_CHRC(TRACK_TITLE, BLOB_DATA_MTU_3('y')), \
+ READ_CHRC(TRACK_TITLE, BLOB_DATA_MTU_1('y')), \
+ /* Changed during read -> notification -> reread */ \
+ READ_BLOB_ERR_CHRC(TRACK_TITLE, 0x40 - 1, 0x80), \
+ NOTIFY_CHRC(TRACK_TITLE, BLOB_DATA_MTU_3('x')), \
+ READ_CHRC(TRACK_TITLE, BLOB_DATA_MTU_1('x')), \
+ READ_BLOB_CHRC(TRACK_TITLE, 0x40 - 1, 'x', 'x', 'x'), \
+ IOV_NULL
+
+static void cl_reread_long_idle(void *user_data)
+{
+ struct test_data *data = user_data;
+
+ if (data->step == 2)
+ tester_test_passed();
+}
+
+static void cl_reread_long_track_title(void *user_data, const uint8_t *value,
+ uint16_t length)
+{
+ struct test_data *data = user_data;
+ const char new_value[0x42] = { [0 ... 0x41] = 'x' };
+
+ if (strncmp((void *)value, "Title", length) == 0 && data->step == 0) {
+ data->step++;
+ } else if (data->step == 1 && length == 0x42 &&
+ !memcmp((void *)value, new_value, length)) {
+ data->step++;
+
+ /* Wait for all reads, to make sure there are no extra */
+ bt_gatt_client_idle_register(
+ bt_mcp_test_util_get_client(data->mcp),
+ cl_reread_long_idle, data, NULL);
+ } else {
+ FAIL_TEST();
+ }
+}
+
+const struct test_config cfg_cl_bluez_2_reread = {
+ .listener_cb = &(struct bt_mcp_listener_callback) {
+ .track_title = cl_reread_long_track_title,
+ },
+ .setup_data = setup_data_mcs,
+ .setup_data_len = ARRAY_SIZE(setup_data_mcs),
+ .gmcs = false,
+};
+
+const struct test_config cfg_cl_bluez_3_reread = {
+ .listener_cb = &(struct bt_mcp_listener_callback) {
+ .track_title = cl_reread_long_track_title,
+ },
+ .setup_data = setup_data_gmcs,
+ .setup_data_len = ARRAY_SIZE(setup_data_gmcs),
+ .gmcs = true,
+};
+
+
static void testgroup_cl_extra(void)
{
define_test("MCP/CL/BLUEZ-1 [Reread On Track Change, No Notify]",
test_setup, test_client,
&cfg_cl_bluez_1_reread, CL_BLUEZ_1_REREAD);
+ define_test("MCP/CL/BLUEZ-2 [Long Value Reread, Track Changed]",
+ test_setup, test_client,
+ &cfg_cl_bluez_2_reread, CL_BLUEZ_2_REREAD);
+ define_test("MCP/CL/BLUEZ-3 [Long Value Reread, Notify]",
+ test_setup, test_client,
+ &cfg_cl_bluez_3_reread, CL_BLUEZ_3_REREAD);
}
/*
@@ -1846,6 +1960,74 @@ static void testgroup_sr_mcp(void)
*/
}
+
+static void sr_spn_value(struct test_data *data, struct iovec *buf, size_t size,
+ uint16_t uuid)
+{
+ /* Longer than MTU */
+ memset(buf->iov_base, 'x' + data->step, 0x42);
+ buf->iov_len = 0x42;
+
+ /* Long value changed during read */
+ if (data->step == 1) {
+ data->step--;
+ bt_mcs_changed(data->mcs, uuid);
+ }
+}
+
+static void sr_spn_media_player_name(void *data, struct iovec *buf, size_t size)
+{
+ sr_spn_value(data, buf, size, MCS_MEDIA_PLAYER_NAME_CHRC_UUID);
+}
+
+static void sr_spn_track_title(void *data, struct iovec *buf, size_t size)
+{
+ sr_spn_value(data, buf, size, MCS_TRACK_TITLE_CHRC_UUID);
+}
+
+const struct test_config cfg_mcs_sr_spn = {
+ .mcs_cb = &(struct bt_mcs_callback) {
+ .media_player_name = sr_spn_media_player_name,
+ .track_title = sr_spn_track_title,
+ .debug = mcs_debug,
+ },
+ .setup_data = setup_data_server_mcs,
+ .setup_data_len = ARRAY_SIZE(setup_data_server_mcs),
+ .gmcs = false,
+};
+
+#define MCS_SR_SPN_PROCEDURE(chrc) \
+ READ_CHRC(chrc, BLOB_DATA_MTU_1('y')), \
+ NOTIFY_CHRC(chrc, BLOB_DATA_MTU_3('x')), \
+ READ_BLOB_ERR_CHRC(chrc, 0x40 - 1, 0x80), \
+ READ_CHRC(chrc, BLOB_DATA_MTU_1('x')), \
+ READ_BLOB_CHRC(chrc, 0x40 - 1, 'x', 'x', 'x')
+
+#define MCS_SR_SPN_BV_01_C MCS_SR_SPN_PROCEDURE(NAME)
+#define MCS_SR_SPN_BV_02_C MCS_SR_SPN_PROCEDURE(TRACK_TITLE)
+
+static void test_sr_spn(const void *user_data)
+{
+ struct test_data *data = (void *)user_data;
+
+ data->step = 1;
+ test_server(data);
+}
+
+static void testgroup_sr_spn(void)
+{
+ /* Only the MCS tests. No point in GMCS as only svc uuid changes */
+
+ /* MCS.TS Sec 4.5 Update Characteristic - Oversized values */
+ define_test("MCS/SR/SPN/BV-01-C [Update Media Player Name - Oversized "
+ "Value]",
+ test_setup_server, test_sr_spn,
+ &cfg_mcs_sr_spn, MCS_SR_SPN_BV_01_C);
+ define_test("MCS/SR/SPN/BV-02-C [Update Track Title - Oversized Value]",
+ test_setup_server, test_sr_spn,
+ &cfg_mcs_sr_spn, MCS_SR_SPN_BV_02_C);
+}
+
int main(int argc, char *argv[])
{
tester_init(&argc, &argv);
@@ -1854,6 +2036,7 @@ int main(int argc, char *argv[])
testgroup_cl_extra();
testgroup_sr_sggit();
testgroup_sr_mcp();
+ testgroup_sr_spn();
return tester_run();
}
--
2.51.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH BlueZ 1/2] shared/mcp: emit MCS error if value changes during long read
2025-12-16 19:46 [PATCH BlueZ 1/2] shared/mcp: emit MCS error if value changes during long read Pauli Virtanen
2025-12-16 19:46 ` [PATCH BlueZ 2/2] test-mcp: add tests for long value reading Pauli Virtanen
2025-12-16 20:40 ` [BlueZ,1/2] shared/mcp: emit MCS error if value changes during long read bluez.test.bot
@ 2026-01-13 15:41 ` Pauli Virtanen
2026-01-13 22:30 ` patchwork-bot+bluetooth
3 siblings, 0 replies; 5+ messages in thread
From: Pauli Virtanen @ 2026-01-13 15:41 UTC (permalink / raw)
To: linux-bluetooth
Hi,
ti, 2025-12-16 kello 21:46 +0200, Pauli Virtanen kirjoitti:
> MCS spec requires emitting Value Changed During Read Long if value
> changes between remote reading with zero offset and nonzero offset.
>
> This is session-specific state, so add support for that.
>
> As server, track value changes and emit that error properly.
>
> As client, we don't need to reread if this error occurs, as there should
> be a notification or track changed that queues a new read.
Friendly ping on this series, while it's still in Patchwork.
> ---
> src/shared/mcp.c | 109 +++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 97 insertions(+), 12 deletions(-)
>
> diff --git a/src/shared/mcp.c b/src/shared/mcp.c
> index 910089f18..9f8952af1 100644
> --- a/src/shared/mcp.c
> +++ b/src/shared/mcp.c
> @@ -30,6 +30,8 @@
> #include "src/shared/mcp.h"
> #include "src/shared/mcs.h"
>
> +#define BT_MCS_ERROR_VALUE_CHANGED_DURING_READ_LONG 0x80
> +
> #define DBG_MCP(mcp, fmt, ...) \
> mcp_debug(mcp, "%s:%s() mcp %p | " fmt, __FILE__, __func__, mcp, \
> ##__VA_ARGS__)
> @@ -76,22 +78,19 @@ struct bt_mcs_db {
> struct gatt_db_attribute *ccid;
> };
>
> -struct bt_mcs_client {
> +struct bt_mcs_session {
> + struct bt_mcs *mcs;
> struct bt_att *att;
> + unsigned int disconn_id;
>
> - /* Per-client state.
> - *
> - * Concurrency is not specified in MCS v1.0.1, everything currently
> - * implemented seems OK to be in global state.
> - *
> - * TODO: Search Results ID likely should go here
> - */
> + /* Per-client state */
> + struct queue *changed;
> };
>
> struct bt_mcs {
> struct gatt_db *db;
> struct bt_mcs_db ldb;
> - struct queue *clients;
> + struct queue *sessions;
>
> uint8_t media_state;
>
> @@ -557,11 +556,86 @@ static bool set_playing_order(struct bt_mcs *mcs, void *data)
> return false;
> }
>
> +static bool match_session_att(const void *data, const void *match_data)
> +{
> + const struct bt_mcs_session *session = data;
> +
> + return session->att == match_data;
> +}
> +
> +static void session_destroy(void *data)
> +{
> + struct bt_mcs_session *session = data;
> +
> + bt_att_unregister_disconnect(session->att, session->disconn_id);
> + queue_destroy(session->changed, NULL);
> + free(session);
> +}
> +
> +static void session_disconnect(int err, void *user_data)
> +{
> + struct bt_mcs_session *session = user_data;
> + struct bt_mcs *mcs = session->mcs;
> +
> + queue_remove(mcs->sessions, session);
> + session_destroy(session);
> +}
> +
> +static struct bt_mcs_session *get_session(struct bt_mcs *mcs,
> + struct bt_att *att)
> +{
> + struct bt_mcs_session *session;
> +
> + session = queue_find(mcs->sessions, match_session_att, att);
> + if (session)
> + return session;
> +
> + session = new0(struct bt_mcs_session, 1);
> + session->disconn_id = bt_att_register_disconnect(att,
> + session_disconnect, session, NULL);
> + if (!session->disconn_id) {
> + free(session);
> + return NULL;
> + }
> +
> + session->mcs = mcs;
> + session->att = att;
> + session->changed = queue_new();
> +
> + queue_push_tail(mcs->sessions, session);
> + return session;
> +}
> +
> +static void session_changed(void *data, void *user_data)
> +{
> + struct bt_mcs_session *session = data;
> + struct gatt_db_attribute *attrib = user_data;
> +
> + if (!queue_find(session->changed, NULL, attrib))
> + queue_push_tail(session->changed, attrib);
> +}
> +
> static void read_result(struct bt_mcs *mcs, struct gatt_db_attribute *attrib,
> - unsigned int id, uint16_t offset, mcs_get_func_t get)
> + unsigned int id, uint16_t offset, struct bt_att *att,
> + mcs_get_func_t get)
> {
> uint8_t buf[BT_ATT_MAX_VALUE_LEN];
> struct iovec iov = { .iov_base = buf, .iov_len = 0 };
> + struct bt_mcs_session *session = get_session(mcs, att);
> +
> + if (!session) {
> + gatt_db_attribute_read_result(attrib, id,
> + BT_ATT_ERROR_UNLIKELY, NULL, 0);
> + return;
> + }
> +
> + if (!offset) {
> + queue_remove(session->changed, attrib);
> + } else if (queue_find(session->changed, NULL, attrib)) {
> + gatt_db_attribute_read_result(attrib, id,
> + BT_MCS_ERROR_VALUE_CHANGED_DURING_READ_LONG, NULL, 0);
> + return;
> + }
>
> get(mcs, &iov, sizeof(buf));
>
> @@ -582,7 +656,7 @@ static void read_result(struct bt_mcs *mcs, struct gatt_db_attribute *attrib,
> void *user_data) \
> { \
> DBG_MCS(user_data, ""); \
> - read_result(user_data, attrib, id, offset, get_ ##name); \
> + read_result(user_data, attrib, id, offset, att, get_ ##name); \
> }
>
> READ_FUNC(media_player_name)
> @@ -683,7 +757,9 @@ void bt_mcs_changed(struct bt_mcs *mcs, uint16_t chrc_uuid)
> if (bt_uuid_cmp(&uuid_attr, &uuid))
> continue;
>
> - DBG_MCS(mcs, "Notify %u", chrc_uuid);
> + queue_foreach(mcs->sessions, session_changed, attrs[i].attr);
> +
> + DBG_MCS(mcs, "Notify 0x%04x", chrc_uuid);
>
> attrs[i].get(mcs, &iov, sizeof(buf));
>
> @@ -925,6 +1001,7 @@ struct bt_mcs *bt_mcs_register(struct gatt_db *db, bool is_gmcs,
> mcs->user_data = user_data;
>
> mcs->media_state = BT_MCS_STATE_INACTIVE;
> + mcs->sessions = queue_new();
>
> if (!mcs_init_db(mcs, is_gmcs)) {
> free(mcs);
> @@ -959,6 +1036,8 @@ void bt_mcs_unregister(struct bt_mcs *mcs)
> servers = NULL;
> }
>
> + queue_destroy(mcs->sessions, session_destroy);
> +
> free(mcs);
> }
>
> @@ -1367,6 +1446,12 @@ static void update_media_player_name(bool success, uint8_t att_ecode,
> {
> struct bt_mcp_service *service = user_data;
>
> + if (!success) {
> + DBG_SVC(service, "Unable to read Media Player Name: "
> + "error 0x%02x", att_ecode);
> + return;
> + }
> +
> DBG_SVC(service, "Media Player Name");
>
> LISTENER_CB(service, media_player_name, value, length);
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 5+ messages in thread