From: Andrei Emeltchenko <[email protected]>
Silence code analyzers and follow strict C standard where passing NULL
pointer results in undefined behaviour.
---
android/tester-main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/android/tester-main.c b/android/tester-main.c
index 30e1c59..ee3444f 100644
--- a/android/tester-main.c
+++ b/android/tester-main.c
@@ -801,12 +801,12 @@ static bool match_data(struct step *step)
return false;
}
- if (exp->store_srvc_handle)
+ if (exp->store_srvc_handle && step->callback_result.srvc_handle)
memcpy(exp->store_srvc_handle,
step->callback_result.srvc_handle,
sizeof(*exp->store_srvc_handle));
- if (exp->store_char_handle)
+ if (exp->store_char_handle && step->callback_result.char_handle)
memcpy(exp->store_char_handle,
step->callback_result.char_handle,
sizeof(*exp->store_char_handle));
--
1.9.1
Hi Andrei,
On 9 October 2014 13:20, Andrei Emeltchenko
<[email protected]> wrote:
> Hi Grzegorz,
>
> On Thu, Oct 09, 2014 at 12:56:19PM +0200, Grzegorz Kolodziejczyk wrote:
>> Hi Andrei,
>>
>> On 9 October 2014 12:16, Andrei Emeltchenko
>> <[email protected]> wrote:
>> > From: Andrei Emeltchenko <[email protected]>
>> >
>> > Silence code analyzers and follow strict C standard where passing NULL
>> > pointer results in undefined behaviour.
>> > ---
>> > android/tester-main.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/android/tester-main.c b/android/tester-main.c
>> > index 30e1c59..ee3444f 100644
>> > --- a/android/tester-main.c
>> > +++ b/android/tester-main.c
>> > @@ -801,12 +801,12 @@ static bool match_data(struct step *step)
>> > return false;
>> > }
>> >
>> > - if (exp->store_srvc_handle)
>> > + if (exp->store_srvc_handle && step->callback_result.srvc_handle)
>> > memcpy(exp->store_srvc_handle,
>> > step->callback_result.srvc_handle,
>> > sizeof(*exp->store_srvc_handle));
>> >
>> > - if (exp->store_char_handle)
>> > + if (exp->store_char_handle && step->callback_result.char_handle)
>> > memcpy(exp->store_char_handle,
>> > step->callback_result.char_handle,
>> > sizeof(*exp->store_char_handle));
>> > --
>> > 1.9.1
>> >
>> > --
>> > 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
>>
>>
>> I think it can cause test cause bad behavior by not setting variable
>> if test step is obliged to do it.
>
> Do you mean that memcpy(src, 0, size) stores something useful?
>
I mean that memcpy(src, 0, size) shows us that something with test
step is wrong, during test case development it should be analyzed and
fixed.
It's only possible to expect storing srvc_handle while we get
service_added callback - this callback always returns service handle -
similar for characteristics.
I prefer to handle it in this way as I mentioned in previous comment.
> Best regards
> Andrei Emeltchenko
>
>> Test cases are defined in this way that one step e.g. 'A' stores
>> service handle and step 'B' uses this handle.
>> If value is not set it cause wrong test case behavior in next steps.
>> This check of exp (if store of srvc handle is mandatory for this step)
>> and step srvc handle (received in callback) only mask problem
>> with storing attribute handle value - in result it shows only that
>> memory was not duplicated well while copying data from incoming
>> callback handler.
>> Summarizing - we consciously force writing this srvc, char handle
>> value, if something is wrong - that means something with test
>> case is wrong and it should be fixed.
>>
>> Best way to handle it - static analyzer and this logic safe is to do
>> it in this way:
>> "if (exp->store_srvc_handle) {
>> if (!step->callback_result.srvc_handle) {
>> tester_debug("wrong srvc handle received in callback");
>> return false;
>> } else {
>> memcpy(exp->store_char_handle,
>> step->callback_result.char_handle,
>>
>> sizeof(*exp->store_char_handle));
>> }
>> }
>>
>> Best Regards,
>> Grzegorz
Best regards,
Grzegorz
Hi Grzegorz,
On Thu, Oct 09, 2014 at 12:56:19PM +0200, Grzegorz Kolodziejczyk wrote:
> Hi Andrei,
>
> On 9 October 2014 12:16, Andrei Emeltchenko
> <[email protected]> wrote:
> > From: Andrei Emeltchenko <[email protected]>
> >
> > Silence code analyzers and follow strict C standard where passing NULL
> > pointer results in undefined behaviour.
> > ---
> > android/tester-main.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/android/tester-main.c b/android/tester-main.c
> > index 30e1c59..ee3444f 100644
> > --- a/android/tester-main.c
> > +++ b/android/tester-main.c
> > @@ -801,12 +801,12 @@ static bool match_data(struct step *step)
> > return false;
> > }
> >
> > - if (exp->store_srvc_handle)
> > + if (exp->store_srvc_handle && step->callback_result.srvc_handle)
> > memcpy(exp->store_srvc_handle,
> > step->callback_result.srvc_handle,
> > sizeof(*exp->store_srvc_handle));
> >
> > - if (exp->store_char_handle)
> > + if (exp->store_char_handle && step->callback_result.char_handle)
> > memcpy(exp->store_char_handle,
> > step->callback_result.char_handle,
> > sizeof(*exp->store_char_handle));
> > --
> > 1.9.1
> >
> > --
> > 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
>
>
> I think it can cause test cause bad behavior by not setting variable
> if test step is obliged to do it.
Do you mean that memcpy(src, 0, size) stores something useful?
Best regards
Andrei Emeltchenko
> Test cases are defined in this way that one step e.g. 'A' stores
> service handle and step 'B' uses this handle.
> If value is not set it cause wrong test case behavior in next steps.
> This check of exp (if store of srvc handle is mandatory for this step)
> and step srvc handle (received in callback) only mask problem
> with storing attribute handle value - in result it shows only that
> memory was not duplicated well while copying data from incoming
> callback handler.
> Summarizing - we consciously force writing this srvc, char handle
> value, if something is wrong - that means something with test
> case is wrong and it should be fixed.
>
> Best way to handle it - static analyzer and this logic safe is to do
> it in this way:
> "if (exp->store_srvc_handle) {
> if (!step->callback_result.srvc_handle) {
> tester_debug("wrong srvc handle received in callback");
> return false;
> } else {
> memcpy(exp->store_char_handle,
> step->callback_result.char_handle,
>
> sizeof(*exp->store_char_handle));
> }
> }
>
> Best Regards,
> Grzegorz
Hi Andrei,
On 9 October 2014 12:16, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> Silence code analyzers and follow strict C standard where passing NULL
> pointer results in undefined behaviour.
> ---
> android/tester-main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/android/tester-main.c b/android/tester-main.c
> index 30e1c59..ee3444f 100644
> --- a/android/tester-main.c
> +++ b/android/tester-main.c
> @@ -801,12 +801,12 @@ static bool match_data(struct step *step)
> return false;
> }
>
> - if (exp->store_srvc_handle)
> + if (exp->store_srvc_handle && step->callback_result.srvc_handle)
> memcpy(exp->store_srvc_handle,
> step->callback_result.srvc_handle,
> sizeof(*exp->store_srvc_handle));
>
> - if (exp->store_char_handle)
> + if (exp->store_char_handle && step->callback_result.char_handle)
> memcpy(exp->store_char_handle,
> step->callback_result.char_handle,
> sizeof(*exp->store_char_handle));
> --
> 1.9.1
>
> --
> 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
I think it can cause test cause bad behavior by not setting variable
if test step is obliged to do it.
Test cases are defined in this way that one step e.g. 'A' stores
service handle and step 'B' uses this handle.
If value is not set it cause wrong test case behavior in next steps.
This check of exp (if store of srvc handle is mandatory for this step)
and step srvc handle (received in callback) only mask problem
with storing attribute handle value - in result it shows only that
memory was not duplicated well while copying data from incoming
callback handler.
Summarizing - we consciously force writing this srvc, char handle
value, if something is wrong - that means something with test
case is wrong and it should be fixed.
Best way to handle it - static analyzer and this logic safe is to do
it in this way:
"if (exp->store_srvc_handle) {
if (!step->callback_result.srvc_handle) {
tester_debug("wrong srvc handle received in callback");
return false;
} else {
memcpy(exp->store_char_handle,
step->callback_result.char_handle,
sizeof(*exp->store_char_handle));
}
}
Best Regards,
Grzegorz