[PATCH BlueZ 1/4] shared/tester: Add tester_test_abort

[PATCH BlueZ 1/4] shared/tester: Add tester_test_abort

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This can be used to abort tests and mark them as not run.
---
 src/shared/tester.c | 47 ++++++++++++++++++++++++++---------------------
 src/shared/tester.h |  1 +
 2 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/src/shared/tester.c b/src/shared/tester.c
index ffcc2ea..acd3df7 100644
--- a/src/shared/tester.c
+++ b/src/shared/tester.c
@@ -495,7 +495,7 @@ void tester_setup_failed(void)
 	test->post_teardown_func(test->test_data);
 }
 
-void tester_test_passed(void)
+static void test_result(enum test_result result)
 {
 	struct test_case *test;
 
@@ -512,33 +512,38 @@ void tester_test_passed(void)
 		test->timeout_id = 0;
 	}
 
-	test->result = TEST_RESULT_PASSED;
-	print_progress(test->name, COLOR_GREEN, "test passed");
+	test->result = result;
+	switch (result) {
+	case TEST_RESULT_PASSED:
+		print_progress(test->name, COLOR_GREEN, "test passed");
+		break;
+	case TEST_RESULT_FAILED:
+		print_progress(test->name, COLOR_RED, "test failed");
+		break;
+	case TEST_RESULT_NOT_RUN:
+		print_progress(test->name, COLOR_YELLOW, "test not run");
+		break;
+	case TEST_RESULT_TIMED_OUT:
+		print_progress(test->name, COLOR_RED, "test timed out");
+		break;
+	}
 
 	g_idle_add(teardown_callback, test);
 }
 
-void tester_test_failed(void)
+void tester_test_passed(void)
 {
-	struct test_case *test;
-
-	if (!test_current)
-		return;
-
-	test = test_current->data;
-
-	if (test->stage != TEST_STAGE_RUN)
-		return;
-
-	if (test->timeout_id > 0) {
-		g_source_remove(test->timeout_id);
-		test->timeout_id = 0;
-	}
+	test_result(TEST_RESULT_PASSED);
+}
 
-	test->result = TEST_RESULT_FAILED;
-	print_progress(test->name, COLOR_RED, "test failed");
+void tester_test_failed(void)
+{
+	test_result(TEST_RESULT_FAILED);
+}
 
-	g_idle_add(teardown_callback, test);
+void tester_test_abort(void)
+{
+	test_result(TEST_RESULT_NOT_RUN);
 }
 
 void tester_teardown_complete(void)
diff --git a/src/shared/tester.h b/src/shared/tester.h
index 0231f19..83ef5de 100644
--- a/src/shared/tester.h
+++ b/src/shared/tester.h
@@ -63,6 +63,7 @@ void tester_setup_failed(void);
 
 void tester_test_passed(void);
 void tester_test_failed(void);
+void tester_test_abort(void);
 
 void tester_teardown_complete(void);
 void tester_teardown_failed(void);
-- 
2.1.0

[PATCH BlueZ 2/4] shared/tester: Fix teardown multiple times

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

tester_test* can be called multiple times which cause teardown callback
to be called multiple times as well leading to to crashes or strange
behavior.
---
 src/shared/tester.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/shared/tester.c b/src/shared/tester.c
index acd3df7..d05bf08 100644
--- a/src/shared/tester.c
+++ b/src/shared/tester.c
@@ -90,6 +90,7 @@ struct test_case {
 	gdouble end_time;
 	unsigned int timeout;
 	unsigned int timeout_id;
+	unsigned int teardown_id;
 	tester_destroy_func_t destroy;
 	void *user_data;
 };
@@ -113,6 +114,9 @@ static void test_destroy(gpointer data)
 	if (test->timeout_id > 0)
 		g_source_remove(test->timeout_id);
 
+	if (test->teardown_id > 0)
+		g_source_remove(test->teardown_id);
+
 	if (test->destroy)
 		test->destroy(test->user_data);
 
@@ -328,6 +332,7 @@ static gboolean teardown_callback(gpointer user_data)
 {
 	struct test_case *test = user_data;
 
+	test->teardown_id = 0;
 	test->stage = TEST_STAGE_TEARDOWN;
 
 	print_progress(test->name, COLOR_MAGENTA, "teardown");
@@ -528,7 +533,10 @@ static void test_result(enum test_result result)
 		break;
 	}
 
-	g_idle_add(teardown_callback, test);
+	if (test->teardown_id > 0)
+		return;
+
+	test->teardown_id = g_idle_add(teardown_callback, test);
 }
 
 void tester_test_passed(void)
-- 
2.1.0

[PATCH BlueZ 3/4] shared/att: Add bt_att_has_crypto

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds the possibility to check if bt_att has crypto enabled.
---
 src/shared/att.c | 8 ++++++++
 src/shared/att.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/src/shared/att.c b/src/shared/att.c
index 7671d67..422cc2c 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -1426,3 +1426,11 @@ bool bt_att_set_remote_key(struct bt_att *att, uint8_t sign_key[16],
 
 	return sign_set_key(&att->remote_sign, sign_key, func, user_data);
 }
+
+bool bt_att_has_crypto(struct bt_att *att)
+{
+	if (!att)
+		return false;
+
+	return att->crypto ? true : false;
+}
diff --git a/src/shared/att.h b/src/shared/att.h
index a440aaf..fb6247e 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -90,3 +90,4 @@ bool bt_att_set_local_key(struct bt_att *att, uint8_t sign_key[16],
 			bt_att_counter_func_t func, void *user_data);
 bool bt_att_set_remote_key(struct bt_att *att, uint8_t sign_key[16],
 			bt_att_counter_func_t func, void *user_data);
+bool bt_att_has_crypto(struct bt_att *att);
-- 
2.1.0

[PATCH BlueZ 4/4] unit/test-gatt: Check if crypto is enabled

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This checks if crypto is enabled and in case it is not do not run test
/TP/GAW/CL/BV-02-C.
---
 unit/test-gatt.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 2edcacb..7668e93 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -304,9 +304,12 @@ static gboolean context_quit(gpointer user_data)
 	if (step && step->post_func)
 		step->post_func(context);
 
-	destroy_context(context);
+	if (context->data->pdu_list[context->pdu_offset].valid)
+		tester_test_abort();
+	else
+		tester_test_passed();
 
-	tester_test_passed();
+	destroy_context(context);
 
 	return FALSE;
 }
@@ -910,6 +913,11 @@ static void test_signed_write(struct context *context)
 	uint8_t key[16] = {0xD8, 0x51, 0x59, 0x48, 0x45, 0x1F, 0xEA, 0x32, 0x0D,
 				0xC0, 0x5A, 0x2E, 0x88, 0x30, 0x81, 0x88 };
 
+	if (!bt_att_has_crypto(context->att)) {
+		context_quit(context);
+		return;
+	}
+
 	g_assert(bt_att_set_local_key(context->att, key, local_counter,
 								context));
 
-- 
2.1.0

Re: [PATCH BlueZ 4/4] unit/test-gatt: Check if crypto is enabled

[Arman Uguray]

Hi Luiz,

> On Tue, Mar 10, 2015 at 6:31 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This checks if crypto is enabled and in case it is not do not run test
> /TP/GAW/CL/BV-02-C.
> ---
>  unit/test-gatt.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index 2edcacb..7668e93 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -304,9 +304,12 @@ static gboolean context_quit(gpointer user_data)
>         if (step && step->post_func)
>                 step->post_func(context);
>
> -       destroy_context(context);
> +       if (context->data->pdu_list[context->pdu_offset].valid)
> +               tester_test_abort();
> +       else
> +               tester_test_passed();
>
> -       tester_test_passed();
> +       destroy_context(context);
>
>         return FALSE;
>  }
> @@ -910,6 +913,11 @@ static void test_signed_write(struct context *context)
>         uint8_t key[16] = {0xD8, 0x51, 0x59, 0x48, 0x45, 0x1F, 0xEA, 0x32, 0x0D,
>                                 0xC0, 0x5A, 0x2E, 0x88, 0x30, 0x81, 0x88 };
>
> +       if (!bt_att_has_crypto(context->att)) {

Should we assert in this case that
bt_gatt_client_write_without_response, given true for "signed_write"
returns 0 without crashing? We would at least be validating the
current behavior, or perhaps we should add a separate test case for
it.

> +               context_quit(context);
> +               return;
> +       }
> +
>         g_assert(bt_att_set_local_key(context->att, key, local_counter,
>                                                                 context));
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks,
Arman

Re: [PATCH BlueZ 4/4] unit/test-gatt: Check if crypto is enabled

[Luiz Augusto von Dentz]

Hi Arman,

On Tue, Mar 10, 2015 at 8:07 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Luiz,
>
>> On Tue, Mar 10, 2015 at 6:31 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This checks if crypto is enabled and in case it is not do not run test
>> /TP/GAW/CL/BV-02-C.
>> ---
>>  unit/test-gatt.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>> index 2edcacb..7668e93 100644
>> --- a/unit/test-gatt.c
>> +++ b/unit/test-gatt.c
>> @@ -304,9 +304,12 @@ static gboolean context_quit(gpointer user_data)
>>         if (step && step->post_func)
>>                 step->post_func(context);
>>
>> -       destroy_context(context);
>> +       if (context->data->pdu_list[context->pdu_offset].valid)
>> +               tester_test_abort();
>> +       else
>> +               tester_test_passed();
>>
>> -       tester_test_passed();
>> +       destroy_context(context);
>>
>>         return FALSE;
>>  }
>> @@ -910,6 +913,11 @@ static void test_signed_write(struct context *context)
>>         uint8_t key[16] = {0xD8, 0x51, 0x59, 0x48, 0x45, 0x1F, 0xEA, 0x32, 0x0D,
>>                                 0xC0, 0x5A, 0x2E, 0x88, 0x30, 0x81, 0x88 };
>>
>> +       if (!bt_att_has_crypto(context->att)) {
>
> Should we assert in this case that
> bt_gatt_client_write_without_response, given true for "signed_write"
> returns 0 without crashing? We would at least be validating the
> current behavior, or perhaps we should add a separate test case for
> it.

What do you mean, note that this won't affect the test for seclevel
which test the same code except the signature since the transport is
considered secure already, all it does it check if crypto has been
enabled and the make the test no run in such case since it would
obviously fail. We might have to remove the use of assert in future
and leave tester_run return the proper result at the end.

>
>> +               context_quit(context);
>> +               return;
>> +       }
>> +
>>         g_assert(bt_att_set_local_key(context->att, key, local_counter,
>>                                                                 context));
>>
>> --
>> 2.1.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Thanks,
> Arman



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 4/4] unit/test-gatt: Check if crypto is enabled

[Marcel Holtmann]

Hi Luiz,

>>> On Tue, Mar 10, 2015 at 6:31 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> 
>>> This checks if crypto is enabled and in case it is not do not run test
>>> /TP/GAW/CL/BV-02-C.
>>> ---
>>> unit/test-gatt.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>> index 2edcacb..7668e93 100644
>>> --- a/unit/test-gatt.c
>>> +++ b/unit/test-gatt.c
>>> @@ -304,9 +304,12 @@ static gboolean context_quit(gpointer user_data)
>>>        if (step && step->post_func)
>>>                step->post_func(context);
>>> 
>>> -       destroy_context(context);
>>> +       if (context->data->pdu_list[context->pdu_offset].valid)
>>> +               tester_test_abort();
>>> +       else
>>> +               tester_test_passed();
>>> 
>>> -       tester_test_passed();
>>> +       destroy_context(context);
>>> 
>>>        return FALSE;
>>> }
>>> @@ -910,6 +913,11 @@ static void test_signed_write(struct context *context)
>>>        uint8_t key[16] = {0xD8, 0x51, 0x59, 0x48, 0x45, 0x1F, 0xEA, 0x32, 0x0D,
>>>                                0xC0, 0x5A, 0x2E, 0x88, 0x30, 0x81, 0x88 };
>>> 
>>> +       if (!bt_att_has_crypto(context->att)) {
>> 
>> Should we assert in this case that
>> bt_gatt_client_write_without_response, given true for "signed_write"
>> returns 0 without crashing? We would at least be validating the
>> current behavior, or perhaps we should add a separate test case for
>> it.
> 
> What do you mean, note that this won't affect the test for seclevel
> which test the same code except the signature since the transport is
> considered secure already, all it does it check if crypto has been
> enabled and the make the test no run in such case since it would
> obviously fail. We might have to remove the use of assert in future
> and leave tester_run return the proper result at the end.

the tester.c code is designed to run all test cases no matter what and give you a summary. Using abort is a bad idea in these cases. If you abort, the summary is pointless. It is then either all pass or abort in the middle.

Regards

Marcel

Re: [PATCH BlueZ 4/4] unit/test-gatt: Check if crypto is enabled

[Luiz Augusto von Dentz]

Hi Marcel,

On Tue, Mar 10, 2015 at 9:23 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>>>> On Tue, Mar 10, 2015 at 6:31 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>
>>>> This checks if crypto is enabled and in case it is not do not run test
>>>> /TP/GAW/CL/BV-02-C.
>>>> ---
>>>> unit/test-gatt.c | 12 ++++++++++--
>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>>> index 2edcacb..7668e93 100644
>>>> --- a/unit/test-gatt.c
>>>> +++ b/unit/test-gatt.c
>>>> @@ -304,9 +304,12 @@ static gboolean context_quit(gpointer user_data)
>>>>        if (step && step->post_func)
>>>>                step->post_func(context);
>>>>
>>>> -       destroy_context(context);
>>>> +       if (context->data->pdu_list[context->pdu_offset].valid)
>>>> +               tester_test_abort();
>>>> +       else
>>>> +               tester_test_passed();
>>>>
>>>> -       tester_test_passed();
>>>> +       destroy_context(context);
>>>>
>>>>        return FALSE;
>>>> }
>>>> @@ -910,6 +913,11 @@ static void test_signed_write(struct context *context)
>>>>        uint8_t key[16] = {0xD8, 0x51, 0x59, 0x48, 0x45, 0x1F, 0xEA, 0x32, 0x0D,
>>>>                                0xC0, 0x5A, 0x2E, 0x88, 0x30, 0x81, 0x88 };
>>>>
>>>> +       if (!bt_att_has_crypto(context->att)) {
>>>
>>> Should we assert in this case that
>>> bt_gatt_client_write_without_response, given true for "signed_write"
>>> returns 0 without crashing? We would at least be validating the
>>> current behavior, or perhaps we should add a separate test case for
>>> it.
>>
>> What do you mean, note that this won't affect the test for seclevel
>> which test the same code except the signature since the transport is
>> considered secure already, all it does it check if crypto has been
>> enabled and the make the test no run in such case since it would
>> obviously fail. We might have to remove the use of assert in future
>> and leave tester_run return the proper result at the end.
>
> the tester.c code is designed to run all test cases no matter what and give you a summary. Using abort is a bad idea in these cases. If you abort, the summary is pointless. It is then either all pass or abort in the middle.

Yep, we should proceed run all tests, but I did not bother changing
this yet since I want to fix the build for platforms without crypto
first.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/4] shared/tester: Add tester_test_abort

[Luiz Augusto von Dentz]

Hi,

On Tue, Mar 10, 2015 at 3:31 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This can be used to abort tests and mark them as not run.
> ---
>  src/shared/tester.c | 47 ++++++++++++++++++++++++++---------------------
>  src/shared/tester.h |  1 +
>  2 files changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/src/shared/tester.c b/src/shared/tester.c
> index ffcc2ea..acd3df7 100644
> --- a/src/shared/tester.c
> +++ b/src/shared/tester.c
> @@ -495,7 +495,7 @@ void tester_setup_failed(void)
>         test->post_teardown_func(test->test_data);
>  }
>
> -void tester_test_passed(void)
> +static void test_result(enum test_result result)
>  {
>         struct test_case *test;
>
> @@ -512,33 +512,38 @@ void tester_test_passed(void)
>                 test->timeout_id = 0;
>         }
>
> -       test->result = TEST_RESULT_PASSED;
> -       print_progress(test->name, COLOR_GREEN, "test passed");
> +       test->result = result;
> +       switch (result) {
> +       case TEST_RESULT_PASSED:
> +               print_progress(test->name, COLOR_GREEN, "test passed");
> +               break;
> +       case TEST_RESULT_FAILED:
> +               print_progress(test->name, COLOR_RED, "test failed");
> +               break;
> +       case TEST_RESULT_NOT_RUN:
> +               print_progress(test->name, COLOR_YELLOW, "test not run");
> +               break;
> +       case TEST_RESULT_TIMED_OUT:
> +               print_progress(test->name, COLOR_RED, "test timed out");
> +               break;
> +       }
>
>         g_idle_add(teardown_callback, test);
>  }
>
> -void tester_test_failed(void)
> +void tester_test_passed(void)
>  {
> -       struct test_case *test;
> -
> -       if (!test_current)
> -               return;
> -
> -       test = test_current->data;
> -
> -       if (test->stage != TEST_STAGE_RUN)
> -               return;
> -
> -       if (test->timeout_id > 0) {
> -               g_source_remove(test->timeout_id);
> -               test->timeout_id = 0;
> -       }
> +       test_result(TEST_RESULT_PASSED);
> +}
>
> -       test->result = TEST_RESULT_FAILED;
> -       print_progress(test->name, COLOR_RED, "test failed");
> +void tester_test_failed(void)
> +{
> +       test_result(TEST_RESULT_FAILED);
> +}
>
> -       g_idle_add(teardown_callback, test);
> +void tester_test_abort(void)
> +{
> +       test_result(TEST_RESULT_NOT_RUN);
>  }
>
>  void tester_teardown_complete(void)
> diff --git a/src/shared/tester.h b/src/shared/tester.h
> index 0231f19..83ef5de 100644
> --- a/src/shared/tester.h
> +++ b/src/shared/tester.h
> @@ -63,6 +63,7 @@ void tester_setup_failed(void);
>
>  void tester_test_passed(void);
>  void tester_test_failed(void);
> +void tester_test_abort(void);
>
>  void tester_teardown_complete(void);
>  void tester_teardown_failed(void);
> --
> 2.1.0

Applied.


-- 
Luiz Augusto von Dentz