[PATCH 0/8] shared/hfp: Improvements and fixes to HFP AG parser

[PATCH 0/8] shared/hfp: Improvements and fixes to HFP AG parser

[Lukasz Rymanowski]

This set brings couple of fixes on parsing AT commands.
This improves robustness.

Also some minor fixes has beed done here

Lukasz Rymanowski (8):
  shared/hfp: Trivial, fix comments style
  shared/hfp: Fix setting result pending flag
  shared/hfp: Improve handling AT command
  unit/test-hfp: Add test for reading \rAT+X\r
  shared/hfp: Refactor call_prefix_handler
  shared/hfp: Remove not used assignment
  shared/hfp: Improve handling AT commands
  unit/test-hfp: Fix parsing empty string test

 src/shared/hfp.c | 123 +++++++++++++++++++++++++++++++++++--------------------
 unit/test-hfp.c  |  41 ++++++++++++++++++-
 2 files changed, 117 insertions(+), 47 deletions(-)

-- 
1.8.4

[PATCH 1/8] shared/hfp: Trivial, fix comments style

[Lukasz Rymanowski]

---
 src/shared/hfp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index a35cb7b..7ad6e42 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -473,7 +473,8 @@ static void process_input(struct hfp_gw *hfp)
 		char *str2;
 		size_t len2;
 
-		/* If there is no more data in ringbuffer,
+		/*
+		 * If there is no more data in ringbuffer,
 		 * it's just an incomplete command.
 		 */
 		if (len == ringbuf_len(hfp->read_buf))
-- 
1.8.4

[PATCH 2/8] shared/hfp: Fix setting result pending flag

[Lukasz Rymanowski]

Let's set result_pending flag only when we are waiting for result from
upper layer. It will be used in following patch.
---
 src/shared/hfp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 7ad6e42..3b61759 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -505,13 +505,13 @@ static void process_input(struct hfp_gw *hfp)
 		*ptr = '\0';
 	}
 
-	hfp->result_pending = true;
-
 	if (!call_prefix_handler(hfp, str)) {
-		if (hfp->command_callback)
+		if (hfp->command_callback) {
 			hfp->command_callback(str, hfp->command_data);
-		else
+			hfp->result_pending = true;
+		} else {
 			hfp_gw_send_result(hfp, HFP_RESULT_ERROR);
+		}
 	}
 
 	len = ringbuf_drain(hfp->read_buf, count + 1);
-- 
1.8.4

[PATCH 3/8] shared/hfp: Improve handling AT command

[Lukasz Rymanowski]

If for some reason there is more than one AT command in the ring
buffer we should make sure that after upper layer sends result, AT
parser gets back to reading data from ring buffer.
---
 src/shared/hfp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 3b61759..42e4c6b 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -722,7 +722,14 @@ bool hfp_gw_send_result(struct hfp_gw *hfp, enum hfp_result result)
 
 	wakeup_writer(hfp);
 
-	hfp->result_pending = false;
+	/*
+	 * There might be already something to read in the ring buffer.
+	 * If so, let's read it.
+	 */
+	if (hfp->result_pending) {
+		hfp->result_pending = false;
+		can_read_data(hfp->io, hfp);
+	}
 
 	return true;
 }
-- 
1.8.4

[PATCH 4/8] unit/test-hfp: Add test for reading \rAT+X\r

[Lukasz Rymanowski]

---
 unit/test-hfp.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/unit/test-hfp.c b/unit/test-hfp.c
index ef8a100..b22b8a5 100644
--- a/unit/test-hfp.c
+++ b/unit/test-hfp.c
@@ -453,6 +453,19 @@ static void check_string_2(struct hfp_context *result,
 	hfp_gw_send_result(context->hfp, HFP_RESULT_ERROR);
 }
 
+static void check_string_3(struct hfp_context *result,
+				enum hfp_gw_cmd_type type, void *user_data)
+{
+	struct context *context = user_data;
+	const struct test_pdu *pdu;
+
+	pdu = &context->data->pdu_list[context->pdu_offset++];
+
+	g_assert(type == pdu->type);
+
+	hfp_gw_send_result(context->hfp, HFP_RESULT_ERROR);
+}
+
 static void test_hf_init(gconstpointer data)
 {
 	struct context *context = create_context(data);
@@ -725,10 +738,15 @@ int main(int argc, char *argv[])
 									'\r'),
 			type_pdu(HFP_GW_CMD_TYPE_SET, 0),
 			data_end());
+	define_test("/hfp/test_corrupted_1", test_register, check_string_3,
+			raw_pdu('D', '\0'),
+			raw_pdu('\r', 'A', 'T', 'D', '\"', '0', '1', '2', '3',
+								'\"', '\r'),
+			type_pdu(HFP_GW_CMD_TYPE_SET, 0),
+			data_end());
 	define_test("/hfp/test_empty", test_fragmented, NULL,
 			raw_pdu('\r'),
 			data_end());
-
 	define_hf_test("/hfp_hf/test_init", test_hf_init, NULL, NULL,
 			data_end());
 	define_hf_test("/hfp_hf/test_send_command_1", test_hf_send_command,
-- 
1.8.4

[PATCH 5/8] shared/hfp: Refactor call_prefix_handler

[Lukasz Rymanowski]

With this patch call_prefix_handle gets new name handle_at_command and
returns true if correct at command has been handled or false when
provided data does not contain any AT command.
---
 src/shared/hfp.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 42e4c6b..2a09658 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -179,7 +179,18 @@ static void skip_whitespace(struct hfp_context *context)
 		context->offset++;
 }
 
-static bool call_prefix_handler(struct hfp_gw *hfp, const char *data)
+static void handle_unknown_at_command(struct hfp_gw *hfp,
+							const char *data)
+{
+	if (hfp->command_callback) {
+		hfp->command_callback(data, hfp->command_data);
+		hfp->result_pending = true;
+	} else {
+		hfp_gw_send_result(hfp, HFP_RESULT_ERROR);
+	}
+}
+
+static bool handle_at_command(struct hfp_gw *hfp, const char *data)
 {
 	struct cmd_handler *handler;
 	const char *separators = ";?=\0";
@@ -247,8 +258,10 @@ done:
 
 	handler = queue_find(hfp->cmd_handlers, match_handler_prefix,
 								lookup_prefix);
-	if (!handler)
-		return false;
+	if (!handler) {
+		handle_unknown_at_command(hfp, data);
+		return true;
+	}
 
 	handler->callback(&context, type, handler->user_data);
 
@@ -505,14 +518,7 @@ static void process_input(struct hfp_gw *hfp)
 		*ptr = '\0';
 	}
 
-	if (!call_prefix_handler(hfp, str)) {
-		if (hfp->command_callback) {
-			hfp->command_callback(str, hfp->command_data);
-			hfp->result_pending = true;
-		} else {
-			hfp_gw_send_result(hfp, HFP_RESULT_ERROR);
-		}
-	}
+	handle_at_command(hfp, str);
 
 	len = ringbuf_drain(hfp->read_buf, count + 1);
 
-- 
1.8.4

[PATCH 6/8] shared/hfp: Remove not used assignment

[Lukasz Rymanowski]

There is no need to store how much data has been drain. It is not used.
---
 src/shared/hfp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 2a09658..41eae74 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -520,7 +520,7 @@ static void process_input(struct hfp_gw *hfp)
 
 	handle_at_command(hfp, str);
 
-	len = ringbuf_drain(hfp->read_buf, count + 1);
+	ringbuf_drain(hfp->read_buf, count + 1);
 
 	if (free_ptr)
 		free(ptr);
-- 
1.8.4

[PATCH 7/8] shared/hfp: Improve handling AT commands

[Lukasz Rymanowski]

This patch improves handling AT commands in cases:

1. In ring buffer we have currupted commands like Y\rAT+X\r
2. In ring buffer for some reason we have more than AT commands and all
commands are unknow except the last one.

In bith cases parser read data until first \r and then stop to read.
This is fixed with this patch
---
 src/shared/hfp.c | 91 ++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 55 insertions(+), 36 deletions(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 41eae74..5a91d12 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -476,54 +476,73 @@ static void process_input(struct hfp_gw *hfp)
 	char *str, *ptr;
 	size_t len, count;
 	bool free_ptr = false;
+	bool read_again;
 
-	str = ringbuf_peek(hfp->read_buf, 0, &len);
-	if (!str)
-		return;
+	do {
+		str = ringbuf_peek(hfp->read_buf, 0, &len);
+		if (!str)
+			return;
 
-	ptr = memchr(str, '\r', len);
-	if (!ptr) {
-		char *str2;
-		size_t len2;
+		ptr = memchr(str, '\r', len);
+		if (!ptr) {
+			char *str2;
+			size_t len2;
 
-		/*
-		 * If there is no more data in ringbuffer,
-		 * it's just an incomplete command.
-		 */
-		if (len == ringbuf_len(hfp->read_buf))
-			return;
+			/*
+			 * If there is no more data in ringbuffer,
+			 * it's just an incomplete command.
+			 */
+			if (len == ringbuf_len(hfp->read_buf))
+				return;
 
-		str2 = ringbuf_peek(hfp->read_buf, len, &len2);
-		if (!str2)
-			return;
+			str2 = ringbuf_peek(hfp->read_buf, len, &len2);
+			if (!str2)
+				return;
 
-		ptr = memchr(str2, '\r', len2);
-		if (!ptr)
-			return;
+			ptr = memchr(str2, '\r', len2);
+			if (!ptr)
+				return;
 
-		*ptr = '\0';
+			*ptr = '\0';
 
-		count = len2 + len;
-		ptr = malloc(count);
-		if (!ptr)
-			return;
+			count = len2 + len;
+			ptr = malloc(count);
+			if (!ptr)
+				return;
 
-		memcpy(ptr, str, len);
-		memcpy(ptr + len, str2, len2);
+			memcpy(ptr, str, len);
+			memcpy(ptr + len, str2, len2);
 
-		free_ptr = true;
-		str = ptr;
-	} else {
-		count = ptr - str;
-		*ptr = '\0';
-	}
+			free_ptr = true;
+			str = ptr;
+		} else {
+			count = ptr - str;
+			*ptr = '\0';
+		}
+
+		if (!handle_at_command(hfp, str))
+			/*
+			 * Command is not handled that means that was some
+			 * trash. Let's skip that and keep reading from ring
+			 * buffer.
+			 */
+			read_again = true;
+		else
+			/*
+			 * Command has been handled. If we are waiting for a
+			 * result from upper layer, we can stop reading. If we
+			 * already reply i.e. ERROR on unknown command, then we
+			 * can keep reading ring buffer. Actually ring buffer
+			 * should be empty but lets just look there.
+			 */
+			read_again = !hfp->result_pending;
 
-	handle_at_command(hfp, str);
+		ringbuf_drain(hfp->read_buf, count + 1);
 
-	ringbuf_drain(hfp->read_buf, count + 1);
+		if (free_ptr)
+			free(ptr);
 
-	if (free_ptr)
-		free(ptr);
+	} while (read_again);
 }
 
 static void read_watch_destroy(void *user_data)
-- 
1.8.4

[PATCH 8/8] unit/test-hfp: Fix parsing empty string test

[Lukasz Rymanowski]

Empty string shall be ignored by AT parser and no response shall be
sent. Therefore this test should close HFP connection just after empty
string is sent, otherwise test hungs.
---
 unit/test-hfp.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/unit/test-hfp.c b/unit/test-hfp.c
index b22b8a5..face9a4 100644
--- a/unit/test-hfp.c
+++ b/unit/test-hfp.c
@@ -358,6 +358,25 @@ static void test_fragmented(gconstpointer data)
 	execute_context(context);
 }
 
+static void test_send_and_close(gconstpointer data)
+{
+	struct context *context = create_context(data);
+	bool ret;
+
+	context->hfp = hfp_gw_new(context->fd_client);
+	g_assert(context->hfp);
+
+	ret = hfp_gw_set_close_on_unref(context->hfp, true);
+	g_assert(ret);
+
+	send_pdu(context);
+
+	hfp_gw_unref(context->hfp);
+	context->hfp = NULL;
+
+	execute_context(context);
+}
+
 static void check_ustring_1(struct hfp_context *result,
 				enum hfp_gw_cmd_type type, void *user_data)
 {
@@ -744,7 +763,7 @@ int main(int argc, char *argv[])
 								'\"', '\r'),
 			type_pdu(HFP_GW_CMD_TYPE_SET, 0),
 			data_end());
-	define_test("/hfp/test_empty", test_fragmented, NULL,
+	define_test("/hfp/test_empty", test_send_and_close, NULL,
 			raw_pdu('\r'),
 			data_end());
 	define_hf_test("/hfp_hf/test_init", test_hf_init, NULL, NULL,
-- 
1.8.4

Re: [PATCH 0/8] shared/hfp: Improvements and fixes to HFP AG parser

[Szymon Janc]

Hi Łukasz,

On Thursday 20 of November 2014 16:55:07 Lukasz Rymanowski wrote:
> This set brings couple of fixes on parsing AT commands.
> This improves robustness.
> 
> Also some minor fixes has beed done here
> 
> Lukasz Rymanowski (8):
>   shared/hfp: Trivial, fix comments style
>   shared/hfp: Fix setting result pending flag
>   shared/hfp: Improve handling AT command
>   unit/test-hfp: Add test for reading \rAT+X\r
>   shared/hfp: Refactor call_prefix_handler
>   shared/hfp: Remove not used assignment
>   shared/hfp: Improve handling AT commands
>   unit/test-hfp: Fix parsing empty string test
> 
>  src/shared/hfp.c | 123
> +++++++++++++++++++++++++++++++++++--------------------
> unit/test-hfp.c  |  41
> ++++++++++++++++++-
>  2 files changed, 117 insertions(+), 47 deletions(-)

All patches applied, thanks.

-- 
BR
Szymon Janc