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(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.
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.