Honor offset when reading external chrc.

PATCH In review Andrejs Hanins 2 participants 3 messages Mar 16, 2015 Bluetooth drivers

lore

Share & tools

lore

Andrejs Hanins

author

2015-03-16 15:11 UTC #0 lore raw

PATCH 1/1 Honor offset when reading external chrc.

Honor offset when reading external characteristics via D-Bus and respond with properly chunked data.

---
 src/gatt-database.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

src/gatt-database.c +20 -5

index e07c70f..3ae2f62 100644
@@ -119,6 +119,7 @@ struct pending_op {
 	struct gatt_db_attribute *attrib;
 	struct queue *owner_queue;
 	struct iovec data;
+	uint16_t offset;
 };
 
 struct device_state {

@@ -1446,6 +1447,19 @@ static void read_reply_cb(DBusMessage *message, void *user_data)
 		goto done;
 	}
 
+	if (op->offset > len) {
+		ecode = BT_ATT_ERROR_INVALID_OFFSET;
+		value = NULL;
+		len = 0;
+		goto done;
+	} else if (op->offset == len) {
+		value = NULL;
+		len = 0;
+	} else {
+		len -= op->offset;
+		value = &value[op->offset];
+	}
+
 	/* Truncate the value if it's too large */
 	len = MIN(BT_ATT_MAX_VALUE_LEN, len);
 	value = len ? value : NULL;

@@ -1466,7 +1480,7 @@ static void pending_op_free(void *data)
 
 static struct pending_op *pending_read_new(struct queue *owner_queue,
 					struct gatt_db_attribute *attrib,
-					unsigned int id)
+					unsigned int id, uint16_t offset)
 {
 	struct pending_op *op;

@@ -1477,6 +1491,7 @@ static struct pending_op *pending_read_new(struct queue *owner_queue,
 	op->owner_queue = owner_queue;
 	op->attrib = attrib;
 	op->id = id;
+	op->offset = offset;
 	queue_push_tail(owner_queue, op);
 
 	return op;

@@ -1484,12 +1499,12 @@ static struct pending_op *pending_read_new(struct queue *owner_queue,
 
 static void send_read(struct gatt_db_attribute *attrib, GDBusProxy *proxy,
 						struct queue *owner_queue,
-						unsigned int id)
+						unsigned int id, uint16_t offset)
 {
 	struct pending_op *op;
 	uint8_t ecode = BT_ATT_ERROR_UNLIKELY;
 
-	op = pending_read_new(owner_queue, attrib, id);
+	op = pending_read_new(owner_queue, attrib, id, offset);
 	if (!op) {
 		error("Failed to allocate memory for pending read call");
 		ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES;

@@ -1785,7 +1800,7 @@ static void desc_read_cb(struct gatt_db_attribute *attrib,
 		return;
 	}
 
-	send_read(attrib, desc->proxy, desc->pending_reads, id);
+	send_read(attrib, desc->proxy, desc->pending_reads, id, offset);
 }
 
 static void desc_write_cb(struct gatt_db_attribute *attrib,

@@ -1843,7 +1858,7 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib,
 		return;
 	}
 
-	send_read(attrib, chrc->proxy, chrc->pending_reads, id);
+	send_read(attrib, chrc->proxy, chrc->pending_reads, id, offset);
 }
 
 static void chrc_write_cb(struct gatt_db_attribute *attrib,

Signature · 1 line

1.9.1

Andrejs Hanins

author

2015-03-16 16:04 UTC #2 lore raw

Re: [PATCH] core/gatt-database: Honor offset when reading external chrc.

Hi Arman,

Quoted 6 lines · Arman Uguray

Hi Andrejs,
Quoted 3 lines · Andrejs Hanins
Honor offset when reading external characteristics via D-Bus and respond with properly chunked data.
---

Quoted 6 lines

The subject should contain "core/gatt: " as its prefix, at least that's how we've been organizing patches to different parts of the code.
Also, a similar problem may also exist with WriteValue but we may not be able to fully address it without an API change.

You are right. I just made a test by writing a big chunk of data (more than negotiated ATT_MTU) from the mobile phone and D-Bus method "WriteValue" is called twice - for the first and second pieces of data. But the data itself looks to be correct, i.e. both chunks concatenated together give the original data. IMO, the bluez daemon should do the assembly of long write chunks and call D-Bus "WriteValue" only once providing the whole write request as initiated by the other side. This way, the API will continue to be simple.

Quoted 52 lines

Quoted 17 lines

 src/gatt-database.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

src/gatt-database.c +1 -0

index e07c70f..3ae2f62 100644
@@ -119,6 +119,7 @@ struct pending_op {
        struct gatt_db_attribute *attrib;
        struct queue *owner_queue;
        struct iovec data;
+       uint16_t offset;
 };
 
 struct device_state {

src/gatt-database.c static void read_reply_cb(DBusMessage *message, void *user_data) · line 1455

Quoted 9 lines

src/gatt-database.c +6 -0

@@ -1446,6 +1447,19 @@ static void read_reply_cb(DBusMessage *message, void *user_data)
                goto done;
        }
 
+       if (op->offset > len) {
+               ecode = BT_ATT_ERROR_INVALID_OFFSET;
+               value = NULL;
+               len = 0;
+               goto done;
+       } else if (op->offset == len) {

No need for the else statement since the previous if-body ends in a goto.

src/gatt-database.c static void read_reply_cb(DBusMessage *message, void *user_data) · line 1461

Quoted 6 lines

src/gatt-database.c +6 -0

+               value = NULL;
+               len = 0;
+       } else {
+               len -= op->offset;
+               value = &value[op->offset];
+       }

The above code is a bit redundant since we already do the NULL assignment below. Basically, all you should need to do is this:

       if (op->offset > len) {
               ecode = BT_ATT_ERROR_INVALID_OFFSET;
               value = NULL;
               len = 0;
               goto done;
       }

       len -= op->offset;

       /* Truncate the value if it's too large */
       len = MIN(BT_ATT_MAX_VALUE_LEN, len);
       value = len ? &value[op->offset] : NULL;

Quoted 3 lines

If len == op->offset, len will end up with 0 as its value and since you already handle the offset being larger than len above, this won't cause an overflow.

Noted. Thank you!

Quoted 62 lines

Quoted 56 lines

src/gatt-database.c +7 -6

+
        /* Truncate the value if it's too large */
        len = MIN(BT_ATT_MAX_VALUE_LEN, len);
        value = len ? value : NULL;

@@ -1466,7 +1480,7 @@ static void pending_op_free(void *data)
 
 static struct pending_op *pending_read_new(struct queue *owner_queue,
                                        struct gatt_db_attribute *attrib,
-                                       unsigned int id)
+                                       unsigned int id, uint16_t offset)
 {
        struct pending_op *op;

@@ -1477,6 +1491,7 @@ static struct pending_op *pending_read_new(struct queue *owner_queue,
        op->owner_queue = owner_queue;
        op->attrib = attrib;
        op->id = id;
+       op->offset = offset;
        queue_push_tail(owner_queue, op);
 
        return op;

@@ -1484,12 +1499,12 @@ static struct pending_op *pending_read_new(struct queue *owner_queue,
 
 static void send_read(struct gatt_db_attribute *attrib, GDBusProxy *proxy,
                                                struct queue *owner_queue,
-                                               unsigned int id)
+                                               unsigned int id, uint16_t offset)
 {
        struct pending_op *op;
        uint8_t ecode = BT_ATT_ERROR_UNLIKELY;
 
-       op = pending_read_new(owner_queue, attrib, id);
+       op = pending_read_new(owner_queue, attrib, id, offset);
        if (!op) {
                error("Failed to allocate memory for pending read call");
                ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES;

@@ -1785,7 +1800,7 @@ static void desc_read_cb(struct gatt_db_attribute *attrib,
                return;
        }
 
-       send_read(attrib, desc->proxy, desc->pending_reads, id);
+       send_read(attrib, desc->proxy, desc->pending_reads, id, offset);
 }
 
 static void desc_write_cb(struct gatt_db_attribute *attrib,

@@ -1843,7 +1858,7 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib,
                return;
        }
 
-       send_read(attrib, chrc->proxy, chrc->pending_reads, id);
+       send_read(attrib, chrc->proxy, chrc->pending_reads, id, offset);
 }
 
 static void chrc_write_cb(struct gatt_db_attribute *attrib,
--

Quoted 2 lines

1.9.1

Thanks,
Arman