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

Lukas Slebodnik lslebodn at redhat.com
Wed Jan 14 15:25:26 UTC 2015


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.

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

LS



More information about the sssd-devel mailing list