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?
[ RUN ] test_run_as_root_daemon tmp > 0xFFFF /home/pbrezina/workspace/sssd/src/tests/cwrap/test_server.c:79: error: Failure
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?
[ 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.
On 01/09/2015 03:18 PM, 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?
[ 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.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
ACK
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@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
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@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 constantbecause 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.
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@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 constantbecause 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)
LS
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@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 constantbecause 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?
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@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 constantbecause 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
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@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 constantbecause 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.
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@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@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@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@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
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@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@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@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@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.
On (14/01/15 21:24), Pavel Březina wrote:
On 01/14/2015 04:25 PM, Lukas Slebodnik wrote:
On (14/01/15 15:57), Pavel Březina wrote:
From 52460bd061607b4d853831298d268197f9e86a25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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.
Generally, I agree with you but splitting "7 insertions(+), 4 deletions(-)" into 3 patches was to much.
From ccc4cb6961a66a8f2c60596c7c933d0fb26de62b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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);
Thank you very much for all changes. ACK
http://sssd-ci.duckdns.org/logs/job/5/99/summary.html
LS
On Thu, Jan 15, 2015 at 07:50:35AM +0100, Lukas Slebodnik wrote:
On (14/01/15 21:24), Pavel Březina wrote:
On 01/14/2015 04:25 PM, Lukas Slebodnik wrote:
On (14/01/15 15:57), Pavel Březina wrote:
From 52460bd061607b4d853831298d268197f9e86a25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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.
Generally, I agree with you but splitting "7 insertions(+), 4 deletions(-)" into 3 patches was to much.
From ccc4cb6961a66a8f2c60596c7c933d0fb26de62b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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);
Thank you very much for all changes. ACK
http://sssd-ci.duckdns.org/logs/job/5/99/summary.html
LS
* master: fd52e9e51bdfe00e035e0ab08ce9a6a5d6b7a974
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Pavel Reichl