[PATCH BlueZ 1/2] Add support for cancelling a LE Scan with Control-C

[PATCH BlueZ 1/2] Add support for cancelling a LE Scan with Control-C

[Vinicius Costa Gomes]

---
 tools/hcitool.c |   39 +++++++++++++++++++++++++++++++++++----
 1 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/tools/hcitool.c b/tools/hcitool.c
index 0dac2ae..988ebe3 100644
--- a/tools/hcitool.c
+++ b/tools/hcitool.c
@@ -38,6 +38,7 @@
 #include <sys/param.h>
 #include <sys/ioctl.h>
 #include <sys/socket.h>
+#include <signal.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/hci.h>
@@ -55,6 +56,11 @@
 
 #define for_each_opt(opt, long, short) while ((opt=getopt_long(argc, argv, short ? short:"+", long, NULL)) != -1)
 
+static struct sighandler_data {
+	struct hci_filter of;
+	int fd;
+} signal_data;
+
 static void usage(void);
 
 static int dev_info(int s, int dev_id, long arg)
@@ -2346,12 +2352,30 @@ static int check_report_filter(uint8_t procedure, le_advertising_info *info)
 	return 0;
 }
 
+static void sigint_handler(int sig)
+{
+	int err;
+
+	setsockopt(signal_data.fd, SOL_HCI, HCI_FILTER, &signal_data.of, sizeof(signal_data.of));
+
+	err = hci_le_set_scan_enable(signal_data.fd, 0x00, 0x00, 1000);
+	if (err < 0) {
+		perror("Disable scan failed");
+		exit(1);
+	}
+
+	hci_close_dev(signal_data.fd);
+
+	exit(0);
+}
+
 static int print_advertising_devices(int dd, uint8_t filter_type)
 {
 	unsigned char buf[HCI_MAX_EVENT_SIZE], *ptr;
 	struct hci_filter nf, of;
+	struct sigaction sa;
 	socklen_t olen;
-	int num, len;
+	int len;
 
 	olen = sizeof(of);
 	if (getsockopt(dd, SOL_HCI, HCI_FILTER, &of, &olen) < 0) {
@@ -2368,9 +2392,16 @@ static int print_advertising_devices(int dd, uint8_t filter_type)
 		return -1;
 	}
 
-	/* Wait for 10 report events */
-	num = 10;
-	while (num--) {
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_flags = SA_NOCLDSTOP;
+	sa.sa_handler = sigint_handler;
+	sigaction(SIGINT, &sa, NULL);
+
+	memset(&signal_data, 0, sizeof(signal_data));
+	signal_data.fd = dd;
+	memcpy(&signal_data.of, &of, sizeof(signal_data.of));
+
+	while (1) {
 		evt_le_meta_event *meta;
 		le_advertising_info *info;
 		char addr[18];
-- 
1.7.6.1

[PATCH BlueZ 2/2] Add support for parsing the remote name during LE Scan

[Vinicius Costa Gomes]

---
 tools/hcitool.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/tools/hcitool.c b/tools/hcitool.c
index 988ebe3..c9d1822 100644
--- a/tools/hcitool.c
+++ b/tools/hcitool.c
@@ -54,6 +54,18 @@
 #define FLAGS_LIMITED_MODE_BIT 0x01
 #define FLAGS_GENERAL_MODE_BIT 0x02
 
+#define EIR_FLAGS                   0x01  /* flags */
+#define EIR_UUID16_SOME             0x02  /* 16-bit UUID, more available */
+#define EIR_UUID16_ALL              0x03  /* 16-bit UUID, all listed */
+#define EIR_UUID32_SOME             0x04  /* 32-bit UUID, more available */
+#define EIR_UUID32_ALL              0x05  /* 32-bit UUID, all listed */
+#define EIR_UUID128_SOME            0x06  /* 128-bit UUID, more available */
+#define EIR_UUID128_ALL             0x07  /* 128-bit UUID, all listed */
+#define EIR_NAME_SHORT              0x08  /* shortened local name */
+#define EIR_NAME_COMPLETE           0x09  /* complete local name */
+#define EIR_TX_POWER                0x0A  /* transmit power level */
+#define EIR_DEVICE_ID               0x10  /* device ID */
+
 #define for_each_opt(opt, long, short) while ((opt=getopt_long(argc, argv, short ? short:"+", long, NULL)) != -1)
 
 static struct sighandler_data {
@@ -2369,6 +2381,38 @@ static void sigint_handler(int sig)
 	exit(0);
 }
 
+static char *eir_parse_name(uint8_t *eir_data)
+{
+	int len;
+
+	if (eir_data == NULL)
+		return NULL;
+
+	len = 0;
+	while (len < HCI_MAX_EIR_LENGTH - 1) {
+		uint8_t field_len = eir_data[0];
+		char *name;
+
+		/* Check for the end of EIR */
+		if (field_len == 0)
+			break;
+
+		switch (eir_data[1]) {
+		case EIR_NAME_SHORT:
+		case EIR_NAME_COMPLETE:
+			name = malloc(sizeof(char) * field_len);
+			memset(name, 0, field_len);
+			memcpy(name, &eir_data[2], field_len - 1);
+			return name;
+		}
+
+		len += field_len + 1;
+		eir_data += field_len + 1;
+	}
+
+	return NULL;
+}
+
 static int print_advertising_devices(int dd, uint8_t filter_type)
 {
 	unsigned char buf[HCI_MAX_EVENT_SIZE], *ptr;
@@ -2423,8 +2467,14 @@ static int print_advertising_devices(int dd, uint8_t filter_type)
 		/* Ignoring multiple reports */
 		info = (le_advertising_info *) (meta->data + 1);
 		if (check_report_filter(filter_type, info)) {
+			char *name;
+
 			ba2str(&info->bdaddr, addr);
-			printf("%s\n", addr);
+			name = eir_parse_name(info->data);
+
+			printf("%s %s\n", addr, name ? : "(unknown)");
+
+			free(name);
 		}
 	}
 
-- 
1.7.6.1

Re: [PATCH BlueZ 1/2] Add support for cancelling a LE Scan with Control-C

[Johan Hedberg]

Hi Vinicius,

On Thu, Oct 06, 2011, Vinicius Costa Gomes wrote:
> ---
> +static void sigint_handler(int sig)
> +{
> +	int err;
> +
> +	setsockopt(signal_data.fd, SOL_HCI, HCI_FILTER, &signal_data.of, sizeof(signal_data.of));
> +
> +	err = hci_le_set_scan_enable(signal_data.fd, 0x00, 0x00, 1000);
> +	if (err < 0) {
> +		perror("Disable scan failed");
> +		exit(1);
> +	}
> +
> +	hci_close_dev(signal_data.fd);
> +
> +	exit(0);
> +}

To my understanding the general recommendation is to do as little as
possible within signal handlers. Would it therefore make more sense to
simply set a flag (indicating which signal was received) in the handler,
and then do the necessary cleanup within the original function by
handling EINTR there (presumably returned by the read system call)?

Johan

Re: [PATCH BlueZ 1/2] Add support for cancelling a LE Scan with Control-C

[Vinicius Costa Gomes]

Hi Johan,

On 18:49 Thu 06 Oct, Johan Hedberg wrote:
> Hi Vinicius,
> 
> On Thu, Oct 06, 2011, Vinicius Costa Gomes wrote:
> > ---
> > +static void sigint_handler(int sig)
> > +{
> > +	int err;
> > +
> > +	setsockopt(signal_data.fd, SOL_HCI, HCI_FILTER, &signal_data.of, sizeof(signal_data.of));
> > +
> > +	err = hci_le_set_scan_enable(signal_data.fd, 0x00, 0x00, 1000);
> > +	if (err < 0) {
> > +		perror("Disable scan failed");
> > +		exit(1);
> > +	}
> > +
> > +	hci_close_dev(signal_data.fd);
> > +
> > +	exit(0);
> > +}
> 
> To my understanding the general recommendation is to do as little as
> possible within signal handlers. Would it therefore make more sense to
> simply set a flag (indicating which signal was received) in the handler,
> and then do the necessary cleanup within the original function by
> handling EINTR there (presumably returned by the read system call)?

Yes, makes sense. Will fix.

> 
> Johan


Cheers,
-- 
Vinicius

[PATCH BlueZ] Add support for cancelling a LE Scan with Control-C

[Vinicius Costa Gomes]

---
 tools/hcitool.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/tools/hcitool.c b/tools/hcitool.c
index 0dac2ae..4be0da0 100644
--- a/tools/hcitool.c
+++ b/tools/hcitool.c
@@ -38,6 +38,7 @@
 #include <sys/param.h>
 #include <sys/ioctl.h>
 #include <sys/socket.h>
+#include <signal.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/hci.h>
@@ -55,6 +56,8 @@
 
 #define for_each_opt(opt, long, short) while ((opt=getopt_long(argc, argv, short ? short:"+", long, NULL)) != -1)
 
+static int signal_received;
+
 static void usage(void);
 
 static int dev_info(int s, int dev_id, long arg)
@@ -2346,12 +2349,18 @@ static int check_report_filter(uint8_t procedure, le_advertising_info *info)
 	return 0;
 }
 
+static void sigint_handler(int sig)
+{
+	signal_received = sig;
+}
+
 static int print_advertising_devices(int dd, uint8_t filter_type)
 {
 	unsigned char buf[HCI_MAX_EVENT_SIZE], *ptr;
 	struct hci_filter nf, of;
+	struct sigaction sa;
 	socklen_t olen;
-	int num, len;
+	int len;
 
 	olen = sizeof(of);
 	if (getsockopt(dd, SOL_HCI, HCI_FILTER, &of, &olen) < 0) {
@@ -2368,14 +2377,22 @@ static int print_advertising_devices(int dd, uint8_t filter_type)
 		return -1;
 	}
 
-	/* Wait for 10 report events */
-	num = 10;
-	while (num--) {
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_flags = SA_NOCLDSTOP;
+	sa.sa_handler = sigint_handler;
+	sigaction(SIGINT, &sa, NULL);
+
+	while (1) {
 		evt_le_meta_event *meta;
 		le_advertising_info *info;
 		char addr[18];
 
 		while ((len = read(dd, buf, sizeof(buf))) < 0) {
+			if (errno == EINTR && signal_received == SIGINT) {
+				len = 0;
+				goto done;
+			}
+
 			if (errno == EAGAIN || errno == EINTR)
 				continue;
 			goto done;
-- 
1.7.6.1

Re: [PATCH BlueZ] Add support for cancelling a LE Scan with Control-C

[Johan Hedberg]

Hi Vinicius,

On Thu, Oct 06, 2011, Vinicius Costa Gomes wrote:
> +static int signal_received;

This needs to be declared volatile and according to our coding style
explicitly initialized to 0. Other than that the patch is fine. I went
ahead and fixed this issue myself and pushed the patch upstream. Thanks.

Johan

Re: [PATCH BlueZ 2/2] Add support for parsing the remote name during LE Scan

[Johan Hedberg]

Hi Vinicius,

On Thu, Oct 06, 2011, Vinicius Costa Gomes wrote:
> ---
>  tools/hcitool.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 51 insertions(+), 1 deletions(-)

Could you rebase and resend this patch. It doesn't apply anymore.

Johan

[PATCH BlueZ] Add support for parsing the remote name during LE Scan

[Vinicius Costa Gomes]

---
 tools/hcitool.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/tools/hcitool.c b/tools/hcitool.c
index 6184a4c..8baf7e7 100644
--- a/tools/hcitool.c
+++ b/tools/hcitool.c
@@ -54,6 +54,18 @@
 #define FLAGS_LIMITED_MODE_BIT 0x01
 #define FLAGS_GENERAL_MODE_BIT 0x02
 
+#define EIR_FLAGS                   0x01  /* flags */
+#define EIR_UUID16_SOME             0x02  /* 16-bit UUID, more available */
+#define EIR_UUID16_ALL              0x03  /* 16-bit UUID, all listed */
+#define EIR_UUID32_SOME             0x04  /* 32-bit UUID, more available */
+#define EIR_UUID32_ALL              0x05  /* 32-bit UUID, all listed */
+#define EIR_UUID128_SOME            0x06  /* 128-bit UUID, more available */
+#define EIR_UUID128_ALL             0x07  /* 128-bit UUID, all listed */
+#define EIR_NAME_SHORT              0x08  /* shortened local name */
+#define EIR_NAME_COMPLETE           0x09  /* complete local name */
+#define EIR_TX_POWER                0x0A  /* transmit power level */
+#define EIR_DEVICE_ID               0x10  /* device ID */
+
 #define for_each_opt(opt, long, short) while ((opt=getopt_long(argc, argv, short ? short:"+", long, NULL)) != -1)
 
 static volatile int signal_received = 0;
@@ -2354,6 +2366,38 @@ static void sigint_handler(int sig)
 	signal_received = sig;
 }
 
+static char *eir_parse_name(uint8_t *eir_data)
+{
+	int len;
+
+	if (eir_data == NULL)
+		return NULL;
+
+	len = 0;
+	while (len < HCI_MAX_EIR_LENGTH - 1) {
+		uint8_t field_len = eir_data[0];
+		char *name;
+
+		/* Check for the end of EIR */
+		if (field_len == 0)
+			break;
+
+		switch (eir_data[1]) {
+		case EIR_NAME_SHORT:
+		case EIR_NAME_COMPLETE:
+			name = malloc(sizeof(char) * field_len);
+			memset(name, 0, field_len);
+			memcpy(name, &eir_data[2], field_len - 1);
+			return name;
+		}
+
+		len += field_len + 1;
+		eir_data += field_len + 1;
+	}
+
+	return NULL;
+}
+
 static int print_advertising_devices(int dd, uint8_t filter_type)
 {
 	unsigned char buf[HCI_MAX_EVENT_SIZE], *ptr;
@@ -2409,8 +2453,14 @@ static int print_advertising_devices(int dd, uint8_t filter_type)
 		/* Ignoring multiple reports */
 		info = (le_advertising_info *) (meta->data + 1);
 		if (check_report_filter(filter_type, info)) {
+			char *name;
+
 			ba2str(&info->bdaddr, addr);
-			printf("%s\n", addr);
+			name = eir_parse_name(info->data);
+
+			printf("%s %s\n", addr, name ? : "(unknown)");
+
+			free(name);
 		}
 	}
 
-- 
1.7.7

[PATCH BlueZ] Add support for parsing the remote name during LE Scan

[Vinicius Costa Gomes]

---
 tools/hcitool.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 53 insertions(+), 1 deletions(-)

diff --git a/tools/hcitool.c b/tools/hcitool.c
index 6184a4c..3547794 100644
--- a/tools/hcitool.c
+++ b/tools/hcitool.c
@@ -54,6 +54,18 @@
 #define FLAGS_LIMITED_MODE_BIT 0x01
 #define FLAGS_GENERAL_MODE_BIT 0x02
 
+#define EIR_FLAGS                   0x01  /* flags */
+#define EIR_UUID16_SOME             0x02  /* 16-bit UUID, more available */
+#define EIR_UUID16_ALL              0x03  /* 16-bit UUID, all listed */
+#define EIR_UUID32_SOME             0x04  /* 32-bit UUID, more available */
+#define EIR_UUID32_ALL              0x05  /* 32-bit UUID, all listed */
+#define EIR_UUID128_SOME            0x06  /* 128-bit UUID, more available */
+#define EIR_UUID128_ALL             0x07  /* 128-bit UUID, all listed */
+#define EIR_NAME_SHORT              0x08  /* shortened local name */
+#define EIR_NAME_COMPLETE           0x09  /* complete local name */
+#define EIR_TX_POWER                0x0A  /* transmit power level */
+#define EIR_DEVICE_ID               0x10  /* device ID */
+
 #define for_each_opt(opt, long, short) while ((opt=getopt_long(argc, argv, short ? short:"+", long, NULL)) != -1)
 
 static volatile int signal_received = 0;
@@ -2354,6 +2366,40 @@ static void sigint_handler(int sig)
 	signal_received = sig;
 }
 
+static void eir_parse_name(uint8_t *eir_data, char *name)
+{
+	int len;
+
+	if (eir_data == NULL)
+		goto failed;
+
+	len = 0;
+	while (len < HCI_MAX_EIR_LENGTH - 1) {
+		uint8_t field_len = eir_data[0];
+
+		/* Check for the end of EIR */
+		if (field_len == 0)
+			break;
+
+		switch (eir_data[1]) {
+		case EIR_NAME_SHORT:
+		case EIR_NAME_COMPLETE:
+			if (field_len > HCI_MAX_NAME_LENGTH)
+				goto failed;
+
+			memcpy(name, &eir_data[2], field_len - 1);
+			return;
+		}
+
+		len += field_len + 1;
+		eir_data += field_len + 1;
+	}
+
+failed:
+	sprintf(name, "(unknown)");
+	return;
+}
+
 static int print_advertising_devices(int dd, uint8_t filter_type)
 {
 	unsigned char buf[HCI_MAX_EVENT_SIZE], *ptr;
@@ -2409,8 +2455,14 @@ static int print_advertising_devices(int dd, uint8_t filter_type)
 		/* Ignoring multiple reports */
 		info = (le_advertising_info *) (meta->data + 1);
 		if (check_report_filter(filter_type, info)) {
+			char name[HCI_MAX_NAME_LENGTH + 1];
+
+			memset(name, 0, sizeof(name));
+
 			ba2str(&info->bdaddr, addr);
-			printf("%s\n", addr);
+			eir_parse_name(info->data, name);
+
+			printf("%s %s\n", addr, name);
 		}
 	}
 
-- 
1.7.7

Re: [PATCH BlueZ] Add support for parsing the remote name during LE Scan

[Johan Hedberg]

Hi Vinicius,

On Thu, Oct 13, 2011, Vinicius Costa Gomes wrote:
> +	while (len < HCI_MAX_EIR_LENGTH - 1) {
> +		uint8_t field_len = eir_data[0];
> +
> +		/* Check for the end of EIR */
> +		if (field_len == 0)
> +			break;

I suppose there should also be a check for:

	if (len + field_len > HCI_MAX_EIR_LENGTH)
		goto failed;

Otherwise you're gonna access past the end of the eir_data buffer when
you do the memcpy later.

> +
> +		switch (eir_data[1]) {
> +		case EIR_NAME_SHORT:
> +		case EIR_NAME_COMPLETE:
> +			if (field_len > HCI_MAX_NAME_LENGTH)
> +				goto failed;

If you add the if-statement I suggested earlier you can remove this one
(since it becomes redundant).

> +
> +			memcpy(name, &eir_data[2], field_len - 1);
> +			return;
> +		}
> +
> +		len += field_len + 1;
> +		eir_data += field_len + 1;
> +	}
> +
> +failed:
> +	sprintf(name, "(unknown)");
> +	return;
> +}

Please remove the unnecessary return statement here.

Johan

Re: [PATCH BlueZ] Add support for parsing the remote name during LE Scan

[Johan Hedberg]

Hi Vinicius,

On Fri, Oct 14, 2011, Johan Hedberg wrote:
> Hi Vinicius,
> 
> On Thu, Oct 13, 2011, Vinicius Costa Gomes wrote:
> > +	while (len < HCI_MAX_EIR_LENGTH - 1) {
> > +		uint8_t field_len = eir_data[0];
> > +
> > +		/* Check for the end of EIR */
> > +		if (field_len == 0)
> > +			break;
> 
> I suppose there should also be a check for:
> 
> 	if (len + field_len > HCI_MAX_EIR_LENGTH)
> 		goto failed;
> 
> Otherwise you're gonna access past the end of the eir_data buffer when
> you do the memcpy later.
> 
> > +
> > +		switch (eir_data[1]) {
> > +		case EIR_NAME_SHORT:
> > +		case EIR_NAME_COMPLETE:
> > +			if (field_len > HCI_MAX_NAME_LENGTH)
> > +				goto failed;
> 
> If you add the if-statement I suggested earlier you can remove this one
> (since it becomes redundant).
> 
> > +
> > +			memcpy(name, &eir_data[2], field_len - 1);
> > +			return;
> > +		}
> > +
> > +		len += field_len + 1;
> > +		eir_data += field_len + 1;
> > +	}
> > +
> > +failed:
> > +	sprintf(name, "(unknown)");
> > +	return;
> > +}
> 
> Please remove the unnecessary return statement here.

I had a slight confusion with my local patch handling and your patch
went upstream by mistake. So, I ended up fixing these issues by myself.
There were actually several more issues which I spotted and fixed in the
same go:

- read_flags() had missing checks too
- The LE EIR data length is variable and max 0x1f (31) bytes, i.e. not
  HCI_MAX_EIR_LENGTH
- Because of the above limit the name is max 29 bytes

Please check upstream (github) and verify that I didn't make any mistake
(since I was only able to do only limited testing here).

Johan