On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:
On (05/02/16 17:30), Michal Židek wrote:
> On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
>> On (05/02/16 16:56), Pavel Reichl wrote:
>>> Hopefully the last one.
>>
>> >From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001
>>> From: Pavel Reichl <preichl(a)redhat.com>
>>> Date: Fri, 22 Jan 2016 08:34:14 -0500
>>> Subject: [PATCH] IDMAP: Add test to validate off by one bug
>>>
>>> Resolves:
>>>
https://fedorahosted.org/sssd/ticket/2922
>>> ---
>>> src/tests/cmocka/test_sss_idmap.c | 125
++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 125 insertions(+)
>>>
>>> diff --git a/src/tests/cmocka/test_sss_idmap.c
b/src/tests/cmocka/test_sss_idmap.c
>>> index
00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644
>>> --- a/src/tests/cmocka/test_sss_idmap.c
>>> +++ b/src/tests/cmocka/test_sss_idmap.c
>>> @@ -43,6 +43,17 @@
>>> #define TEST_OFFSET 1000000
>>> #define TEST_OFFSET_STR "1000000"
>>>
>>> +#define TEST_2922_MIN_ID 1842600000
>>> +#define TEST_2922_MAX_ID 1842799999
>>> +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
>>> +
>>> +#define TEST_2922_DFL_SLIDE 9212
>>> +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
>>> +/* Last SID = first SID + (default) rangesize -1 */
>>> +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999"
>>> +/* Last SID = first SID + rangesize */
>>> +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
>>> +
>> We should prefer to use constants instead of #defines.
>> Constants are more type safe.
>>
>
> Do you want Pavel to change it? If so, then I would prefer
> at least the strings to remain as #defines. It is more
> comfortable to concatenate string literals. But honestly
> I am ok with the numbers to remain as #defines as well,
> but I agree with your point in general.
>
Yes, please.
Define can still be used for simplification of string initialisation
e.g.
#define DOM_SID "S-1-2-3-4"
const char TEST_2922_LAST_SID[] = DOM_SID"-99";
const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";
Yes, they can. But do you require it? As I said I would
prefer the strings to remain #defines, but it is not strong
preference.
>>
>>
>>> struct test_ctx {
>>> TALLOC_CTX *mem_idmap;
>>> struct sss_idmap_ctx *idmap_ctx;
>>> @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool
external_mapping,
>>> return 0;
>>> }
>>>
>>> +static int setup_ranges_2922(struct test_ctx *test_ctx)
>>> +{
>>> + struct sss_idmap_range range;
>>> + enum idmap_error_code err;
>>> + const char *name;
>>> + const char *sid;
>>> + /* Pick a new slice. */
>>> + id_t slice_num = -1;
>>> +
>>> + if (test_ctx == NULL) {
>>> + DEBUG(SSSDBG_FATAL_FAILURE,
>>> + "test context is NULL\n");
>>> + return 1;
>>> + }
>>> +
>>> + name = TEST_DOM_NAME;
>>> + sid = TEST_DOM_SID;
>>> +
>>> + err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid,
&slice_num,
>>> + &range);
>>> + if (err != IDMAP_SUCCESS) {
>>> + DEBUG(SSSDBG_FATAL_FAILURE,
>>> + "sss_idmap_calculate_range failed: exp: [%d] but got
[%d]\n",
>>> + IDMAP_SUCCESS, err);
>>> + return 1;
>> Please use different numbers for different failures.
>
> I think it is OK to use the same number in tests, as long as
> the debug message is present. We can identify the error
> based on stderr messages.
>
No it's not OK to have same return code.
The return codes have to be unique.
They do not need to be unique here. Compare:
[ RUN ] test_map_id_2922
(Fri Feb 5 19:00:17:723118 2016) [sssd]
[test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL
Could not run the test - check test fixtures
[ ERROR ] test_map_id_2922
With this:
[ RUN ] test_map_id_2922
(Fri Feb 5 19:01:17:376602 2016) [sssd]
[test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL
Could not run the test - check test fixtures
[ ERROR ] test_map_id_2922
I returned different error code in both cases but the result
is the same.
You might easily overlook as part of review that messages are the
same
and you will don't know where it failed. (copy & paste problem.
It's much simpler to review that return codes are unique.
LS
But the DEBUG macro will tell you what function the message is
from. So it does not matter that much what return code you
use here. This is just a setup function for
a test and all non-zero codes are treated the same way. We
can use something like EINVAL if it is something with bad
value or whatever we find suitable in errno.h if you think
that 1 is too magic value, but since any value will be treated
the same by the cmocka, it does not really matter here
and returning 1 is just fine IMO. It really is just a fixture for test.
The debug message is more important than the error code.
Michal