[PATCH 1/3] mesh: Segmentation fails in gatt.c:pipe_write()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



From: Steve Brown <sbrown@xxxxxxxxxxxx>

If the first command output in a new connection exceeds 20 bytes,
mesh_gatt_write sets the SAR to FIRST as the write_mtu is initially 0
and the default is GATT_MTU-3 (20).

When pipe_write gets called, a new larger write_mtu has been set, but
the SAR is still set to FIRST. It's assumed that data->gatt_len >
max_len. However, it's not which causes lots of bogus output.

---

At Luiz' suggestion.

1. The WriteValue code has been removed
2. The SAR logic has been moved to pipe_write

It was tested against the zephyr mesh_shell and a small mtu to test the
segmentation logic.
---
 mesh/gatt.c | 141 ++++++++----------------------------------------------------
 1 file changed, 18 insertions(+), 123 deletions(-)

diff --git a/mesh/gatt.c b/mesh/gatt.c
index 197291e67..9116a9de1 100644
--- a/mesh/gatt.c
+++ b/mesh/gatt.c
@@ -102,27 +102,6 @@ static void write_data_free(void *user_data)
 	free(data);
 }
 
-static void write_setup(DBusMessageIter *iter, void *user_data)
-{
-	struct write_data *data = user_data;
-	struct iovec *iov = &data->iov;
-	DBusMessageIter array, dict;
-
-	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "y", &array);
-	dbus_message_iter_append_fixed_array(&array, DBUS_TYPE_BYTE,
-						&iov->iov_base, iov->iov_len);
-	dbus_message_iter_close_container(iter, &array);
-
-	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
-					DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
-					DBUS_TYPE_STRING_AS_STRING
-					DBUS_TYPE_VARIANT_AS_STRING
-					DBUS_DICT_ENTRY_END_CHAR_AS_STRING,
-					&dict);
-
-	dbus_message_iter_close_container(iter, &dict);
-}
-
 uint16_t mesh_gatt_sar(uint8_t **pkt, uint16_t size)
 {
 	const uint8_t *data = *pkt;
@@ -192,11 +171,12 @@ static bool pipe_write(struct io *io, void *user_data)
 	struct write_data *data = user_data;
 	struct iovec iov[2];
 	uint8_t sar;
-	uint8_t max_len = write_mtu - 4;
+	uint8_t max_len;
 
 	if (data == NULL)
 		return true;
 
+	max_len = write_mtu ? write_mtu - 3 - 1 : GATT_MTU - 3 - 1;
 	print_byte_array("GATT-TX:\t", data->gatt_data, data->gatt_len);
 
 	sar = data->gatt_data[0];
@@ -204,6 +184,14 @@ static bool pipe_write(struct io *io, void *user_data)
 	data->iov.iov_base = data->gatt_data + 1;
 	data->iov.iov_len--;
 
+	sar = data->gatt_data[0] & GATT_TYPE_MASK;
+	data->gatt_len--;
+
+	if (data->gatt_len > max_len) {
+		sar |= GATT_SAR_FIRST;
+		data->iov.iov_len = max_len;
+	}
+
 	iov[0].iov_base = &sar;
 	iov[0].iov_len = sizeof(sar);
 
@@ -245,73 +233,6 @@ static bool pipe_write(struct io *io, void *user_data)
 	}
 }
 
-static void write_reply(DBusMessage *message, void *user_data)
-{
-	struct write_data *data = user_data;
-	struct write_data *tmp;
-	uint8_t *dptr = data->gatt_data;
-	uint8_t max_len = GATT_MTU - 3;
-	uint8_t max_seg = GATT_MTU - 4;
-	DBusError error;
-
-	dbus_error_init(&error);
-
-	if (dbus_set_error_from_message(&error, message) == TRUE) {
-		bt_shell_printf("Failed to write: %s\n", error.name);
-		dbus_error_free(&error);
-		return;
-	}
-
-	if (data == NULL)
-		return;
-
-	switch (data->gatt_data[0] & GATT_SAR_MASK) {
-		case GATT_SAR_FIRST:
-		case GATT_SAR_CONTINUE:
-			tmp = g_new0(struct write_data, 1);
-			if (!data)
-				return;
-
-			*tmp = *data;
-			tmp->gatt_data = g_malloc(data->gatt_len);
-
-			if (!tmp->gatt_data) {
-				g_free(tmp);
-				return;
-			}
-
-			tmp->gatt_data[0] = dptr[0];
-			data = tmp;
-			memcpy(data->gatt_data + 1, dptr + max_len,
-					data->gatt_len - max_seg);
-			data->gatt_len -= max_seg;
-			data->gatt_data[0] &= GATT_TYPE_MASK;
-			data->iov.iov_base = data->gatt_data;
-			if (max_len < data->gatt_len) {
-				data->iov.iov_len = max_len;
-				data->gatt_data[0] |= GATT_SAR_CONTINUE;
-			} else {
-				data->iov.iov_len = data->gatt_len;
-				data->gatt_data[0] |= GATT_SAR_LAST;
-			}
-
-			break;
-
-		default:
-			if(data->cb)
-				data->cb(message, data->user_data);
-			return;
-	}
-
-	if (g_dbus_proxy_method_call(data->proxy, "WriteValue", write_setup,
-				write_reply, data, write_data_free) == FALSE) {
-		bt_shell_printf("Failed to write\n");
-		write_data_free(data);
-		return;
-	}
-
-}
-
 static void write_io_destroy(void)
 {
 	io_destroy(write_io);
@@ -361,12 +282,8 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
 
 	if (dbus_set_error_from_message(&error, message) == TRUE) {
 		dbus_error_free(&error);
-		if (g_dbus_proxy_method_call(data->proxy, "WriteValue",
-				write_setup, write_reply, data,
-				write_data_free) == FALSE) {
-			bt_shell_printf("Failed to write\n");
-			write_data_free(data);
-		}
+		bt_shell_printf("Failed to write\n");
+		write_data_free(data);
 		return;
 	}
 
@@ -402,8 +319,6 @@ bool mesh_gatt_write(GDBusProxy *proxy, uint8_t *buf, uint16_t len,
 			GDBusReturnFunction cb, void *user_data)
 {
 	struct write_data *data;
-	DBusMessageIter iter;
-	uint8_t max_len;
 
 	if (!buf || !len)
 		return false;
@@ -415,17 +330,11 @@ bool mesh_gatt_write(GDBusProxy *proxy, uint8_t *buf, uint16_t len,
 	if (!data)
 		return false;
 
-	max_len = write_mtu ? write_mtu - 3 : GATT_MTU - 3;
-
 	/* TODO: should keep in queue in case we need to cancel write? */
 
 	data->gatt_len = len;
 	data->gatt_data = g_memdup(buf, len);
 	data->gatt_data[0] &= GATT_TYPE_MASK;
-	if (max_len < len) {
-		len = max_len;
-		data->gatt_data[0] |= GATT_SAR_FIRST;
-	}
 	data->iov.iov_base = data->gatt_data;
 	data->iov.iov_len = len;
 	data->proxy = proxy;
@@ -435,27 +344,13 @@ bool mesh_gatt_write(GDBusProxy *proxy, uint8_t *buf, uint16_t len,
 	if (write_io)
 		return pipe_write(write_io, data);
 
-	if (g_dbus_proxy_get_property(proxy, "WriteAcquired", &iter)) {
-		if (g_dbus_proxy_method_call(proxy, "AcquireWrite",
-					acquire_setup, acquire_write_reply,
-					data, NULL) == FALSE) {
-			bt_shell_printf("Failed to AcquireWrite\n");
-			write_data_free(data);
-			return false;
-		}
-	} else {
-		if (g_dbus_proxy_method_call(data->proxy, "WriteValue",
-				write_setup, write_reply, data,
-				write_data_free) == FALSE) {
-			bt_shell_printf("Failed to write\n");
-			write_data_free(data);
-			return false;
-		}
-		print_byte_array("GATT-TX: ", buf, len);
-		bt_shell_printf("Attempting to write %s\n",
-						g_dbus_proxy_get_path(proxy));
+	if (g_dbus_proxy_method_call(proxy, "AcquireWrite",
+				acquire_setup, acquire_write_reply,
+				data, NULL) == FALSE) {
+		bt_shell_printf("Failed to AcquireWrite\n");
+		write_data_free(data);
+		return false;
 	}
-
 	return true;
 }
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux