[SSSD] [PATCH] server-tests: do not use complex condition in assert

Pavel Březina pbrezina at redhat.com
Wed Jan 14 20:24:49 UTC 2015


On 01/14/2015 04:25 PM, Lukas Slebodnik wrote:
> On (14/01/15 15:57), Pavel Březina wrote:
>> On 01/13/2015 12:48 PM, Lukas Slebodnik wrote:
>>> On (13/01/15 12:37), Jakub Hrozek wrote:
>>>> On Tue, Jan 13, 2015 at 12:28:22PM +0100, Lukas Slebodnik wrote:
>>>>> On (13/01/15 11:14), Jakub Hrozek wrote:
>>>>>> On Fri, Jan 09, 2015 at 05:32:41PM +0100, Lukas Slebodnik wrote:
>>>>>>> On (09/01/15 15:18), Pavel Březina wrote:
>>>>>>>> On 01/09/2015 03:17 PM, Pavel Březina wrote:
>>>>>>>>> If OR is used in assert macro we never know what part of the condition
>>>>>>>>> was not met from a failure message directly.
>>>>>>>>>
>>>>>>>>> I'm seeing a failure in server-tests, because pid number of sssd process
>>>>>>>>> is greater than 0xffff, it is bigger than 100k. Where does this constant
>>>>>>>>> come from?
>>>>>>> cat /proc/sys/kernel/pid_max
>>>>>>>
>>>>>>> On my system i is 0x8000
>>>>>>> echo "obase=16; `cat /proc/sys/kernel/pid_max`" | bc
>>>>>>>
>>>>>>>>>
>>>>>>>>> [ RUN      ] test_run_as_root_daemon
>>>>>>>>> tmp > 0xFFFF
>>>>>>>>> /home/pbrezina/workspace/sssd/src/tests/cwrap/test_server.c:79: error:
>>>>>>>>> Failure
>>>>>>>>
>>>>>>>> And now with the patch.
>>>>>>>>
>>>>>>>
>>>>>>> >From 7d5912f3206205dc24537f3640e9a8701995ceaf Mon Sep 17 00:00:00 2001
>>>>>>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>>>>>>> Date: Fri, 9 Jan 2015 15:12:31 +0100
>>>>>>>> Subject: [PATCH] server-tests: do not use complex condition in assert
>>>>>>>>
>>>>>>>> If OR is used in assert macro we never know what part of the
>>>>>>>> condition was not met from a failure message directly.
>>>>>>>> ---
>>>>>>>> src/tests/cwrap/test_server.c | 4 +++-
>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/src/tests/cwrap/test_server.c b/src/tests/cwrap/test_server.c
>>>>>>>> index d0aeac47d0b067fdbc3399037c0a74f150337a23..560d73330f8caa5ba9706d13bae296594bb2841f 100644
>>>>>>>> --- a/src/tests/cwrap/test_server.c
>>>>>>>> +++ b/src/tests/cwrap/test_server.c
>>>>>>>> @@ -75,7 +75,9 @@ static void wait_for_bg_server(const char *pidfile)
>>>>>>>>      buf[sizeof(buf) - 1] = '\0';
>>>>>>>>
>>>>>>>>      tmp = strtol(buf, NULL, 10);
>>>>>>>> -    assert_false(tmp == 0 || tmp > 0xFFFF || errno == ERANGE);
>>>>>>>> +    assert_false(tmp == 0);
>>>>>>>> +    assert_false(tmp > 0xFFFF);
>>>>>>>                           ^^^^^^
>>>>>>>                      I think we should not rely on such constant
>>>>>>> because  you can set the value higher
>>>>>>> (up to 2^22 on 32-bit machines: 4,194,304) with:
>>>>>>> echo 4194303 > /proc/sys/kernel/pid_max
>>>>>>>
>>>>>>> LS
>>>>>>
>>>>>> git blame points to me and IIRC I took part of these tests from
>>>>>> uid_wrapper code without too much thinking about them.
>>>>>>
>>>>>> I think we can just remove the 0xFFFF part completely.
>>>>> I agree.
>>>>>
>>>>> or we can use our function strtouint32 to convert number instead of strtol.
>>>>> There is assumption sizeof(pid_t) == sizeof(uint32_t)
>>>>
>>>> Good idea, can either you or Pavel B. prepare a patch?
>>> I let Pavel to finish his work :-)
>>>
>>> LS
>> Attached.
>>
>>
>
>>From e31737f6488528be051ca4268b8712050c316b95 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Fri, 9 Jan 2015 15:12:31 +0100
>> Subject: [PATCH 1/3] server-tests: do not use complex condition in assert
>>
>> If OR is used in assert macro we never know what part of the
>> condition was not met from a failure message directly.
>> ---
>> src/tests/cwrap/test_server.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/tests/cwrap/test_server.c b/src/tests/cwrap/test_server.c
>> index d0aeac47d0b067fdbc3399037c0a74f150337a23..560d73330f8caa5ba9706d13bae296594bb2841f 100644
>> --- a/src/tests/cwrap/test_server.c
>> +++ b/src/tests/cwrap/test_server.c
>> @@ -75,7 +75,9 @@ static void wait_for_bg_server(const char *pidfile)
>>      buf[sizeof(buf) - 1] = '\0';
>>
>>      tmp = strtol(buf, NULL, 10);
>> -    assert_false(tmp == 0 || tmp > 0xFFFF || errno == ERANGE);
>> +    assert_false(tmp == 0);
>> +    assert_false(tmp > 0xFFFF);
>> +    assert_false(errno == ERANGE);
>>
>>      pid = (pid_t) (tmp & 0xFFFF);
>>
>> --
>> 1.9.3
>>
>
>>From c12b5b8c1bd825d2526c58b2b9b89e3d6b02dc02 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 14 Jan 2015 15:52:40 +0100
>> Subject: [PATCH 2/3] server-tests: use strtouint32 instead strtol
>>
>> PID may be greater than 0xffff thus we remove this check but it is
>> supposed to be in range of uint32.
>> ---
>> src/tests/cwrap/test_server.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/tests/cwrap/test_server.c b/src/tests/cwrap/test_server.c
>> index 560d73330f8caa5ba9706d13bae296594bb2841f..323c17d45407cfdd6fa3065b065427b4309b9ac6 100644
>> --- a/src/tests/cwrap/test_server.c
>> +++ b/src/tests/cwrap/test_server.c
>> @@ -26,6 +26,7 @@
>>
>> #include <popt.h>
>> #include "util/util.h"
>> +#include "util/strtonum.h"
>> #include "tests/cmocka/common_mock.h"
>>
>> static void wait_for_fg_server(pid_t pid)
>> @@ -44,7 +45,7 @@ static void wait_for_fg_server(pid_t pid)
>> static void wait_for_bg_server(const char *pidfile)
>> {
>>      int fd;
>> -    long tmp;
>> +    uint32_t tmp;
>>      char buf[16];
>>      pid_t pid;
>>      int ret;
>> @@ -74,12 +75,11 @@ static void wait_for_bg_server(const char *pidfile)
>>
>>      buf[sizeof(buf) - 1] = '\0';
>>
>> -    tmp = strtol(buf, NULL, 10);
>> +    tmp = strtouint32(buf, NULL, 10);
>>      assert_false(tmp == 0);
>> -    assert_false(tmp > 0xFFFF);
>>      assert_false(errno == ERANGE);
>>
>> -    pid = (pid_t) (tmp & 0xFFFF);
>> +    pid = (pid_t) (tmp);
>>
>>      /* Make sure the daemon goes away! */
>>      ret = kill(pid, SIGTERM);
>> --
>> 1.9.3
>>
>
>>From 52460bd061607b4d853831298d268197f9e86a25 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 14 Jan 2015 15:54:41 +0100
>> Subject: [PATCH 3/3] server-tests: do not test for specific errno value
>>
>> Even though strtouint32 sets errno to 0 and uses only
>> ERANGE error code we should not rely on it.
>> ---
>> src/tests/cwrap/test_server.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/tests/cwrap/test_server.c b/src/tests/cwrap/test_server.c
>> index 323c17d45407cfdd6fa3065b065427b4309b9ac6..84f3925c37203ade07b909267ed674844fb34390 100644
>> --- a/src/tests/cwrap/test_server.c
>> +++ b/src/tests/cwrap/test_server.c
>> @@ -75,9 +75,10 @@ static void wait_for_bg_server(const char *pidfile)
>>
>>      buf[sizeof(buf) - 1] = '\0';
>>
>> +    errno = 0;
>>      tmp = strtouint32(buf, NULL, 10);
>>      assert_false(tmp == 0);
>> -    assert_false(errno == ERANGE);
>> +    assert_true(errno == 0);
> NACK
> Please use assert_int_equal. In case of error we will be able to see value of
> errno. The same applies to asser "tmp == 0" changed in previous patch.

Good idea. New patch is attached.

> BTW: Do we really need 3 patches for such simple change?

I personally prefer to put different changes to different patch even 
though it is small and in the same code. But I don't really mind having 
it as a single patch. I squashed it together.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-server-tests-use-strtouint32-instead-strtol.patch
Type: text/x-patch
Size: 1623 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150114/18c4b97d/attachment.bin>


More information about the sssd-devel mailing list