2014-10-28 19:37:41

by Marcin Kraglak

[permalink] [raw]
Subject: [PATCHv2 1/7] shared/io: Remove disconnect handler in io_destroy

Diconnect callback may be called after destroying IO in next
mainloop iteration (it leada to reading freed data). Removing
disconnect handler prevents that situation.
---
src/shared/io-glib.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
index a2ada66..5ebde3d 100644
--- a/src/shared/io-glib.c
+++ b/src/shared/io-glib.c
@@ -115,6 +115,11 @@ void io_destroy(struct io *io)
io->write_watch = 0;
}

+ if (io->disconnect_watch > 0) {
+ g_source_remove(io->disconnect_watch);
+ io->disconnect_watch = 0;
+ }
+
g_io_channel_unref(io->channel);
io->channel = NULL;

--
1.9.3



2014-10-31 14:03:05

by Marcin Kraglak

[permalink] [raw]
Subject: Re: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case

Hi Luiz,

On 30 October 2014 16:41, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Marcin,
>
> On Thu, Oct 30, 2014 at 8:26 AM, Marcin Kraglak
> <[email protected]> wrote:
>> Hi Luiz, Arman
>>
>> On 29 October 2014 17:11, Arman Uguray <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>> On Wed, Oct 29, 2014 at 8:25 AM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> Hi Michael,
>>>>
>>>> On Wed, Oct 29, 2014 at 5:03 PM, Michael Janssen <[email protected]> wrote:
>>>>> On Wed, Oct 29, 2014 at 5:32 AM, Luiz Augusto von Dentz
>>>>> <[email protected]> wrote:
>>>>>> Hi Marcin,
>>>>>>
>>>>>> On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak
>>>>>> <[email protected]> wrote:
>>>>>>> ---
>>>>>>> unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>>>>>>> 1 file changed, 77 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>>>>>> index bbbf9a5..f84b0fc 100644
>>>>>>> --- a/unit/test-gatt.c
>>>>>>> +++ b/unit/test-gatt.c
>>>>>>> @@ -35,8 +35,10 @@
>>>>>>>
>>>>>>> #include <glib.h>
>>>>>>>
>>>>>>> +#include "lib/uuid.h"
>>>>>>> #include "src/shared/util.h"
>>>>>>> #include "src/shared/att.h"
>>>>>>> +#include "src/shared/gatt-helpers.h"
>>>>>>> #include "src/shared/gatt-client.h"
>>>>>>>
>>>>>>> struct test_pdu {
>>>>>>> @@ -45,14 +47,22 @@ struct test_pdu {
>>>>>>> size_t size;
>>>>>>> };
>>>>>>>
>>>>>>> +enum context_type {
>>>>>>> + ATT,
>>>>>>> + CLIENT,
>>>>>>> + SERVER
>>>>>>> +};
>>>>>>
>>>>>> Well the ATT part of this is a bit weird, do we really need to do this
>>>>>> distinction.
>>>>>>
>>>>>>> struct test_data {
>>>>>>> char *test_name;
>>>>>>> struct test_pdu *pdu_list;
>>>>>>> + enum context_type context_type;
>>>>>>> };
>>>>>>>
>>>>>>> struct context {
>>>>>>> GMainLoop *main_loop;
>>>>>>> struct bt_gatt_client *client;
>>>>>>> + struct bt_att *att;
>>>>>>> guint source;
>>>>>>> guint process;
>>>>>>> int fd;
>>>>>>> @@ -69,13 +79,14 @@ struct context {
>>>>>>> .size = sizeof(data(args)), \
>>>>>>> }
>>>>>>>
>>>>>>> -#define define_test(name, function, args...) \
>>>>>>> +#define define_test(name, function, type, args...) \
>>>>>>> do { \
>>>>>>> const struct test_pdu pdus[] = { \
>>>>>>> args, { } \
>>>>>>> }; \
>>>>>>> static struct test_data data; \
>>>>>>> data.test_name = g_strdup(name); \
>>>>>>> + data.context_type = type; \
>>>>>>> data.pdu_list = g_malloc(sizeof(pdus)); \
>>>>>>> memcpy(data.pdu_list, pdus, sizeof(pdus)); \
>>>>>>> g_test_add_data_func(name, &data, function); \
>>>>>>> @@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data)
>>>>>>> static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>>>> {
>>>>>>> struct context *context = g_new0(struct context, 1);
>>>>>>> + const struct test_data *test_data = data;
>>>>>>> GIOChannel *channel;
>>>>>>> int err, sv[2];
>>>>>>> struct bt_att *att;
>>>>>>> @@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>>>> att = bt_att_new(sv[0]);
>>>>>>> g_assert(att);
>>>>>>>
>>>>>>> - context->client = bt_gatt_client_new(att, mtu);
>>>>>>> - g_assert(context->client);
>>>>>>> + switch (test_data->context_type) {
>>>>>>> + case ATT:
>>>>>>> + context->att = att;
>>>>>>>
>>>>>>> - if (g_test_verbose())
>>>>>>> - bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
>>>>>>> - NULL);
>>>>>>> + bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL);
>>>>>>> + break;
>>>>>>> + case CLIENT:
>>>>>>> + context->client = bt_gatt_client_new(att, mtu);
>>>>>>> + g_assert(context->client);
>>>>>>> +
>>>>>>> + if (g_test_verbose())
>>>>>>> + bt_gatt_client_set_debug(context->client, gatt_debug,
>>>>>>> + "gatt:", NULL);
>>>>>>> +
>>>>>>> + bt_att_unref(att);
>>>>>>> + break;
>>>>>>> + default:
>>>>>>> + break;
>>>>>>> + }
>>>>>>>
>>>>>>> channel = g_io_channel_unix_new(sv[1]);
>>>>>>>
>>>>>>> @@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>>>> g_assert(context->source > 0);
>>>>>>>
>>>>>>> g_io_channel_unref(channel);
>>>>>>> - bt_att_unref(att);
>>>>>>>
>>>>>>> context->fd = sv[1];
>>>>>>> context->data = data;
>>>>>>> @@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>>>> return context;
>>>>>>> }
>>>>>>>
>>>>>>> +static void primary_cb(bool success, uint8_t att_ecode,
>>>>>>> + struct bt_gatt_result *result,
>>>>>>> + void *user_data)
>>>>>>> +{
>>>>>>> + struct context *context = user_data;
>>>>>>> +
>>>>>>> + g_assert(success);
>>>>>>> +
>>>>>>> + context_quit(context);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void destroy_context(struct context *context)
>>>>>>> {
>>>>>>> if (context->source > 0)
>>>>>>> @@ -229,6 +264,9 @@ static void destroy_context(struct context *context)
>>>>>>>
>>>>>>> bt_gatt_client_unref(context->client);
>>>>>>>
>>>>>>> + if (context->att)
>>>>>>> + bt_att_unref(context->att);
>>>>>>> +
>>>>>>> g_main_loop_unref(context->main_loop);
>>>>>>>
>>>>>>> test_free(context->data);
>>>>>>> @@ -249,6 +287,16 @@ static void test_client(gconstpointer data)
>>>>>>> execute_context(context);
>>>>>>> }
>>>>>>>
>>>>>>> +static void test_search_primary(gconstpointer data)
>>>>>>> +{
>>>>>>> + struct context *context = create_context(512, data);
>>>>>>> +
>>>>>>> + bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
>>>>>>> + context, NULL);
>>>>>>
>>>>>> Doesn't bt_gatt_client does the same here, I guess I missed this
>>>>>> detail in the first time but if this is just testing client side then
>>>>>> it should use bt_gatt_client and if that cannot be tested we probably
>>>>>> have a problem with our API. I guess PTS would just pass if we do more
>>>>>> than search the primary services, so things like setting the MTU etc
>>>>>> should not be a problem here.
>>>>>
>>>>> Specifically setting the MTU is allowed, but the reason to avoid using
>>>>> bt_gatt_client_new/init here is to avoid the automatic service
>>>>> discovery. This is probably a good idea at least for testing the
>>>>> different types of discovery. (is bt_gatt_client Alternately maybe
>>>>> just test the type of discovery used by bt_gatt_client. When
>>>>> implementing the read value tests, the auto-discovery of
>>>>> bt_gatt_client may make the test data longer because it needs to be
>>>>> included in every one.
>>>>
>>>> Isn't it what we want? To run exactly as it would be run against PTS
>>>> so we avoid surprises? Not only this avoid auto-discovery but the
>>>> caching done by bt_gatt_client which we would probably have to test
>>>> separately, so it is much more code that we exercise if we test
>>>> bt_gatt_client at the expense of a little bit more PDUs. Perhaps we
>>>> could have a define containing the common PDUs that auto-discovery
>>>> causes so the upcoming tests don't need to duplicate these PDUs.
>>>>
>>
>> I agree we test much more code while testing gatt-client. My idea was
>> to use bt_att in GAD group,
>> because we need to send specified packets and it is more like testing
>> protocol than profile.
>> Whole discovery using gatt-client will be tested in most of GAR and
>> GAW test cases.
>> I'm sure that it will be bigger part of unit tests.
>
> Fair enough, but please add the test description to the commits so we
> are consistent, for example:
>
> unit/test-gatt: Add <test name> test
>
> Verify that a Generic Attribute Profile client discovers Primary
> Services in a GATT server.
I've added descriptions to commit messages

BR
Marcin

2014-10-30 15:41:03

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case

Hi Marcin,

On Thu, Oct 30, 2014 at 8:26 AM, Marcin Kraglak
<[email protected]> wrote:
> Hi Luiz, Arman
>
> On 29 October 2014 17:11, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Wed, Oct 29, 2014 at 8:25 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> Hi Michael,
>>>
>>> On Wed, Oct 29, 2014 at 5:03 PM, Michael Janssen <[email protected]> wrote:
>>>> On Wed, Oct 29, 2014 at 5:32 AM, Luiz Augusto von Dentz
>>>> <[email protected]> wrote:
>>>>> Hi Marcin,
>>>>>
>>>>> On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak
>>>>> <[email protected]> wrote:
>>>>>> ---
>>>>>> unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>>>>>> 1 file changed, 77 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>>>>> index bbbf9a5..f84b0fc 100644
>>>>>> --- a/unit/test-gatt.c
>>>>>> +++ b/unit/test-gatt.c
>>>>>> @@ -35,8 +35,10 @@
>>>>>>
>>>>>> #include <glib.h>
>>>>>>
>>>>>> +#include "lib/uuid.h"
>>>>>> #include "src/shared/util.h"
>>>>>> #include "src/shared/att.h"
>>>>>> +#include "src/shared/gatt-helpers.h"
>>>>>> #include "src/shared/gatt-client.h"
>>>>>>
>>>>>> struct test_pdu {
>>>>>> @@ -45,14 +47,22 @@ struct test_pdu {
>>>>>> size_t size;
>>>>>> };
>>>>>>
>>>>>> +enum context_type {
>>>>>> + ATT,
>>>>>> + CLIENT,
>>>>>> + SERVER
>>>>>> +};
>>>>>
>>>>> Well the ATT part of this is a bit weird, do we really need to do this
>>>>> distinction.
>>>>>
>>>>>> struct test_data {
>>>>>> char *test_name;
>>>>>> struct test_pdu *pdu_list;
>>>>>> + enum context_type context_type;
>>>>>> };
>>>>>>
>>>>>> struct context {
>>>>>> GMainLoop *main_loop;
>>>>>> struct bt_gatt_client *client;
>>>>>> + struct bt_att *att;
>>>>>> guint source;
>>>>>> guint process;
>>>>>> int fd;
>>>>>> @@ -69,13 +79,14 @@ struct context {
>>>>>> .size = sizeof(data(args)), \
>>>>>> }
>>>>>>
>>>>>> -#define define_test(name, function, args...) \
>>>>>> +#define define_test(name, function, type, args...) \
>>>>>> do { \
>>>>>> const struct test_pdu pdus[] = { \
>>>>>> args, { } \
>>>>>> }; \
>>>>>> static struct test_data data; \
>>>>>> data.test_name = g_strdup(name); \
>>>>>> + data.context_type = type; \
>>>>>> data.pdu_list = g_malloc(sizeof(pdus)); \
>>>>>> memcpy(data.pdu_list, pdus, sizeof(pdus)); \
>>>>>> g_test_add_data_func(name, &data, function); \
>>>>>> @@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data)
>>>>>> static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>>> {
>>>>>> struct context *context = g_new0(struct context, 1);
>>>>>> + const struct test_data *test_data = data;
>>>>>> GIOChannel *channel;
>>>>>> int err, sv[2];
>>>>>> struct bt_att *att;
>>>>>> @@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>>> att = bt_att_new(sv[0]);
>>>>>> g_assert(att);
>>>>>>
>>>>>> - context->client = bt_gatt_client_new(att, mtu);
>>>>>> - g_assert(context->client);
>>>>>> + switch (test_data->context_type) {
>>>>>> + case ATT:
>>>>>> + context->att = att;
>>>>>>
>>>>>> - if (g_test_verbose())
>>>>>> - bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
>>>>>> - NULL);
>>>>>> + bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL);
>>>>>> + break;
>>>>>> + case CLIENT:
>>>>>> + context->client = bt_gatt_client_new(att, mtu);
>>>>>> + g_assert(context->client);
>>>>>> +
>>>>>> + if (g_test_verbose())
>>>>>> + bt_gatt_client_set_debug(context->client, gatt_debug,
>>>>>> + "gatt:", NULL);
>>>>>> +
>>>>>> + bt_att_unref(att);
>>>>>> + break;
>>>>>> + default:
>>>>>> + break;
>>>>>> + }
>>>>>>
>>>>>> channel = g_io_channel_unix_new(sv[1]);
>>>>>>
>>>>>> @@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>>> g_assert(context->source > 0);
>>>>>>
>>>>>> g_io_channel_unref(channel);
>>>>>> - bt_att_unref(att);
>>>>>>
>>>>>> context->fd = sv[1];
>>>>>> context->data = data;
>>>>>> @@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>>> return context;
>>>>>> }
>>>>>>
>>>>>> +static void primary_cb(bool success, uint8_t att_ecode,
>>>>>> + struct bt_gatt_result *result,
>>>>>> + void *user_data)
>>>>>> +{
>>>>>> + struct context *context = user_data;
>>>>>> +
>>>>>> + g_assert(success);
>>>>>> +
>>>>>> + context_quit(context);
>>>>>> +}
>>>>>> +
>>>>>> static void destroy_context(struct context *context)
>>>>>> {
>>>>>> if (context->source > 0)
>>>>>> @@ -229,6 +264,9 @@ static void destroy_context(struct context *context)
>>>>>>
>>>>>> bt_gatt_client_unref(context->client);
>>>>>>
>>>>>> + if (context->att)
>>>>>> + bt_att_unref(context->att);
>>>>>> +
>>>>>> g_main_loop_unref(context->main_loop);
>>>>>>
>>>>>> test_free(context->data);
>>>>>> @@ -249,6 +287,16 @@ static void test_client(gconstpointer data)
>>>>>> execute_context(context);
>>>>>> }
>>>>>>
>>>>>> +static void test_search_primary(gconstpointer data)
>>>>>> +{
>>>>>> + struct context *context = create_context(512, data);
>>>>>> +
>>>>>> + bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
>>>>>> + context, NULL);
>>>>>
>>>>> Doesn't bt_gatt_client does the same here, I guess I missed this
>>>>> detail in the first time but if this is just testing client side then
>>>>> it should use bt_gatt_client and if that cannot be tested we probably
>>>>> have a problem with our API. I guess PTS would just pass if we do more
>>>>> than search the primary services, so things like setting the MTU etc
>>>>> should not be a problem here.
>>>>
>>>> Specifically setting the MTU is allowed, but the reason to avoid using
>>>> bt_gatt_client_new/init here is to avoid the automatic service
>>>> discovery. This is probably a good idea at least for testing the
>>>> different types of discovery. (is bt_gatt_client Alternately maybe
>>>> just test the type of discovery used by bt_gatt_client. When
>>>> implementing the read value tests, the auto-discovery of
>>>> bt_gatt_client may make the test data longer because it needs to be
>>>> included in every one.
>>>
>>> Isn't it what we want? To run exactly as it would be run against PTS
>>> so we avoid surprises? Not only this avoid auto-discovery but the
>>> caching done by bt_gatt_client which we would probably have to test
>>> separately, so it is much more code that we exercise if we test
>>> bt_gatt_client at the expense of a little bit more PDUs. Perhaps we
>>> could have a define containing the common PDUs that auto-discovery
>>> causes so the upcoming tests don't need to duplicate these PDUs.
>>>
>
> I agree we test much more code while testing gatt-client. My idea was
> to use bt_att in GAD group,
> because we need to send specified packets and it is more like testing
> protocol than profile.
> Whole discovery using gatt-client will be tested in most of GAR and
> GAW test cases.
> I'm sure that it will be bigger part of unit tests.

Fair enough, but please add the test description to the commits so we
are consistent, for example:

unit/test-gatt: Add <test name> test

Verify that a Generic Attribute Profile client discovers Primary
Services in a GATT server.

2014-10-30 06:26:18

by Marcin Kraglak

[permalink] [raw]
Subject: Re: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case

Hi Luiz, Arman

On 29 October 2014 17:11, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
> On Wed, Oct 29, 2014 at 8:25 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Michael,
>>
>> On Wed, Oct 29, 2014 at 5:03 PM, Michael Janssen <[email protected]> wrote:
>>> On Wed, Oct 29, 2014 at 5:32 AM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> Hi Marcin,
>>>>
>>>> On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak
>>>> <[email protected]> wrote:
>>>>> ---
>>>>> unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>>>>> 1 file changed, 77 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>>>> index bbbf9a5..f84b0fc 100644
>>>>> --- a/unit/test-gatt.c
>>>>> +++ b/unit/test-gatt.c
>>>>> @@ -35,8 +35,10 @@
>>>>>
>>>>> #include <glib.h>
>>>>>
>>>>> +#include "lib/uuid.h"
>>>>> #include "src/shared/util.h"
>>>>> #include "src/shared/att.h"
>>>>> +#include "src/shared/gatt-helpers.h"
>>>>> #include "src/shared/gatt-client.h"
>>>>>
>>>>> struct test_pdu {
>>>>> @@ -45,14 +47,22 @@ struct test_pdu {
>>>>> size_t size;
>>>>> };
>>>>>
>>>>> +enum context_type {
>>>>> + ATT,
>>>>> + CLIENT,
>>>>> + SERVER
>>>>> +};
>>>>
>>>> Well the ATT part of this is a bit weird, do we really need to do this
>>>> distinction.
>>>>
>>>>> struct test_data {
>>>>> char *test_name;
>>>>> struct test_pdu *pdu_list;
>>>>> + enum context_type context_type;
>>>>> };
>>>>>
>>>>> struct context {
>>>>> GMainLoop *main_loop;
>>>>> struct bt_gatt_client *client;
>>>>> + struct bt_att *att;
>>>>> guint source;
>>>>> guint process;
>>>>> int fd;
>>>>> @@ -69,13 +79,14 @@ struct context {
>>>>> .size = sizeof(data(args)), \
>>>>> }
>>>>>
>>>>> -#define define_test(name, function, args...) \
>>>>> +#define define_test(name, function, type, args...) \
>>>>> do { \
>>>>> const struct test_pdu pdus[] = { \
>>>>> args, { } \
>>>>> }; \
>>>>> static struct test_data data; \
>>>>> data.test_name = g_strdup(name); \
>>>>> + data.context_type = type; \
>>>>> data.pdu_list = g_malloc(sizeof(pdus)); \
>>>>> memcpy(data.pdu_list, pdus, sizeof(pdus)); \
>>>>> g_test_add_data_func(name, &data, function); \
>>>>> @@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data)
>>>>> static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>> {
>>>>> struct context *context = g_new0(struct context, 1);
>>>>> + const struct test_data *test_data = data;
>>>>> GIOChannel *channel;
>>>>> int err, sv[2];
>>>>> struct bt_att *att;
>>>>> @@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>> att = bt_att_new(sv[0]);
>>>>> g_assert(att);
>>>>>
>>>>> - context->client = bt_gatt_client_new(att, mtu);
>>>>> - g_assert(context->client);
>>>>> + switch (test_data->context_type) {
>>>>> + case ATT:
>>>>> + context->att = att;
>>>>>
>>>>> - if (g_test_verbose())
>>>>> - bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
>>>>> - NULL);
>>>>> + bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL);
>>>>> + break;
>>>>> + case CLIENT:
>>>>> + context->client = bt_gatt_client_new(att, mtu);
>>>>> + g_assert(context->client);
>>>>> +
>>>>> + if (g_test_verbose())
>>>>> + bt_gatt_client_set_debug(context->client, gatt_debug,
>>>>> + "gatt:", NULL);
>>>>> +
>>>>> + bt_att_unref(att);
>>>>> + break;
>>>>> + default:
>>>>> + break;
>>>>> + }
>>>>>
>>>>> channel = g_io_channel_unix_new(sv[1]);
>>>>>
>>>>> @@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>> g_assert(context->source > 0);
>>>>>
>>>>> g_io_channel_unref(channel);
>>>>> - bt_att_unref(att);
>>>>>
>>>>> context->fd = sv[1];
>>>>> context->data = data;
>>>>> @@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>>> return context;
>>>>> }
>>>>>
>>>>> +static void primary_cb(bool success, uint8_t att_ecode,
>>>>> + struct bt_gatt_result *result,
>>>>> + void *user_data)
>>>>> +{
>>>>> + struct context *context = user_data;
>>>>> +
>>>>> + g_assert(success);
>>>>> +
>>>>> + context_quit(context);
>>>>> +}
>>>>> +
>>>>> static void destroy_context(struct context *context)
>>>>> {
>>>>> if (context->source > 0)
>>>>> @@ -229,6 +264,9 @@ static void destroy_context(struct context *context)
>>>>>
>>>>> bt_gatt_client_unref(context->client);
>>>>>
>>>>> + if (context->att)
>>>>> + bt_att_unref(context->att);
>>>>> +
>>>>> g_main_loop_unref(context->main_loop);
>>>>>
>>>>> test_free(context->data);
>>>>> @@ -249,6 +287,16 @@ static void test_client(gconstpointer data)
>>>>> execute_context(context);
>>>>> }
>>>>>
>>>>> +static void test_search_primary(gconstpointer data)
>>>>> +{
>>>>> + struct context *context = create_context(512, data);
>>>>> +
>>>>> + bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
>>>>> + context, NULL);
>>>>
>>>> Doesn't bt_gatt_client does the same here, I guess I missed this
>>>> detail in the first time but if this is just testing client side then
>>>> it should use bt_gatt_client and if that cannot be tested we probably
>>>> have a problem with our API. I guess PTS would just pass if we do more
>>>> than search the primary services, so things like setting the MTU etc
>>>> should not be a problem here.
>>>
>>> Specifically setting the MTU is allowed, but the reason to avoid using
>>> bt_gatt_client_new/init here is to avoid the automatic service
>>> discovery. This is probably a good idea at least for testing the
>>> different types of discovery. (is bt_gatt_client Alternately maybe
>>> just test the type of discovery used by bt_gatt_client. When
>>> implementing the read value tests, the auto-discovery of
>>> bt_gatt_client may make the test data longer because it needs to be
>>> included in every one.
>>
>> Isn't it what we want? To run exactly as it would be run against PTS
>> so we avoid surprises? Not only this avoid auto-discovery but the
>> caching done by bt_gatt_client which we would probably have to test
>> separately, so it is much more code that we exercise if we test
>> bt_gatt_client at the expense of a little bit more PDUs. Perhaps we
>> could have a define containing the common PDUs that auto-discovery
>> causes so the upcoming tests don't need to duplicate these PDUs.
>>

I agree we test much more code while testing gatt-client. My idea was
to use bt_att in GAD group,
because we need to send specified packets and it is more like testing
protocol than profile.
Whole discovery using gatt-client will be tested in most of GAR and
GAW test cases.
I'm sure that it will be bigger part of unit tests.

>
> In general, there is value in unit testing the gatt-helpers functions
> directly but overall I agree with Luiz that we should run these tests
> exactly like we would run it against the PTS. As long as the
> "database" we're returning in the response PDUs matches the complexity
> required by the test specification, a simple initialization of
> bt_gatt_client would give us all major discovery test cases. Of course
> we'll need to call gatt-helpers directly for test cases involving GATT
> procedures that don't aren't currently being used by bt_gatt_client
> (e.g. discover services by UUID).
>
> So it's probably OK to leave these ATT level tests for now but we also
> want at least a CLIENT level test to verify all of discovery test spec
> cases using the service/characteristic/include iterators.
>
>>>>> + execute_context(context);
>>>>> +}
>>>>> +
>>>>> int main(int argc, char *argv[])
>>>>> {
>>>>> g_test_init(&argc, &argv, NULL);
>>>>> @@ -259,8 +307,29 @@ int main(int argc, char *argv[])
>>>>> * The test group objective is to verify Generic Attribute Profile
>>>>> * Server Configuration.
>>>>> */
>>>>> - define_test("/TP/GAC/CL/BV-01-C", test_client,
>>>>> + define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT,
>>>>> raw_pdu(0x02, 0x00, 0x02));
>>>>>
>>>>> + /*
>>>>> + * Discovery
>>>>> + *
>>>>> + * The test group objective is to verify Generic Attribute Profile
>>>>> + * Discovery of Services and Service Characteristics.
>>>>> + */
>>>>> + define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT,
>>>>> + raw_pdu(0x02, 0x00, 0x02),
>>>>> + raw_pdu(0x03, 0x00, 0x02),
>>>>> + raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>>> + raw_pdu(0x11, 0x06, 0x10, 0x00, 0x13, 0x00, 0x00, 0x18,
>>>>> + 0x20, 0x00, 0x29, 0x00, 0xb0, 0x68,
>>>>> + 0x30, 0x00, 0x32, 0x00, 0x19, 0x18),
>>>>> + raw_pdu(0x10, 0x33, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>>> + raw_pdu(0x11, 0x14, 0x90, 0x00, 0x96, 0x00, 0xef, 0xcd,
>>>>> + 0xab, 0x89, 0x67, 0x45, 0x23, 0x01,
>>>>> + 0x00, 0x00, 0x00, 0x00, 0x85, 0x60,
>>>>> + 0x00, 0x00),
>>>>> + raw_pdu(0x10, 0x97, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>>> + raw_pdu(0x01, 0x10, 0x97, 0x00, 0x0a));
>>>>> +
>>>>> return g_test_run();
>>>>> }
>>>>> --
>>>>> 1.9.3
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>>> the body of a message to [email protected]
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>>
>>>> --
>>>> Luiz Augusto von Dentz
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>> --
>>> Michael Janssen
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Arman
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-10-29 16:11:55

by Arman Uguray

[permalink] [raw]
Subject: Re: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case

Hi Luiz,

On Wed, Oct 29, 2014 at 8:25 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Michael,
>
> On Wed, Oct 29, 2014 at 5:03 PM, Michael Janssen <[email protected]> wrote:
>> On Wed, Oct 29, 2014 at 5:32 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> Hi Marcin,
>>>
>>> On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak
>>> <[email protected]> wrote:
>>>> ---
>>>> unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 77 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>>> index bbbf9a5..f84b0fc 100644
>>>> --- a/unit/test-gatt.c
>>>> +++ b/unit/test-gatt.c
>>>> @@ -35,8 +35,10 @@
>>>>
>>>> #include <glib.h>
>>>>
>>>> +#include "lib/uuid.h"
>>>> #include "src/shared/util.h"
>>>> #include "src/shared/att.h"
>>>> +#include "src/shared/gatt-helpers.h"
>>>> #include "src/shared/gatt-client.h"
>>>>
>>>> struct test_pdu {
>>>> @@ -45,14 +47,22 @@ struct test_pdu {
>>>> size_t size;
>>>> };
>>>>
>>>> +enum context_type {
>>>> + ATT,
>>>> + CLIENT,
>>>> + SERVER
>>>> +};
>>>
>>> Well the ATT part of this is a bit weird, do we really need to do this
>>> distinction.
>>>
>>>> struct test_data {
>>>> char *test_name;
>>>> struct test_pdu *pdu_list;
>>>> + enum context_type context_type;
>>>> };
>>>>
>>>> struct context {
>>>> GMainLoop *main_loop;
>>>> struct bt_gatt_client *client;
>>>> + struct bt_att *att;
>>>> guint source;
>>>> guint process;
>>>> int fd;
>>>> @@ -69,13 +79,14 @@ struct context {
>>>> .size = sizeof(data(args)), \
>>>> }
>>>>
>>>> -#define define_test(name, function, args...) \
>>>> +#define define_test(name, function, type, args...) \
>>>> do { \
>>>> const struct test_pdu pdus[] = { \
>>>> args, { } \
>>>> }; \
>>>> static struct test_data data; \
>>>> data.test_name = g_strdup(name); \
>>>> + data.context_type = type; \
>>>> data.pdu_list = g_malloc(sizeof(pdus)); \
>>>> memcpy(data.pdu_list, pdus, sizeof(pdus)); \
>>>> g_test_add_data_func(name, &data, function); \
>>>> @@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data)
>>>> static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>> {
>>>> struct context *context = g_new0(struct context, 1);
>>>> + const struct test_data *test_data = data;
>>>> GIOChannel *channel;
>>>> int err, sv[2];
>>>> struct bt_att *att;
>>>> @@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>> att = bt_att_new(sv[0]);
>>>> g_assert(att);
>>>>
>>>> - context->client = bt_gatt_client_new(att, mtu);
>>>> - g_assert(context->client);
>>>> + switch (test_data->context_type) {
>>>> + case ATT:
>>>> + context->att = att;
>>>>
>>>> - if (g_test_verbose())
>>>> - bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
>>>> - NULL);
>>>> + bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL);
>>>> + break;
>>>> + case CLIENT:
>>>> + context->client = bt_gatt_client_new(att, mtu);
>>>> + g_assert(context->client);
>>>> +
>>>> + if (g_test_verbose())
>>>> + bt_gatt_client_set_debug(context->client, gatt_debug,
>>>> + "gatt:", NULL);
>>>> +
>>>> + bt_att_unref(att);
>>>> + break;
>>>> + default:
>>>> + break;
>>>> + }
>>>>
>>>> channel = g_io_channel_unix_new(sv[1]);
>>>>
>>>> @@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>> g_assert(context->source > 0);
>>>>
>>>> g_io_channel_unref(channel);
>>>> - bt_att_unref(att);
>>>>
>>>> context->fd = sv[1];
>>>> context->data = data;
>>>> @@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>>> return context;
>>>> }
>>>>
>>>> +static void primary_cb(bool success, uint8_t att_ecode,
>>>> + struct bt_gatt_result *result,
>>>> + void *user_data)
>>>> +{
>>>> + struct context *context = user_data;
>>>> +
>>>> + g_assert(success);
>>>> +
>>>> + context_quit(context);
>>>> +}
>>>> +
>>>> static void destroy_context(struct context *context)
>>>> {
>>>> if (context->source > 0)
>>>> @@ -229,6 +264,9 @@ static void destroy_context(struct context *context)
>>>>
>>>> bt_gatt_client_unref(context->client);
>>>>
>>>> + if (context->att)
>>>> + bt_att_unref(context->att);
>>>> +
>>>> g_main_loop_unref(context->main_loop);
>>>>
>>>> test_free(context->data);
>>>> @@ -249,6 +287,16 @@ static void test_client(gconstpointer data)
>>>> execute_context(context);
>>>> }
>>>>
>>>> +static void test_search_primary(gconstpointer data)
>>>> +{
>>>> + struct context *context = create_context(512, data);
>>>> +
>>>> + bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
>>>> + context, NULL);
>>>
>>> Doesn't bt_gatt_client does the same here, I guess I missed this
>>> detail in the first time but if this is just testing client side then
>>> it should use bt_gatt_client and if that cannot be tested we probably
>>> have a problem with our API. I guess PTS would just pass if we do more
>>> than search the primary services, so things like setting the MTU etc
>>> should not be a problem here.
>>
>> Specifically setting the MTU is allowed, but the reason to avoid using
>> bt_gatt_client_new/init here is to avoid the automatic service
>> discovery. This is probably a good idea at least for testing the
>> different types of discovery. (is bt_gatt_client Alternately maybe
>> just test the type of discovery used by bt_gatt_client. When
>> implementing the read value tests, the auto-discovery of
>> bt_gatt_client may make the test data longer because it needs to be
>> included in every one.
>
> Isn't it what we want? To run exactly as it would be run against PTS
> so we avoid surprises? Not only this avoid auto-discovery but the
> caching done by bt_gatt_client which we would probably have to test
> separately, so it is much more code that we exercise if we test
> bt_gatt_client at the expense of a little bit more PDUs. Perhaps we
> could have a define containing the common PDUs that auto-discovery
> causes so the upcoming tests don't need to duplicate these PDUs.
>

In general, there is value in unit testing the gatt-helpers functions
directly but overall I agree with Luiz that we should run these tests
exactly like we would run it against the PTS. As long as the
"database" we're returning in the response PDUs matches the complexity
required by the test specification, a simple initialization of
bt_gatt_client would give us all major discovery test cases. Of course
we'll need to call gatt-helpers directly for test cases involving GATT
procedures that don't aren't currently being used by bt_gatt_client
(e.g. discover services by UUID).

So it's probably OK to leave these ATT level tests for now but we also
want at least a CLIENT level test to verify all of discovery test spec
cases using the service/characteristic/include iterators.

>>>> + execute_context(context);
>>>> +}
>>>> +
>>>> int main(int argc, char *argv[])
>>>> {
>>>> g_test_init(&argc, &argv, NULL);
>>>> @@ -259,8 +307,29 @@ int main(int argc, char *argv[])
>>>> * The test group objective is to verify Generic Attribute Profile
>>>> * Server Configuration.
>>>> */
>>>> - define_test("/TP/GAC/CL/BV-01-C", test_client,
>>>> + define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT,
>>>> raw_pdu(0x02, 0x00, 0x02));
>>>>
>>>> + /*
>>>> + * Discovery
>>>> + *
>>>> + * The test group objective is to verify Generic Attribute Profile
>>>> + * Discovery of Services and Service Characteristics.
>>>> + */
>>>> + define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT,
>>>> + raw_pdu(0x02, 0x00, 0x02),
>>>> + raw_pdu(0x03, 0x00, 0x02),
>>>> + raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>> + raw_pdu(0x11, 0x06, 0x10, 0x00, 0x13, 0x00, 0x00, 0x18,
>>>> + 0x20, 0x00, 0x29, 0x00, 0xb0, 0x68,
>>>> + 0x30, 0x00, 0x32, 0x00, 0x19, 0x18),
>>>> + raw_pdu(0x10, 0x33, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>> + raw_pdu(0x11, 0x14, 0x90, 0x00, 0x96, 0x00, 0xef, 0xcd,
>>>> + 0xab, 0x89, 0x67, 0x45, 0x23, 0x01,
>>>> + 0x00, 0x00, 0x00, 0x00, 0x85, 0x60,
>>>> + 0x00, 0x00),
>>>> + raw_pdu(0x10, 0x97, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>> + raw_pdu(0x01, 0x10, 0x97, 0x00, 0x0a));
>>>> +
>>>> return g_test_run();
>>>> }
>>>> --
>>>> 1.9.3
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>> Michael Janssen
>
>
>
> --
> Luiz Augusto von Dentz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Arman

2014-10-29 15:25:40

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case

Hi Michael,

On Wed, Oct 29, 2014 at 5:03 PM, Michael Janssen <[email protected]> wrote:
> On Wed, Oct 29, 2014 at 5:32 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Marcin,
>>
>> On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak
>> <[email protected]> wrote:
>>> ---
>>> unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 77 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>> index bbbf9a5..f84b0fc 100644
>>> --- a/unit/test-gatt.c
>>> +++ b/unit/test-gatt.c
>>> @@ -35,8 +35,10 @@
>>>
>>> #include <glib.h>
>>>
>>> +#include "lib/uuid.h"
>>> #include "src/shared/util.h"
>>> #include "src/shared/att.h"
>>> +#include "src/shared/gatt-helpers.h"
>>> #include "src/shared/gatt-client.h"
>>>
>>> struct test_pdu {
>>> @@ -45,14 +47,22 @@ struct test_pdu {
>>> size_t size;
>>> };
>>>
>>> +enum context_type {
>>> + ATT,
>>> + CLIENT,
>>> + SERVER
>>> +};
>>
>> Well the ATT part of this is a bit weird, do we really need to do this
>> distinction.
>>
>>> struct test_data {
>>> char *test_name;
>>> struct test_pdu *pdu_list;
>>> + enum context_type context_type;
>>> };
>>>
>>> struct context {
>>> GMainLoop *main_loop;
>>> struct bt_gatt_client *client;
>>> + struct bt_att *att;
>>> guint source;
>>> guint process;
>>> int fd;
>>> @@ -69,13 +79,14 @@ struct context {
>>> .size = sizeof(data(args)), \
>>> }
>>>
>>> -#define define_test(name, function, args...) \
>>> +#define define_test(name, function, type, args...) \
>>> do { \
>>> const struct test_pdu pdus[] = { \
>>> args, { } \
>>> }; \
>>> static struct test_data data; \
>>> data.test_name = g_strdup(name); \
>>> + data.context_type = type; \
>>> data.pdu_list = g_malloc(sizeof(pdus)); \
>>> memcpy(data.pdu_list, pdus, sizeof(pdus)); \
>>> g_test_add_data_func(name, &data, function); \
>>> @@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data)
>>> static struct context *create_context(uint16_t mtu, gconstpointer data)
>>> {
>>> struct context *context = g_new0(struct context, 1);
>>> + const struct test_data *test_data = data;
>>> GIOChannel *channel;
>>> int err, sv[2];
>>> struct bt_att *att;
>>> @@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>> att = bt_att_new(sv[0]);
>>> g_assert(att);
>>>
>>> - context->client = bt_gatt_client_new(att, mtu);
>>> - g_assert(context->client);
>>> + switch (test_data->context_type) {
>>> + case ATT:
>>> + context->att = att;
>>>
>>> - if (g_test_verbose())
>>> - bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
>>> - NULL);
>>> + bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL);
>>> + break;
>>> + case CLIENT:
>>> + context->client = bt_gatt_client_new(att, mtu);
>>> + g_assert(context->client);
>>> +
>>> + if (g_test_verbose())
>>> + bt_gatt_client_set_debug(context->client, gatt_debug,
>>> + "gatt:", NULL);
>>> +
>>> + bt_att_unref(att);
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>>
>>> channel = g_io_channel_unix_new(sv[1]);
>>>
>>> @@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>> g_assert(context->source > 0);
>>>
>>> g_io_channel_unref(channel);
>>> - bt_att_unref(att);
>>>
>>> context->fd = sv[1];
>>> context->data = data;
>>> @@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>>> return context;
>>> }
>>>
>>> +static void primary_cb(bool success, uint8_t att_ecode,
>>> + struct bt_gatt_result *result,
>>> + void *user_data)
>>> +{
>>> + struct context *context = user_data;
>>> +
>>> + g_assert(success);
>>> +
>>> + context_quit(context);
>>> +}
>>> +
>>> static void destroy_context(struct context *context)
>>> {
>>> if (context->source > 0)
>>> @@ -229,6 +264,9 @@ static void destroy_context(struct context *context)
>>>
>>> bt_gatt_client_unref(context->client);
>>>
>>> + if (context->att)
>>> + bt_att_unref(context->att);
>>> +
>>> g_main_loop_unref(context->main_loop);
>>>
>>> test_free(context->data);
>>> @@ -249,6 +287,16 @@ static void test_client(gconstpointer data)
>>> execute_context(context);
>>> }
>>>
>>> +static void test_search_primary(gconstpointer data)
>>> +{
>>> + struct context *context = create_context(512, data);
>>> +
>>> + bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
>>> + context, NULL);
>>
>> Doesn't bt_gatt_client does the same here, I guess I missed this
>> detail in the first time but if this is just testing client side then
>> it should use bt_gatt_client and if that cannot be tested we probably
>> have a problem with our API. I guess PTS would just pass if we do more
>> than search the primary services, so things like setting the MTU etc
>> should not be a problem here.
>
> Specifically setting the MTU is allowed, but the reason to avoid using
> bt_gatt_client_new/init here is to avoid the automatic service
> discovery. This is probably a good idea at least for testing the
> different types of discovery. (is bt_gatt_client Alternately maybe
> just test the type of discovery used by bt_gatt_client. When
> implementing the read value tests, the auto-discovery of
> bt_gatt_client may make the test data longer because it needs to be
> included in every one.

Isn't it what we want? To run exactly as it would be run against PTS
so we avoid surprises? Not only this avoid auto-discovery but the
caching done by bt_gatt_client which we would probably have to test
separately, so it is much more code that we exercise if we test
bt_gatt_client at the expense of a little bit more PDUs. Perhaps we
could have a define containing the common PDUs that auto-discovery
causes so the upcoming tests don't need to duplicate these PDUs.

>>> + execute_context(context);
>>> +}
>>> +
>>> int main(int argc, char *argv[])
>>> {
>>> g_test_init(&argc, &argv, NULL);
>>> @@ -259,8 +307,29 @@ int main(int argc, char *argv[])
>>> * The test group objective is to verify Generic Attribute Profile
>>> * Server Configuration.
>>> */
>>> - define_test("/TP/GAC/CL/BV-01-C", test_client,
>>> + define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT,
>>> raw_pdu(0x02, 0x00, 0x02));
>>>
>>> + /*
>>> + * Discovery
>>> + *
>>> + * The test group objective is to verify Generic Attribute Profile
>>> + * Discovery of Services and Service Characteristics.
>>> + */
>>> + define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT,
>>> + raw_pdu(0x02, 0x00, 0x02),
>>> + raw_pdu(0x03, 0x00, 0x02),
>>> + raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
>>> + raw_pdu(0x11, 0x06, 0x10, 0x00, 0x13, 0x00, 0x00, 0x18,
>>> + 0x20, 0x00, 0x29, 0x00, 0xb0, 0x68,
>>> + 0x30, 0x00, 0x32, 0x00, 0x19, 0x18),
>>> + raw_pdu(0x10, 0x33, 0x00, 0xff, 0xff, 0x00, 0x28),
>>> + raw_pdu(0x11, 0x14, 0x90, 0x00, 0x96, 0x00, 0xef, 0xcd,
>>> + 0xab, 0x89, 0x67, 0x45, 0x23, 0x01,
>>> + 0x00, 0x00, 0x00, 0x00, 0x85, 0x60,
>>> + 0x00, 0x00),
>>> + raw_pdu(0x10, 0x97, 0x00, 0xff, 0xff, 0x00, 0x28),
>>> + raw_pdu(0x01, 0x10, 0x97, 0x00, 0x0a));
>>> +
>>> return g_test_run();
>>> }
>>> --
>>> 1.9.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Michael Janssen



--
Luiz Augusto von Dentz

2014-10-29 15:03:54

by Michael Janssen

[permalink] [raw]
Subject: Re: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case

On Wed, Oct 29, 2014 at 5:32 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Marcin,
>
> On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak
> <[email protected]> wrote:
>> ---
>> unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 77 insertions(+), 8 deletions(-)
>>
>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>> index bbbf9a5..f84b0fc 100644
>> --- a/unit/test-gatt.c
>> +++ b/unit/test-gatt.c
>> @@ -35,8 +35,10 @@
>>
>> #include <glib.h>
>>
>> +#include "lib/uuid.h"
>> #include "src/shared/util.h"
>> #include "src/shared/att.h"
>> +#include "src/shared/gatt-helpers.h"
>> #include "src/shared/gatt-client.h"
>>
>> struct test_pdu {
>> @@ -45,14 +47,22 @@ struct test_pdu {
>> size_t size;
>> };
>>
>> +enum context_type {
>> + ATT,
>> + CLIENT,
>> + SERVER
>> +};
>
> Well the ATT part of this is a bit weird, do we really need to do this
> distinction.
>
>> struct test_data {
>> char *test_name;
>> struct test_pdu *pdu_list;
>> + enum context_type context_type;
>> };
>>
>> struct context {
>> GMainLoop *main_loop;
>> struct bt_gatt_client *client;
>> + struct bt_att *att;
>> guint source;
>> guint process;
>> int fd;
>> @@ -69,13 +79,14 @@ struct context {
>> .size = sizeof(data(args)), \
>> }
>>
>> -#define define_test(name, function, args...) \
>> +#define define_test(name, function, type, args...) \
>> do { \
>> const struct test_pdu pdus[] = { \
>> args, { } \
>> }; \
>> static struct test_data data; \
>> data.test_name = g_strdup(name); \
>> + data.context_type = type; \
>> data.pdu_list = g_malloc(sizeof(pdus)); \
>> memcpy(data.pdu_list, pdus, sizeof(pdus)); \
>> g_test_add_data_func(name, &data, function); \
>> @@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data)
>> static struct context *create_context(uint16_t mtu, gconstpointer data)
>> {
>> struct context *context = g_new0(struct context, 1);
>> + const struct test_data *test_data = data;
>> GIOChannel *channel;
>> int err, sv[2];
>> struct bt_att *att;
>> @@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>> att = bt_att_new(sv[0]);
>> g_assert(att);
>>
>> - context->client = bt_gatt_client_new(att, mtu);
>> - g_assert(context->client);
>> + switch (test_data->context_type) {
>> + case ATT:
>> + context->att = att;
>>
>> - if (g_test_verbose())
>> - bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
>> - NULL);
>> + bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL);
>> + break;
>> + case CLIENT:
>> + context->client = bt_gatt_client_new(att, mtu);
>> + g_assert(context->client);
>> +
>> + if (g_test_verbose())
>> + bt_gatt_client_set_debug(context->client, gatt_debug,
>> + "gatt:", NULL);
>> +
>> + bt_att_unref(att);
>> + break;
>> + default:
>> + break;
>> + }
>>
>> channel = g_io_channel_unix_new(sv[1]);
>>
>> @@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>> g_assert(context->source > 0);
>>
>> g_io_channel_unref(channel);
>> - bt_att_unref(att);
>>
>> context->fd = sv[1];
>> context->data = data;
>> @@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
>> return context;
>> }
>>
>> +static void primary_cb(bool success, uint8_t att_ecode,
>> + struct bt_gatt_result *result,
>> + void *user_data)
>> +{
>> + struct context *context = user_data;
>> +
>> + g_assert(success);
>> +
>> + context_quit(context);
>> +}
>> +
>> static void destroy_context(struct context *context)
>> {
>> if (context->source > 0)
>> @@ -229,6 +264,9 @@ static void destroy_context(struct context *context)
>>
>> bt_gatt_client_unref(context->client);
>>
>> + if (context->att)
>> + bt_att_unref(context->att);
>> +
>> g_main_loop_unref(context->main_loop);
>>
>> test_free(context->data);
>> @@ -249,6 +287,16 @@ static void test_client(gconstpointer data)
>> execute_context(context);
>> }
>>
>> +static void test_search_primary(gconstpointer data)
>> +{
>> + struct context *context = create_context(512, data);
>> +
>> + bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
>> + context, NULL);
>
> Doesn't bt_gatt_client does the same here, I guess I missed this
> detail in the first time but if this is just testing client side then
> it should use bt_gatt_client and if that cannot be tested we probably
> have a problem with our API. I guess PTS would just pass if we do more
> than search the primary services, so things like setting the MTU etc
> should not be a problem here.

Specifically setting the MTU is allowed, but the reason to avoid using
bt_gatt_client_new/init here is to avoid the automatic service
discovery. This is probably a good idea at least for testing the
different types of discovery. (is bt_gatt_client Alternately maybe
just test the type of discovery used by bt_gatt_client. When
implementing the read value tests, the auto-discovery of
bt_gatt_client may make the test data longer because it needs to be
included in every one.

>> + execute_context(context);
>> +}
>> +
>> int main(int argc, char *argv[])
>> {
>> g_test_init(&argc, &argv, NULL);
>> @@ -259,8 +307,29 @@ int main(int argc, char *argv[])
>> * The test group objective is to verify Generic Attribute Profile
>> * Server Configuration.
>> */
>> - define_test("/TP/GAC/CL/BV-01-C", test_client,
>> + define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT,
>> raw_pdu(0x02, 0x00, 0x02));
>>
>> + /*
>> + * Discovery
>> + *
>> + * The test group objective is to verify Generic Attribute Profile
>> + * Discovery of Services and Service Characteristics.
>> + */
>> + define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT,
>> + raw_pdu(0x02, 0x00, 0x02),
>> + raw_pdu(0x03, 0x00, 0x02),
>> + raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
>> + raw_pdu(0x11, 0x06, 0x10, 0x00, 0x13, 0x00, 0x00, 0x18,
>> + 0x20, 0x00, 0x29, 0x00, 0xb0, 0x68,
>> + 0x30, 0x00, 0x32, 0x00, 0x19, 0x18),
>> + raw_pdu(0x10, 0x33, 0x00, 0xff, 0xff, 0x00, 0x28),
>> + raw_pdu(0x11, 0x14, 0x90, 0x00, 0x96, 0x00, 0xef, 0xcd,
>> + 0xab, 0x89, 0x67, 0x45, 0x23, 0x01,
>> + 0x00, 0x00, 0x00, 0x00, 0x85, 0x60,
>> + 0x00, 0x00),
>> + raw_pdu(0x10, 0x97, 0x00, 0xff, 0xff, 0x00, 0x28),
>> + raw_pdu(0x01, 0x10, 0x97, 0x00, 0x0a));
>> +
>> return g_test_run();
>> }
>> --
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Michael Janssen

2014-10-29 12:32:26

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case

Hi Marcin,

On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak
<[email protected]> wrote:
> ---
> unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 77 insertions(+), 8 deletions(-)
>
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index bbbf9a5..f84b0fc 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -35,8 +35,10 @@
>
> #include <glib.h>
>
> +#include "lib/uuid.h"
> #include "src/shared/util.h"
> #include "src/shared/att.h"
> +#include "src/shared/gatt-helpers.h"
> #include "src/shared/gatt-client.h"
>
> struct test_pdu {
> @@ -45,14 +47,22 @@ struct test_pdu {
> size_t size;
> };
>
> +enum context_type {
> + ATT,
> + CLIENT,
> + SERVER
> +};

Well the ATT part of this is a bit weird, do we really need to do this
distinction.

> struct test_data {
> char *test_name;
> struct test_pdu *pdu_list;
> + enum context_type context_type;
> };
>
> struct context {
> GMainLoop *main_loop;
> struct bt_gatt_client *client;
> + struct bt_att *att;
> guint source;
> guint process;
> int fd;
> @@ -69,13 +79,14 @@ struct context {
> .size = sizeof(data(args)), \
> }
>
> -#define define_test(name, function, args...) \
> +#define define_test(name, function, type, args...) \
> do { \
> const struct test_pdu pdus[] = { \
> args, { } \
> }; \
> static struct test_data data; \
> data.test_name = g_strdup(name); \
> + data.context_type = type; \
> data.pdu_list = g_malloc(sizeof(pdus)); \
> memcpy(data.pdu_list, pdus, sizeof(pdus)); \
> g_test_add_data_func(name, &data, function); \
> @@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data)
> static struct context *create_context(uint16_t mtu, gconstpointer data)
> {
> struct context *context = g_new0(struct context, 1);
> + const struct test_data *test_data = data;
> GIOChannel *channel;
> int err, sv[2];
> struct bt_att *att;
> @@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
> att = bt_att_new(sv[0]);
> g_assert(att);
>
> - context->client = bt_gatt_client_new(att, mtu);
> - g_assert(context->client);
> + switch (test_data->context_type) {
> + case ATT:
> + context->att = att;
>
> - if (g_test_verbose())
> - bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
> - NULL);
> + bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL);
> + break;
> + case CLIENT:
> + context->client = bt_gatt_client_new(att, mtu);
> + g_assert(context->client);
> +
> + if (g_test_verbose())
> + bt_gatt_client_set_debug(context->client, gatt_debug,
> + "gatt:", NULL);
> +
> + bt_att_unref(att);
> + break;
> + default:
> + break;
> + }
>
> channel = g_io_channel_unix_new(sv[1]);
>
> @@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
> g_assert(context->source > 0);
>
> g_io_channel_unref(channel);
> - bt_att_unref(att);
>
> context->fd = sv[1];
> context->data = data;
> @@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
> return context;
> }
>
> +static void primary_cb(bool success, uint8_t att_ecode,
> + struct bt_gatt_result *result,
> + void *user_data)
> +{
> + struct context *context = user_data;
> +
> + g_assert(success);
> +
> + context_quit(context);
> +}
> +
> static void destroy_context(struct context *context)
> {
> if (context->source > 0)
> @@ -229,6 +264,9 @@ static void destroy_context(struct context *context)
>
> bt_gatt_client_unref(context->client);
>
> + if (context->att)
> + bt_att_unref(context->att);
> +
> g_main_loop_unref(context->main_loop);
>
> test_free(context->data);
> @@ -249,6 +287,16 @@ static void test_client(gconstpointer data)
> execute_context(context);
> }
>
> +static void test_search_primary(gconstpointer data)
> +{
> + struct context *context = create_context(512, data);
> +
> + bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
> + context, NULL);

Doesn't bt_gatt_client does the same here, I guess I missed this
detail in the first time but if this is just testing client side then
it should use bt_gatt_client and if that cannot be tested we probably
have a problem with our API. I guess PTS would just pass if we do more
than search the primary services, so things like setting the MTU etc
should not be a problem here.

> + execute_context(context);
> +}
> +
> int main(int argc, char *argv[])
> {
> g_test_init(&argc, &argv, NULL);
> @@ -259,8 +307,29 @@ int main(int argc, char *argv[])
> * The test group objective is to verify Generic Attribute Profile
> * Server Configuration.
> */
> - define_test("/TP/GAC/CL/BV-01-C", test_client,
> + define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT,
> raw_pdu(0x02, 0x00, 0x02));
>
> + /*
> + * Discovery
> + *
> + * The test group objective is to verify Generic Attribute Profile
> + * Discovery of Services and Service Characteristics.
> + */
> + define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT,
> + raw_pdu(0x02, 0x00, 0x02),
> + raw_pdu(0x03, 0x00, 0x02),
> + raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
> + raw_pdu(0x11, 0x06, 0x10, 0x00, 0x13, 0x00, 0x00, 0x18,
> + 0x20, 0x00, 0x29, 0x00, 0xb0, 0x68,
> + 0x30, 0x00, 0x32, 0x00, 0x19, 0x18),
> + raw_pdu(0x10, 0x33, 0x00, 0xff, 0xff, 0x00, 0x28),
> + raw_pdu(0x11, 0x14, 0x90, 0x00, 0x96, 0x00, 0xef, 0xcd,
> + 0xab, 0x89, 0x67, 0x45, 0x23, 0x01,
> + 0x00, 0x00, 0x00, 0x00, 0x85, 0x60,
> + 0x00, 0x00),
> + raw_pdu(0x10, 0x97, 0x00, 0xff, 0xff, 0x00, 0x28),
> + raw_pdu(0x01, 0x10, 0x97, 0x00, 0x0a));
> +
> return g_test_run();
> }
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html



--
Luiz Augusto von Dentz

2014-10-29 00:40:46

by Arman Uguray

[permalink] [raw]
Subject: Re: [PATCHv2 7/7] unit/test-gatt: Add search characteristics test case

Hi Marcin,

On Tue, Oct 28, 2014 at 12:37 PM, Marcin Kraglak
<[email protected]> wrote:
> ---
> unit/test-gatt.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index 3ee9460..27471ea 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -270,6 +270,17 @@ static void primary_cb(bool success, uint8_t att_ecode,
> context_quit(context);
> }
>
> +static void characteristic_cb(bool success, uint8_t att_ecode,
> + struct bt_gatt_result *result,
> + void *user_data)
> +{
> + struct context *context = user_data;
> +
> + g_assert(success);
> +
> + context_quit(context);
> +}
> +
> static void included_cb(bool success, uint8_t att_ecode,
> struct bt_gatt_result *result,
> void *user_data)
> @@ -332,6 +343,17 @@ static void test_search_included(gconstpointer data)
> execute_context(context);
> }
>
> +static void test_search_chars(gconstpointer data)
> +{
> + struct context *context = create_context(512, data);
> +
> + g_assert(bt_gatt_discover_characteristics(context->att, 0x0010, 0x0020,
> + characteristic_cb, context,
> + NULL));

It's good that we're adding test cases using gatt-helpers. Are you
going to add a CLIENT case as well? It would be good to at least have
a test case where we initialize bt_gatt_client and use the iterators
to verify all the services, characteristics, etc. were cached
correctly.

> +
> + execute_context(context);
> +}
> +
> int main(int argc, char *argv[])
> {
> g_test_init(&argc, &argv, NULL);
> @@ -413,5 +435,19 @@ int main(int argc, char *argv[])
> raw_pdu(0x08, 0x06, 0x00, 0xff, 0xff, 0x02, 0x28),
> raw_pdu(0x01, 0x08, 0x06, 0x00, 0x0a));
>
> + define_test("/TP/GAD/CL/BV-04-C", test_search_chars, ATT, NULL,
> + raw_pdu(0x02, 0x00, 0x02),
> + raw_pdu(0x03, 0x00, 0x02),
> + raw_pdu(0x08, 0x10, 0x00, 0x20, 0x00, 0x03, 0x28),
> + raw_pdu(0x09, 0x07, 0x11, 0x00, 02, 0x12, 0x00, 0x25,
> + 0x2a),
> + raw_pdu(0x08, 0x12, 0x00, 0x20, 0x00, 0x03, 0x28),
> + raw_pdu(0x09, 0x15, 0x13, 0x00, 0x02, 0x14, 0x00, 0x85,
> + 0x00, 0xef, 0xcd, 0xab, 0x89, 0x67,
> + 0x45, 0x23, 0x01, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00),
> + raw_pdu(0x08, 0x14, 0x00, 0x20, 0x00, 0x03, 0x28),
> + raw_pdu(0x01, 0x08, 0x12, 0x00, 0x0a));
> +
> return g_test_run();
> }
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Cheers,
Arman

2014-10-28 19:37:47

by Marcin Kraglak

[permalink] [raw]
Subject: [PATCHv2 7/7] unit/test-gatt: Add search characteristics test case

---
unit/test-gatt.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)

diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 3ee9460..27471ea 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -270,6 +270,17 @@ static void primary_cb(bool success, uint8_t att_ecode,
context_quit(context);
}

+static void characteristic_cb(bool success, uint8_t att_ecode,
+ struct bt_gatt_result *result,
+ void *user_data)
+{
+ struct context *context = user_data;
+
+ g_assert(success);
+
+ context_quit(context);
+}
+
static void included_cb(bool success, uint8_t att_ecode,
struct bt_gatt_result *result,
void *user_data)
@@ -332,6 +343,17 @@ static void test_search_included(gconstpointer data)
execute_context(context);
}

+static void test_search_chars(gconstpointer data)
+{
+ struct context *context = create_context(512, data);
+
+ g_assert(bt_gatt_discover_characteristics(context->att, 0x0010, 0x0020,
+ characteristic_cb, context,
+ NULL));
+
+ execute_context(context);
+}
+
int main(int argc, char *argv[])
{
g_test_init(&argc, &argv, NULL);
@@ -413,5 +435,19 @@ int main(int argc, char *argv[])
raw_pdu(0x08, 0x06, 0x00, 0xff, 0xff, 0x02, 0x28),
raw_pdu(0x01, 0x08, 0x06, 0x00, 0x0a));

+ define_test("/TP/GAD/CL/BV-04-C", test_search_chars, ATT, NULL,
+ raw_pdu(0x02, 0x00, 0x02),
+ raw_pdu(0x03, 0x00, 0x02),
+ raw_pdu(0x08, 0x10, 0x00, 0x20, 0x00, 0x03, 0x28),
+ raw_pdu(0x09, 0x07, 0x11, 0x00, 02, 0x12, 0x00, 0x25,
+ 0x2a),
+ raw_pdu(0x08, 0x12, 0x00, 0x20, 0x00, 0x03, 0x28),
+ raw_pdu(0x09, 0x15, 0x13, 0x00, 0x02, 0x14, 0x00, 0x85,
+ 0x00, 0xef, 0xcd, 0xab, 0x89, 0x67,
+ 0x45, 0x23, 0x01, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00),
+ raw_pdu(0x08, 0x14, 0x00, 0x20, 0x00, 0x03, 0x28),
+ raw_pdu(0x01, 0x08, 0x12, 0x00, 0x0a));
+
return g_test_run();
}
--
1.9.3


2014-10-28 19:37:46

by Marcin Kraglak

[permalink] [raw]
Subject: [PATCHv2 6/7] unit/test-gatt: Add search included services test case

---
unit/test-gatt.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)

diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 239f471..3ee9460 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -270,6 +270,17 @@ static void primary_cb(bool success, uint8_t att_ecode,
context_quit(context);
}

+static void included_cb(bool success, uint8_t att_ecode,
+ struct bt_gatt_result *result,
+ void *user_data)
+{
+ struct context *context = user_data;
+
+ g_assert(success);
+
+ context_quit(context);
+}
+
static void destroy_context(struct context *context)
{
if (context->source > 0)
@@ -311,6 +322,16 @@ static void test_search_primary(gconstpointer data)
execute_context(context);
}

+static void test_search_included(gconstpointer data)
+{
+ struct context *context = create_context(512, data);
+
+ bt_gatt_discover_included_services(context->att, 0x0001, 0xffff,
+ included_cb, context, NULL);
+
+ execute_context(context);
+}
+
int main(int argc, char *argv[])
{
g_test_init(&argc, &argv, NULL);
@@ -369,5 +390,28 @@ int main(int argc, char *argv[])
0x18, 0x00, 0x00),
raw_pdu(0x01, 0x06, 0x08, 0x00, 0x0a));

+ define_test("/TP/GAD/CL/BV-03-C", test_search_included, ATT, NULL,
+ raw_pdu(0x02, 0x00, 0x02),
+ raw_pdu(0x03, 0x00, 0x02),
+ raw_pdu(0x08, 0x01, 0x00, 0xff, 0xff, 0x02, 0x28),
+ raw_pdu(0x09, 0x08, 0x02, 0x00, 0x10, 0x00, 0x1f, 0x00,
+ 0x0f, 0x18),
+ raw_pdu(0x08, 0x03, 0x00, 0xff, 0xff, 0x02, 0x28),
+ raw_pdu(0x09, 0x06, 0x03, 0x00, 0x20, 0x00, 0x2f, 0x00,
+ 0x04, 0x00, 0x30, 0x00, 0x3f, 0x00),
+ raw_pdu(0x0a, 0x20, 0x00),
+ raw_pdu(0x0b, 0x00, 0x00, 0x3e, 0x39, 0x00, 0x00, 0x00,
+ 0x00, 0x01, 0x23, 0x45, 0x67, 0x89,
+ 0xab, 0xcd, 0xef),
+ raw_pdu(0x0a, 0x30, 0x00),
+ raw_pdu(0x0b, 0x00, 0x00, 0x3b, 0x39, 0x00, 0x00, 0x00,
+ 0x00, 0x01, 0x23, 0x45, 0x67, 0x89,
+ 0xab, 0xcd, 0xef),
+ raw_pdu(0x08, 0x05, 0x00, 0xff, 0xff, 0x02, 0x28),
+ raw_pdu(0x09, 0x08, 0x05, 0x00, 0x40, 0x00, 0x4f, 0x00,
+ 0x0a, 0x18),
+ raw_pdu(0x08, 0x06, 0x00, 0xff, 0xff, 0x02, 0x28),
+ raw_pdu(0x01, 0x08, 0x06, 0x00, 0x0a));
+
return g_test_run();
}
--
1.9.3


2014-10-28 19:37:43

by Marcin Kraglak

[permalink] [raw]
Subject: [PATCHv2 3/7] unit/test-gatt: Add search primary by 16 but UUID test case

---
unit/test-gatt.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index f84b0fc..1167d4d 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -57,6 +57,7 @@ struct test_data {
char *test_name;
struct test_pdu *pdu_list;
enum context_type context_type;
+ bt_uuid_t *uuid;
};

struct context {
@@ -79,7 +80,7 @@ struct context {
.size = sizeof(data(args)), \
}

-#define define_test(name, function, type, args...) \
+#define define_test(name, function, type, bt_uuid, args...) \
do { \
const struct test_pdu pdus[] = { \
args, { } \
@@ -87,11 +88,17 @@ struct context {
static struct test_data data; \
data.test_name = g_strdup(name); \
data.context_type = type; \
+ data.uuid = bt_uuid; \
data.pdu_list = g_malloc(sizeof(pdus)); \
memcpy(data.pdu_list, pdus, sizeof(pdus)); \
g_test_add_data_func(name, &data, function); \
} while (0)

+static bt_uuid_t uuid_16 = {
+ .type = BT_UUID16,
+ .value.u16 = 0x1800
+};
+
static void test_debug(const char *str, void *user_data)
{
const char *prefix = user_data;
@@ -290,9 +297,10 @@ static void test_client(gconstpointer data)
static void test_search_primary(gconstpointer data)
{
struct context *context = create_context(512, data);
+ const struct test_data *test_data = data;

- bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
- context, NULL);
+ bt_gatt_discover_all_primary_services(context->att, test_data->uuid,
+ primary_cb, context, NULL);

execute_context(context);
}
@@ -307,7 +315,7 @@ int main(int argc, char *argv[])
* The test group objective is to verify Generic Attribute Profile
* Server Configuration.
*/
- define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT,
+ define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT, NULL,
raw_pdu(0x02, 0x00, 0x02));

/*
@@ -316,7 +324,7 @@ int main(int argc, char *argv[])
* The test group objective is to verify Generic Attribute Profile
* Discovery of Services and Service Characteristics.
*/
- define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT,
+ define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT, NULL,
raw_pdu(0x02, 0x00, 0x02),
raw_pdu(0x03, 0x00, 0x02),
raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
@@ -331,5 +339,15 @@ int main(int argc, char *argv[])
raw_pdu(0x10, 0x97, 0x00, 0xff, 0xff, 0x00, 0x28),
raw_pdu(0x01, 0x10, 0x97, 0x00, 0x0a));

+ define_test("/TP/GAD/CL/BV-02-C-1", test_search_primary, ATT, &uuid_16,
+ raw_pdu(0x02, 0x00, 0x02),
+ raw_pdu(0x03, 0x00, 0x02),
+ raw_pdu(0x06, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28, 0x00,
+ 0x18),
+ raw_pdu(0x07, 0x01, 0x00, 0x07, 0x00),
+ raw_pdu(0x06, 0x08, 0x00, 0xff, 0xff, 0x00, 0x28, 0x00,
+ 0x18),
+ raw_pdu(0x01, 0x06, 0x08, 0x00, 0x0a));
+
return g_test_run();
}
--
1.9.3


2014-10-28 19:37:44

by Marcin Kraglak

[permalink] [raw]
Subject: [PATCHv2 4/7] shared/gatt: Fix incorrect data read

Data set in Find By Type Value response contains 4 octets,
2 for start handle and two for end group handle. Reading data
with offset 6 from end of pdu can cause illegal access.
---
src/shared/gatt-helpers.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index d751d5a..6e19066 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -686,7 +686,13 @@ static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
op->result_tail = cur_result;
}

- last_end = get_le16(pdu + length - 6);
+ /*
+ * Each data set contains:
+ * 2 octets with start handle
+ * 2 octets with end handle
+ * last_end is end handle of last data set
+ */
+ last_end = get_le16(pdu + length - 2);
if (last_end < op->end_handle) {
uint8_t pdu[6 + get_uuid_len(&op->uuid)];

--
1.9.3


2014-10-28 19:37:45

by Marcin Kraglak

[permalink] [raw]
Subject: [PATCHv2 5/7] unit/test-gatt: Add search primary by 128 bit UUID test case

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

diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 1167d4d..239f471 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -99,6 +99,12 @@ static bt_uuid_t uuid_16 = {
.value.u16 = 0x1800
};

+static bt_uuid_t uuid_128 = {
+ .type = BT_UUID128,
+ .value.u128.data = {0x00, 0x00, 0x18, 0x0d, 0x00, 0x00, 0x10, 0x00,
+ 0x80, 0x00, 0x00, 0x80, 0x5f, 0x9b, 0x34, 0xfb}
+};
+
static void test_debug(const char *str, void *user_data)
{
const char *prefix = user_data;
@@ -349,5 +355,19 @@ int main(int argc, char *argv[])
0x18),
raw_pdu(0x01, 0x06, 0x08, 0x00, 0x0a));

+ define_test("/TP/GAD/CL/BV-02-C-2", test_search_primary, ATT, &uuid_128,
+ raw_pdu(0x02, 0x00, 0x02),
+ raw_pdu(0x03, 0x00, 0x02),
+ raw_pdu(06, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28, 0xfb,
+ 0x34, 0x9b, 0x5f, 0x80, 0x00, 0x00,
+ 0x80, 0x00, 0x10, 0x00, 0x00, 0x0d,
+ 0x18, 0x00, 0x00),
+ raw_pdu(0x07, 0x10, 0x00, 0x17, 0x00),
+ raw_pdu(06, 0x18, 0x00, 0xff, 0xff, 0x00, 0x28, 0xfb,
+ 0x34, 0x9b, 0x5f, 0x80, 0x00, 0x00,
+ 0x80, 0x00, 0x10, 0x00, 0x00, 0x0d,
+ 0x18, 0x00, 0x00),
+ raw_pdu(0x01, 0x06, 0x08, 0x00, 0x0a));
+
return g_test_run();
}
--
1.9.3


2014-10-28 19:37:42

by Marcin Kraglak

[permalink] [raw]
Subject: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case

---
unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 77 insertions(+), 8 deletions(-)

diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index bbbf9a5..f84b0fc 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -35,8 +35,10 @@

#include <glib.h>

+#include "lib/uuid.h"
#include "src/shared/util.h"
#include "src/shared/att.h"
+#include "src/shared/gatt-helpers.h"
#include "src/shared/gatt-client.h"

struct test_pdu {
@@ -45,14 +47,22 @@ struct test_pdu {
size_t size;
};

+enum context_type {
+ ATT,
+ CLIENT,
+ SERVER
+};
+
struct test_data {
char *test_name;
struct test_pdu *pdu_list;
+ enum context_type context_type;
};

struct context {
GMainLoop *main_loop;
struct bt_gatt_client *client;
+ struct bt_att *att;
guint source;
guint process;
int fd;
@@ -69,13 +79,14 @@ struct context {
.size = sizeof(data(args)), \
}

-#define define_test(name, function, args...) \
+#define define_test(name, function, type, args...) \
do { \
const struct test_pdu pdus[] = { \
args, { } \
}; \
static struct test_data data; \
data.test_name = g_strdup(name); \
+ data.context_type = type; \
data.pdu_list = g_malloc(sizeof(pdus)); \
memcpy(data.pdu_list, pdus, sizeof(pdus)); \
g_test_add_data_func(name, &data, function); \
@@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data)
static struct context *create_context(uint16_t mtu, gconstpointer data)
{
struct context *context = g_new0(struct context, 1);
+ const struct test_data *test_data = data;
GIOChannel *channel;
int err, sv[2];
struct bt_att *att;
@@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
att = bt_att_new(sv[0]);
g_assert(att);

- context->client = bt_gatt_client_new(att, mtu);
- g_assert(context->client);
+ switch (test_data->context_type) {
+ case ATT:
+ context->att = att;

- if (g_test_verbose())
- bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
- NULL);
+ bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL);
+ break;
+ case CLIENT:
+ context->client = bt_gatt_client_new(att, mtu);
+ g_assert(context->client);
+
+ if (g_test_verbose())
+ bt_gatt_client_set_debug(context->client, gatt_debug,
+ "gatt:", NULL);
+
+ bt_att_unref(att);
+ break;
+ default:
+ break;
+ }

channel = g_io_channel_unix_new(sv[1]);

@@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
g_assert(context->source > 0);

g_io_channel_unref(channel);
- bt_att_unref(att);

context->fd = sv[1];
context->data = data;
@@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
return context;
}

+static void primary_cb(bool success, uint8_t att_ecode,
+ struct bt_gatt_result *result,
+ void *user_data)
+{
+ struct context *context = user_data;
+
+ g_assert(success);
+
+ context_quit(context);
+}
+
static void destroy_context(struct context *context)
{
if (context->source > 0)
@@ -229,6 +264,9 @@ static void destroy_context(struct context *context)

bt_gatt_client_unref(context->client);

+ if (context->att)
+ bt_att_unref(context->att);
+
g_main_loop_unref(context->main_loop);

test_free(context->data);
@@ -249,6 +287,16 @@ static void test_client(gconstpointer data)
execute_context(context);
}

+static void test_search_primary(gconstpointer data)
+{
+ struct context *context = create_context(512, data);
+
+ bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb,
+ context, NULL);
+
+ execute_context(context);
+}
+
int main(int argc, char *argv[])
{
g_test_init(&argc, &argv, NULL);
@@ -259,8 +307,29 @@ int main(int argc, char *argv[])
* The test group objective is to verify Generic Attribute Profile
* Server Configuration.
*/
- define_test("/TP/GAC/CL/BV-01-C", test_client,
+ define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT,
raw_pdu(0x02, 0x00, 0x02));

+ /*
+ * Discovery
+ *
+ * The test group objective is to verify Generic Attribute Profile
+ * Discovery of Services and Service Characteristics.
+ */
+ define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT,
+ raw_pdu(0x02, 0x00, 0x02),
+ raw_pdu(0x03, 0x00, 0x02),
+ raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
+ raw_pdu(0x11, 0x06, 0x10, 0x00, 0x13, 0x00, 0x00, 0x18,
+ 0x20, 0x00, 0x29, 0x00, 0xb0, 0x68,
+ 0x30, 0x00, 0x32, 0x00, 0x19, 0x18),
+ raw_pdu(0x10, 0x33, 0x00, 0xff, 0xff, 0x00, 0x28),
+ raw_pdu(0x11, 0x14, 0x90, 0x00, 0x96, 0x00, 0xef, 0xcd,
+ 0xab, 0x89, 0x67, 0x45, 0x23, 0x01,
+ 0x00, 0x00, 0x00, 0x00, 0x85, 0x60,
+ 0x00, 0x00),
+ raw_pdu(0x10, 0x97, 0x00, 0xff, 0xff, 0x00, 0x28),
+ raw_pdu(0x01, 0x10, 0x97, 0x00, 0x0a));
+
return g_test_run();
}
--
1.9.3