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