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(a)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(a)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(a)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(a)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