[PATCH BlueZ 1/2] shared/mcp: emit MCS error if value changes during long read

[PATCH BlueZ 1/2] shared/mcp: emit MCS error if value changes during long read

[Pauli Virtanen]

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.
---
 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);
-- 
2.51.1

[PATCH BlueZ 2/2] test-mcp: add tests for long value reading

[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

RE: [BlueZ,1/2] shared/mcp: emit MCS error if value changes during long read

[bluez.test.bot]

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

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=1033906

---Test result---

Test Summary:
CheckPatch                    PENDING   0.35 seconds
GitLint                       PENDING   0.29 seconds
BuildEll                      PASS      19.79 seconds
BluezMake                     PASS      644.38 seconds
MakeCheck                     PASS      22.21 seconds
MakeDistcheck                 PASS      239.01 seconds
CheckValgrind                 PASS      297.79 seconds
CheckSmatch                   PASS      346.90 seconds
bluezmakeextell               PASS      179.64 seconds
IncrementalBuild              PENDING   0.37 seconds
ScanBuild                     PASS      1006.66 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth