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?= <pbrez...@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?= <pbrez...@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?= <pbrez...@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?= <pbrez...@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.
>From ccc4cb6961a66a8f2c60596c7c933d0fb26de62b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Fri, 9 Jan 2015 15:12:31 +0100
Subject: [PATCH] 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.
There are also some improvements to get more information from
assertions.
---
src/tests/cwrap/test_server.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/tests/cwrap/test_server.c b/src/tests/cwrap/test_server.c
index d0aeac47d0b067fdbc3399037c0a74f150337a23..55ef2ecae852199b7b9e5300b56fdcb751d0d31c 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,10 +75,12 @@ 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);
+ errno = 0;
+ tmp = strtouint32(buf, NULL, 10);
+ assert_int_not_equal(tmp, 0);
+ assert_int_equal(errno, 0);
- pid = (pid_t) (tmp & 0xFFFF);
+ pid = (pid_t) (tmp);
/* Make sure the daemon goes away! */
ret = kill(pid, SIGTERM);
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
