serial_bt, bluez: rework diag in BLE reception, accept zero length data

Rework how the sr_bt_check_notify() routine processes messages that were
received from a Bluetooth socket. Comment on edge cases that we have yet
to see how they are handled. Extend the amount of details that are given
in diagnostics messages, and tune their verbosity level. The debug level
nicely illustrates how data flows, the spew level makes bytes available.

Consider zero length data chunks in receive code paths non-fatal. Accept
either NULL or non-NULL addresses when the length is zero. A NULL pointer
in combination with a non-zero length is considered fatal though.

diff --git a/src/bt/bt_bluez.c b/src/bt/bt_bluez.c
index d0a3b7c847605ca2d427fb8e93f9e36eea02ded0..71c0dbd4e5b902fbe2126344236c676e816f9548 100644 (file)
--- a/src/bt/bt_bluez.c
+++ b/src/bt/bt_bluez.c
@@ -74,6 +74,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <glib.h>
+#include <inttypes.h>
 #include <poll.h>
 #include <stdarg.h>
 #include <stdio.h>
@@ -902,10 +903,14 @@ SR_PRIV int sr_bt_check_notify(struct sr_bt_desc *desc)
 {
        uint8_t buf[1024];
        ssize_t rdlen;
+       const uint8_t *bufptr;
+       size_t buflen;
        uint8_t packet_type;
        uint16_t packet_handle;
        uint8_t *packet_data;
        size_t packet_dlen;
+       const char *type_text;
+       int ret;
 
        if (!desc)
                return -1;
@@ -913,57 +918,96 @@ SR_PRIV int sr_bt_check_notify(struct sr_bt_desc *desc)
        if (sr_bt_check_socket_usable(desc) < 0)
                return -2;
 
-       /* Get another message from the Bluetooth socket. */
+       /*
+        * Get another message from the Bluetooth socket.
+        *
+        * TODO Can we assume that every "message" comes in a separate
+        * read(2) call, or can data combine at the caller's? Need we
+        * loop over the received content until all was consumed?
+        */
        rdlen = sr_bt_read(desc, buf, sizeof(buf));
-       if (rdlen < 0)
+       if (rdlen < 0) {
+               sr_dbg("check notifiy, read error, %zd", rdlen);
                return -2;
-       if (!rdlen)
+       }
+       if (!rdlen) {
+               if (0) sr_spew("check notifiy, empty read");
                return 0;
+       }
+       bufptr = &buf[0];
+       buflen = (size_t)rdlen;
+       if (sr_log_loglevel_get() >= SR_LOG_SPEW) {
+               GString *txt;
+               txt = sr_hexdump_new(bufptr, buflen);
+               sr_spew("check notifiy, read succes, length %zd, data: %s",
+                       rdlen, txt->str);
+               sr_hexdump_free(txt);
+       }
 
-       /* Get header fields and references to the payload data. */
-       packet_type = 0x00;
+       /*
+        * Get header fields and references to the payload data. Notice
+        * that the first 16bit item after the packet type often is the
+        * handle, but need not always be. That is why the read position
+        * is kept, so that individual packet type handlers can either
+        * read _their_ layout strictly sequentially, or can conveniently
+        * access what a common preparation step has provided to them.
+        */
        packet_handle = 0x0000;
        packet_data = NULL;
        packet_dlen = 0;
-       if (rdlen >= 1)
-               packet_type = buf[0];
-       if (rdlen >= 3) {
-               packet_handle = bt_get_le16(&buf[1]);
-               packet_data = &buf[3];
-               packet_dlen = rdlen - 3;
+       packet_type = read_u8_inc_len(&bufptr, &buflen);
+       if (buflen >= sizeof(uint16_t)) {
+               packet_handle = read_u16le(bufptr);
+               packet_data = (void *)&bufptr[sizeof(uint16_t)];
+               packet_dlen = buflen - sizeof(uint16_t);
+               if (!packet_dlen)
+                       packet_data = NULL;
        }
+       if (0) sr_spew("check notifiy, prep, hdl %" PRIu16 ", data %p len %zu",
+               packet_handle, packet_data, packet_dlen);
 
        /* Dispatch according to the message type. */
        switch (packet_type) {
        case BLE_ATT_ERROR_RESP:
-               sr_spew("read() len %zd, type 0x%02x (%s)", rdlen, buf[0], "error response");
+               type_text = "error response";
+               if (!buflen) {
+                       sr_dbg("%s, no payload", type_text);
+                       break;
+               }
                /* EMPTY */
+               sr_dbg("%s, not handled here", type_text);
                break;
        case BLE_ATT_WRITE_RESP:
-               sr_spew("read() len %zd, type 0x%02x (%s)", rdlen, buf[0], "write response");
-               /* EMPTY */
+               type_text = "write response";
+               sr_dbg("%s, note taken", type_text);
                break;
        case BLE_ATT_HANDLE_INDICATION:
-               sr_spew("read() len %zd, type 0x%02x (%s)", rdlen, buf[0], "handle indication");
+               type_text = "handle indication";
+               sr_dbg("%s, data len %zu", type_text, packet_dlen);
                sr_bt_write_type(desc, BLE_ATT_HANDLE_CONFIRMATION);
+               sr_spew("%s, confirmation sent", type_text);
                if (packet_handle != desc->read_handle)
                        return -4;
-               if (!packet_data)
-                       return -4;
                if (!desc->data_cb)
                        return 0;
-               return desc->data_cb(desc->data_cb_data, packet_data, packet_dlen);
+               ret = desc->data_cb(desc->data_cb_data,
+                       packet_data, packet_dlen);
+               sr_spew("%s, data cb ret %d", type_text, ret);
+               return ret;
        case BLE_ATT_HANDLE_NOTIFICATION:
-               sr_spew("read() len %zd, type 0x%02x (%s)", rdlen, buf[0], "handle notification");
+               type_text = "handle notification";
+               sr_dbg("%s, data len %zu", type_text, packet_dlen);
                if (packet_handle != desc->read_handle)
                        return -4;
-               if (!packet_data)
-                       return -4;
                if (!desc->data_cb)
                        return 0;
-               return desc->data_cb(desc->data_cb_data, packet_data, packet_dlen);
+               ret = desc->data_cb(desc->data_cb_data,
+                       packet_data, packet_dlen);
+               sr_spew("%s, data cb ret %d", type_text, ret);
+               return ret;
        default:
-               sr_spew("unsupported type 0x%02x", packet_type);
+               sr_dbg("unhandled type 0x%02x, len %zu",
+                       packet_type, buflen);
                return -3;
        }
 

diff --git a/src/serial_bt.c b/src/serial_bt.c
index ba2edab0e06a9e594547abb78c28881be96b2a3c..33abec0ff481715993920aed1fc57a59433c44a7 100644 (file)
--- a/src/serial_bt.c
+++ b/src/serial_bt.c
@@ -408,6 +408,11 @@ static int ser_bt_data_cb(void *cb_data, uint8_t *data, size_t dlen)
        if (!serial)
                return -1;
 
+       if (!data && dlen)
+               return -1;
+       if (!data || !dlen)
+               return 0;
+
        ser_bt_mask_databits(serial, data, dlen);
        sr_ser_queue_rx_data(serial, data, dlen);