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