[PATCH 01/11] android/unit: Fix checking for expected termination

[PATCH 01/11] android/unit: Fix checking for expected termination

[Jakub Tyszkowski]

This fix makes sure that when signalled termination is expected,
it actually happens. If IPC termination is expected no response will be
sent, so cmd_watch will never be executed. But if it is executed when
expecting termination, its a failure.
---
 android/test-ipc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index 8af5739..d0f3f6b 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -82,6 +82,8 @@ static gboolean cmd_watch(GIOChannel *io, GIOCondition cond,
 	uint8_t buf[128];
 	int sk;
 
+	g_assert(test_data->expected_signal == 0);
+
 	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) {
 		g_assert(FALSE);
 		return FALSE;
-- 
1.8.5.2

[PATCH 02/11] android/unit: Rename cmd handler

[Jakub Tyszkowski]

This handler responses for opcode == 1, thus should use proper naming to
avoid confision when more functions sending different responses will be
added.
---
 android/test-ipc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index d0f3f6b..8e31507 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -363,7 +363,7 @@ static void test_cmd_reg_1(gconstpointer data)
 	ipc_cleanup();
 }
 
-static void test_cmd_handler(const void *buf, uint16_t len)
+static void test_cmd_handler_1(const void *buf, uint16_t len)
 {
 	ipc_send_rsp(0, 1, 0);
 }
@@ -383,7 +383,7 @@ static const struct test_data test_cmd_service_invalid_1 = {
 };
 
 static const struct ipc_handler cmd_handlers[] = {
-	{ test_cmd_handler, false, 0 }
+	{ test_cmd_handler_1, false, 0 }
 };
 
 static const struct test_data test_cmd_service_valid_1 = {
-- 
1.8.5.2

[PATCH 03/11] android/unit: Add test cases for proper handler calls

[Jakub Tyszkowski]

This patch adds tests for calling proper opcode handler. Two handlers
are registered, but one always results in failure. No failure means that
proper opcode <-> handler maching is done by the ipc mechanism.
---
 android/test-ipc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index 8e31507..1e027fa 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -368,6 +368,16 @@ static void test_cmd_handler_1(const void *buf, uint16_t len)
 	ipc_send_rsp(0, 1, 0);
 }
 
+static void test_cmd_handler_2(const void *buf, uint16_t len)
+{
+	ipc_send_rsp(0, 2, 0);
+}
+
+static void test_cmd_handler_invalid(const void *buf, uint16_t len)
+{
+	raise(SIGTERM);
+}
+
 static const struct test_data test_init_1 = {};
 
 static const struct hal_hdr test_cmd_1_hdr = {
@@ -376,6 +386,12 @@ static const struct hal_hdr test_cmd_1_hdr = {
 	.len = 0
 };
 
+static const struct hal_hdr test_cmd_2_hdr = {
+	.service_id = 0,
+	.opcode = 2,
+	.len = 0
+};
+
 static const struct test_data test_cmd_service_invalid_1 = {
 	.cmd = &test_cmd_1_hdr,
 	.cmd_size = sizeof(test_cmd_1_hdr),
@@ -403,6 +419,32 @@ static const struct test_data test_cmd_service_invalid_2 = {
 	.expected_signal = SIGTERM
 };
 
+static const struct ipc_handler cmd_handlers_invalid_2[] = {
+	{ test_cmd_handler_1, false, 0 },
+	{ test_cmd_handler_invalid, false, 0 }
+};
+
+static const struct ipc_handler cmd_handlers_invalid_1[] = {
+	{ test_cmd_handler_invalid, false, 0 },
+	{ test_cmd_handler_2, false, 0 },
+};
+
+static const struct test_data test_cmd_opcode_valid_1 = {
+	.cmd = &test_cmd_1_hdr,
+	.cmd_size = sizeof(test_cmd_1_hdr),
+	.service = 0,
+	.handlers = cmd_handlers_invalid_2,
+	.handlers_size = 2,
+};
+
+static const struct test_data test_cmd_opcode_valid_2 = {
+	.cmd = &test_cmd_2_hdr,
+	.cmd_size = sizeof(test_cmd_2_hdr),
+	.service = 0,
+	.handlers = cmd_handlers_invalid_1,
+	.handlers_size = 2,
+};
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -417,6 +459,10 @@ int main(int argc, char *argv[])
 					&test_cmd_service_valid_1, test_cmd_reg);
 	g_test_add_data_func("/android_ipc/test_cmd_service_invalid_2",
 					&test_cmd_service_invalid_2, test_cmd_reg_1);
+	g_test_add_data_func("/android_ipc/test_cmd_opcode_valid_1",
+					&test_cmd_opcode_valid_1, test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_opcode_valid_2",
+					&test_cmd_opcode_valid_2, test_cmd_reg);
 
 	return g_test_run();
 }
-- 
1.8.5.2

[PATCH 04/11] android/unit: Add case for opcode without handler

[Jakub Tyszkowski]

This test case checks if IPC shuts down on unhandled opcode.
---
 android/test-ipc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index 1e027fa..cb6f518 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -445,6 +445,15 @@ static const struct test_data test_cmd_opcode_valid_2 = {
 	.handlers_size = 2,
 };
 
+static const struct test_data test_cmd_opcode_invalid_1 = {
+	.cmd = &test_cmd_2_hdr,
+	.cmd_size = sizeof(test_cmd_2_hdr),
+	.service = 0,
+	.handlers = cmd_handlers,
+	.handlers_size = 1,
+	.expected_signal = SIGTERM
+};
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -463,6 +472,9 @@ int main(int argc, char *argv[])
 					&test_cmd_opcode_valid_1, test_cmd_reg);
 	g_test_add_data_func("/android_ipc/test_cmd_opcode_valid_2",
 					&test_cmd_opcode_valid_2, test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_opcode_invalid_1",
+					&test_cmd_opcode_invalid_1,
+					test_cmd_reg);
 
 	return g_test_run();
 }
-- 
1.8.5.2

[PATCH 05/11] android/unit: Add support for variable length data

[Jakub Tyszkowski]

This patch adds sending messages larger than just hal_hdr, and fixes
response verification which worked only for empty messages but was
failing when sending something more than just header.
---
 android/test-ipc.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index cb6f518..a2d3085 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -44,7 +44,7 @@
 
 struct test_data {
 	uint32_t expected_signal;
-	const struct hal_hdr *cmd;
+	const void *cmd;
 	uint16_t cmd_size;
 	uint8_t service;
 	const struct ipc_handler *handlers;
@@ -79,9 +79,16 @@ static gboolean cmd_watch(GIOChannel *io, GIOCondition cond,
 {
 	struct context *context = user_data;
 	const struct test_data *test_data = context->data;
+	const struct hal_hdr *sent_msg = test_data->cmd;
 	uint8_t buf[128];
 	int sk;
 
+	struct hal_hdr success_resp = {
+		.service_id = sent_msg->service_id,
+		.opcode = sent_msg->opcode,
+		.len = 0,
+	};
+
 	g_assert(test_data->expected_signal == 0);
 
 	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) {
@@ -91,8 +98,8 @@ static gboolean cmd_watch(GIOChannel *io, GIOCondition cond,
 
 	sk = g_io_channel_unix_get_fd(io);
 
-	g_assert(read(sk, buf, sizeof(buf)) == test_data->cmd_size);
-	g_assert(!memcmp(test_data->cmd, buf, test_data->cmd_size));
+	g_assert(read(sk, buf, sizeof(buf)) == sizeof(struct hal_hdr));
+	g_assert(!memcmp(&success_resp, buf, sizeof(struct hal_hdr)));
 
 	context_quit(context);
 
-- 
1.8.5.2

[PATCH 06/11] android/unit: Add test case for variable sized data

[Jakub Tyszkowski]

Check if sending variable length data with proper msg size declared inside the
header succeeds.
---
 android/test-ipc.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index a2d3085..523011e 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -461,6 +461,32 @@ static const struct test_data test_cmd_opcode_invalid_1 = {
 	.expected_signal = SIGTERM
 };
 
+#define VARDATA_EX1 "some data example"
+
+struct vardata {
+	struct hal_hdr hdr;
+	uint8_t data[BLUEZ_HAL_MTU - sizeof(struct hal_hdr)];
+} __attribute__((packed));
+
+static const struct vardata test_cmd_vardata = {
+	.hdr.service_id = 0,
+	.hdr.opcode = 1,
+	.hdr.len = sizeof(VARDATA_EX1),
+	.data = VARDATA_EX1,
+};
+
+static const struct ipc_handler cmd_vardata_handlers[] = {
+	{ test_cmd_handler_1, true, sizeof(VARDATA_EX1) }
+};
+
+static const struct test_data test_cmd_vardata_valid = {
+	.cmd = &test_cmd_vardata,
+	.cmd_size = sizeof(struct hal_hdr) + sizeof(VARDATA_EX1),
+	.service = 0,
+	.handlers = cmd_vardata_handlers,
+	.handlers_size = 1,
+};
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -482,6 +508,9 @@ int main(int argc, char *argv[])
 	g_test_add_data_func("/android_ipc/test_cmd_opcode_invalid_1",
 					&test_cmd_opcode_invalid_1,
 					test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_vardata_valid",
+					&test_cmd_vardata_valid,
+					test_cmd_reg);
 
 	return g_test_run();
 }
-- 
1.8.5.2

[PATCH 07/11] android/unit: Add another case for variable sized data

[Jakub Tyszkowski]

This patch adds test for variable length data handling. Handlers struct
have static values representing minimum payload. It cannot be predicted
how large data will be sent so they should accept data larger than
declared inside ipc_handler array, which holds the minimum size of such
message.
---
 android/test-ipc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index 523011e..2120d15 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -487,6 +487,18 @@ static const struct test_data test_cmd_vardata_valid = {
 	.handlers_size = 1,
 };
 
+static const struct ipc_handler cmd_vardata_handlers_valid2[] = {
+	{ test_cmd_handler_1, true, sizeof(VARDATA_EX1) - 1 }
+};
+
+static const struct test_data test_cmd_vardata_valid_2 = {
+	.cmd = &test_cmd_vardata,
+	.cmd_size = sizeof(struct hal_hdr) + sizeof(VARDATA_EX1),
+	.service = 0,
+	.handlers = cmd_vardata_handlers_valid2,
+	.handlers_size = 1,
+};
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -511,6 +523,9 @@ int main(int argc, char *argv[])
 	g_test_add_data_func("/android_ipc/test_cmd_vardata_valid",
 					&test_cmd_vardata_valid,
 					test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_vardata_valid_2",
+					&test_cmd_vardata_valid_2,
+					test_cmd_reg);
 
 	return g_test_run();
 }
-- 
1.8.5.2

[PATCH 08/11] android/unit: Add case for out of range service

[Jakub Tyszkowski]

Check reaction for sending message to services not listed inside
hal-msg.h
---
 android/test-ipc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index 2120d15..6bb6cd6 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -499,6 +499,21 @@ static const struct test_data test_cmd_vardata_valid_2 = {
 	.handlers_size = 1,
 };
 
+static const struct hal_hdr test_cmd_service_offrange_hdr = {
+	.service_id = HAL_SERVICE_ID_MAX + 1,
+	.opcode = 1,
+	.len = 0
+};
+
+static const struct test_data test_cmd_service_offrange = {
+	.cmd = &test_cmd_service_offrange_hdr,
+	.cmd_size = sizeof(struct hal_hdr),
+	.service = HAL_SERVICE_ID_MAX + 1,
+	.handlers = cmd_handlers,
+	.handlers_size = 1,
+	.expected_signal = SIGTERM
+};
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -526,6 +541,9 @@ int main(int argc, char *argv[])
 	g_test_add_data_func("/android_ipc/test_cmd_vardata_valid_2",
 					&test_cmd_vardata_valid_2,
 					test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_service_offrange",
+					&test_cmd_service_offrange,
+					test_cmd_reg);
 
 	return g_test_run();
 }
-- 
1.8.5.2

[PATCH 09/11] android/unit: Add negative case for msg size verification

[Jakub Tyszkowski]

Case for checking message size declared inside the header against the
amount of data sent for variable sized message.
---
 android/test-ipc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index 6bb6cd6..e4463eb 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -499,6 +499,15 @@ static const struct test_data test_cmd_vardata_valid_2 = {
 	.handlers_size = 1,
 };
 
+static const struct test_data test_cmd_vardata_invalid_1 = {
+	.cmd = &test_cmd_vardata,
+	.cmd_size = sizeof(struct hal_hdr) + sizeof(VARDATA_EX1) - 1,
+	.service = 0,
+	.handlers = cmd_vardata_handlers,
+	.handlers_size = 1,
+	.expected_signal = SIGTERM
+};
+
 static const struct hal_hdr test_cmd_service_offrange_hdr = {
 	.service_id = HAL_SERVICE_ID_MAX + 1,
 	.opcode = 1,
@@ -541,6 +550,9 @@ int main(int argc, char *argv[])
 	g_test_add_data_func("/android_ipc/test_cmd_vardata_valid_2",
 					&test_cmd_vardata_valid_2,
 					test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_vardata_invalid_1",
+					&test_cmd_vardata_invalid_1,
+					test_cmd_reg);
 	g_test_add_data_func("/android_ipc/test_cmd_service_offrange",
 					&test_cmd_service_offrange,
 					test_cmd_reg);
-- 
1.8.5.2

[PATCH 10/11] android/unit: Add case for sending incomplete header

[Jakub Tyszkowski]

Header size is the bare minimum that should always be sent.
---
 android/test-ipc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index e4463eb..d05544d 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -461,6 +461,15 @@ static const struct test_data test_cmd_opcode_invalid_1 = {
 	.expected_signal = SIGTERM
 };
 
+static const struct test_data test_cmd_hdr_invalid = {
+	.cmd = &test_cmd_1_hdr,
+	.cmd_size = sizeof(test_cmd_1_hdr) - 1,
+	.service = 0,
+	.handlers = cmd_handlers,
+	.handlers_size = 1,
+	.expected_signal = SIGTERM
+};
+
 #define VARDATA_EX1 "some data example"
 
 struct vardata {
@@ -556,6 +565,9 @@ int main(int argc, char *argv[])
 	g_test_add_data_func("/android_ipc/test_cmd_service_offrange",
 					&test_cmd_service_offrange,
 					test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_hdr_invalid",
+					&test_cmd_hdr_invalid,
+					test_cmd_reg);
 
 	return g_test_run();
 }
-- 
1.8.5.2

[PATCH 11/11] android/unit: Add cases for msg size verification

[Jakub Tyszkowski]

This patch adds checking for proper msg size verification in case it is
not declared in handlers that this is variable sized message. In
such case malformed data should not be accepted.
---
 android/test-ipc.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index d05544d..a276063 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -532,6 +532,38 @@ static const struct test_data test_cmd_service_offrange = {
 	.expected_signal = SIGTERM
 };
 
+static const struct vardata test_cmd_invalid_data_1 = {
+	.hdr.service_id = 0,
+	.hdr.opcode = 1,
+	.hdr.len = sizeof(VARDATA_EX1),
+	.data = VARDATA_EX1,
+};
+
+static const struct test_data test_cmd_msg_invalid_1 = {
+	.cmd = &test_cmd_invalid_data_1,
+	.cmd_size = sizeof(struct hal_hdr) + sizeof(VARDATA_EX1) - 1,
+	.service = 0,
+	.handlers = cmd_handlers,
+	.handlers_size = 1,
+	.expected_signal = SIGTERM
+};
+
+static const struct vardata test_cmd_invalid_data_2 = {
+	.hdr.service_id = 0,
+	.hdr.opcode = 1,
+	.hdr.len = sizeof(VARDATA_EX1) - 1,
+	.data = VARDATA_EX1,
+};
+
+static const struct test_data test_cmd_msg_invalid_2 = {
+	.cmd = &test_cmd_invalid_data_2,
+	.cmd_size = sizeof(struct hal_hdr) + sizeof(VARDATA_EX1),
+	.service = 0,
+	.handlers = cmd_handlers,
+	.handlers_size = 1,
+	.expected_signal = SIGTERM
+};
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -568,6 +600,12 @@ int main(int argc, char *argv[])
 	g_test_add_data_func("/android_ipc/test_cmd_hdr_invalid",
 					&test_cmd_hdr_invalid,
 					test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_msg_invalid_1",
+					&test_cmd_msg_invalid_1,
+					test_cmd_reg);
+	g_test_add_data_func("/android_ipc/test_cmd_msg_invalid_2",
+					&test_cmd_msg_invalid_2,
+					test_cmd_reg);
 
 	return g_test_run();
 }
-- 
1.8.5.2

Re: [PATCH 01/11] android/unit: Fix checking for expected termination

[Szymon Janc]

Hi Jakub,

On Tuesday 04 February 2014 15:38:59 Jakub Tyszkowski wrote:
> This fix makes sure that when signalled termination is expected,
> it actually happens. If IPC termination is expected no response will be
> sent, so cmd_watch will never be executed. But if it is executed when
> expecting termination, its a failure.
> ---
>  android/test-ipc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/android/test-ipc.c b/android/test-ipc.c
> index 8af5739..d0f3f6b 100644
> --- a/android/test-ipc.c
> +++ b/android/test-ipc.c
> @@ -82,6 +82,8 @@ static gboolean cmd_watch(GIOChannel *io, GIOCondition
> cond, uint8_t buf[128];
>  	int sk;
> 
> +	g_assert(test_data->expected_signal == 0);
> +
>  	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) {
>  		g_assert(FALSE);
>  		return FALSE;

All patches are now upstream, thanks.

-- 
Szymon K. Janc
szymon.janc@gmail.com