[LTP] [RFC PATCH 0/1] Fix return value checks for posix apis

[LTP] [RFC PATCH 0/1] Fix return value checks for posix apis

[Sandeep Patil]

This is an RFC patch to demonstrate that most of the pthread_* calls
in the open_posix_testsuite seem to be doing the return value checks
incorrectly. Most posix_* apis return positive errno on failure and 0 on
success.

PATCH 1/1 demonstrates the fixes needed for pthread_sigmask/6-1 for
example. This pattern is fairly widespread in open_posix_testsuite.

After going through the documentation in the project, I wasn't sure if I
tested this correctly. After building, I tried the following

$ cd testcases/open_posix_testsuite
$ ./bin/run_tesit.sh conformance/interfaces/pthread_sigmask pthread_sigmask_6-1.run-test

...and that keeps running into test being skipped due to missing file.
(I do have pthread_sigmask_6-1.run-test in place).

So, consider this build tested only for now. I'd love to know how we can
test this and if we are still using the testsuite, then I am happy to
fix rest of the places where its broken.

Sandeep Patil (1):
  open_posix_testsuite/pthread_sigmask: fix return value checks

 .../interfaces/pthread_sigmask/6-1.c          | 69 +++++++++++--------
 1 file changed, 40 insertions(+), 29 deletions(-)

-- 
2.21.0.1020.gf2820cf01a-goog

[LTP] [RFC PATCH 1/1] open_posix_testsuite/pthread_sigmask: fix return value checks

[Sandeep Patil]

Most posix_ functions return 0 on success and positive errno on failure.
For all incorrect error checks in pthread_sigmask/6-1 test to make sure
we only check against the 0 return value.

Consistently use error(3) and replace all occurences of perror()
at the same time.

Signed-off-by: Sandeep Patil <sspatil@android.com>
---
 .../interfaces/pthread_sigmask/6-1.c          | 69 +++++++++++--------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/testcases/open_posix_testsuite/conformance/interfaces/pthread_sigmask/6-1.c b/testcases/open_posix_testsuite/conformance/interfaces/pthread_sigmask/6-1.c
index d2eb1827a..c04069d5d 100644
--- a/testcases/open_posix_testsuite/conformance/interfaces/pthread_sigmask/6-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/pthread_sigmask/6-1.c
@@ -27,6 +27,8 @@
       unexpected function failure.
 */
 
+#include <errno.h>
+#include <error.h>
 #include <pthread.h>
 #include <signal.h>
 #include <stdio.h>
@@ -43,6 +45,7 @@ void *a_thread_func()
 {
 	struct sigaction act;
 	sigset_t set1, set2, pending_set;
+	int status;
 
 	sigemptyset(&set1);
 	sigaddset(&set1, SIGABRT);
@@ -55,46 +58,52 @@ void *a_thread_func()
 	act.sa_flags = 0;
 	sigemptyset(&act.sa_mask);
 
-	if (sigaction(SIGABRT, &act, 0) == -1) {
-		perror("Unexpected error while attempting to setup test "
+	status = sigaction(SIGABRT, &act, 0);
+	if (status < 0) {
+		error(0, errno, "Unexpected error while attempting to setup test "
 		       "pre-conditions");
 		pthread_exit((void *)1);
 	}
 
-	if (sigaction(SIGALRM, &act, 0) == -1) {
-		perror("Unexpected error while attempting to setup test "
+	status = sigaction(SIGALRM, &act, 0);
+	if (status < 0) {
+		error(0, errno, "Unexpected error while attempting to setup test "
 		       "pre-conditions");
 		pthread_exit((void *)1);
 	}
 
-	if (pthread_sigmask(SIG_SETMASK, &set1, NULL) == -1) {
-		perror
-		    ("Unexpected error while attempting to use pthread_sigmask.\n");
+	status = pthread_sigmask(SIG_SETMASK, &set1, NULL);
+	if (status != 0) {
+		error(0, status, "Unexpected error while attempting to use "
+			"pthread_sigmask.\n");
 		pthread_exit((void *)1);
 	}
 
-	if (pthread_sigmask(SIG_UNBLOCK, &set2, NULL) == -1) {
-		perror
-		    ("Unexpected error while attempting to use pthread_sigmask.\n");
+	status = pthread_sigmask(SIG_UNBLOCK, &set2, NULL);
+	if (status != 0) {
+		error(0, status, "Unexpected error while attempting to use "
+			"pthread_sigmask.\n");
 		pthread_exit((void *)1);
 	}
 
-	if (raise(SIGALRM) == -1) {
-		perror("Unexpected error while attempting to setup test "
-		       "pre-conditions");
+	status = raise(SIGALRM);
+	if (status != 0) {
+		error(0, errno, "Unexpected error while attempting to setup "
+			"test pre-conditions");
 		pthread_exit((void *)1);
 	}
 
 	if (!handler_called) {
-		printf
-		    ("FAIL: Handler was not called for even though signal was removed from the signal mask\n");
+		printf("FAIL: Handler was not called for even though signal "
+			"was removed from the signal mask\n");
 		pthread_exit((void *)-1);
 	}
 
 	handler_called = 0;
-	if (raise(SIGABRT) == -1) {
-		perror("Unexpected error while attempting to setup test "
-		       "pre-conditions");
+	status = raise(SIGABRT);
+	if (status != 0) {
+		error(0, errno, "Unexpected error while attempting to setup "
+			"test pre-conditions");
 		pthread_exit((void *)1);
 	}
 
@@ -104,18 +113,20 @@ void *a_thread_func()
 		pthread_exit((void *)-1);
 	}
 
-	if (sigpending(&pending_set) == -1) {
-		perror("Unexpected error while attempting to use sigpending\n");
+	status = sigpending(&pending_set);
+	if (status != 0) {
+		error(0, errno, "Unexpected error while attempting to use "
+			"sigpending\n");
 		pthread_exit((void *)1);
 	}
 
 	if (sigismember(&pending_set, SIGABRT) != 1) {
-		perror("FAIL: sigismember did not return 1\n");
+		error(0, errno, "FAIL: sigismember did not return 1\n");
 		pthread_exit((void *)-1);
 	}
 
 	if (sigismember(&pending_set, SIGALRM) != 0) {
-		perror("FAIL: sigismember did not return 0\n");
+		error(0, errno, "FAIL: sigismember did not return 0\n");
 		pthread_exit((void *)-1);
 	}
 
@@ -125,19 +136,19 @@ void *a_thread_func()
 
 int main(void)
 {
-
+	int status;
 	int *thread_return_value;
 
 	pthread_t new_thread;
 
-	if (pthread_create(&new_thread, NULL, a_thread_func, NULL) != 0) {
-		perror("Error creating new thread\n");
-		return PTS_UNRESOLVED;
+	status = pthread_create(&new_thread, NULL, a_thread_func, NULL);
+	if (status != 0) {
+		error(PTS_UNRESOLVED, status, "Error creating new thread\n");
 	}
 
-	if (pthread_join(new_thread, (void *)&thread_return_value) != 0) {
-		perror("Error in pthread_join()\n");
-		return PTS_UNRESOLVED;
+	status = pthread_join(new_thread, (void *)&thread_return_value);
+	if (status != 0) {
+		error(PTS_UNRESOLVED, status, "Error in pthread_join()\n");
 	}
 
 	if ((long)thread_return_value != 0) {
-- 
2.21.0.1020.gf2820cf01a-goog

[LTP] [RFC PATCH 1/1] open_posix_testsuite/pthread_sigmask: fix return value checks

[Cyril Hrubis]

Hi!
> Consistently use error(3) and replace all occurences of perror()
> at the same time.

The error(3) is GNU extension we cannot use it in a POSIX testsuite as
such, I guess that we will have to add custom error reporting functions
(in a separate patch) to the open_posix_testsuite/include/posixtest.h
header...

Other than that the patch is obviously correct.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH 1/1] open_posix_testsuite/pthread_sigmask: fix return value checks

[Sandeep Patil]

On Mon, May 20, 2019 at 11:53:57AM +0200, Cyril Hrubis wrote:
> Hi!
> > Consistently use error(3) and replace all occurences of perror()
> > at the same time.
> 
> The error(3) is GNU extension we cannot use it in a POSIX testsuite as
> such, I guess that we will have to add custom error reporting functions
> (in a separate patch) to the open_posix_testsuite/include/posixtest.h
> header...
> 
> Other than that the patch is obviously correct.

Thanks you want me to add an error(3)-like function there?
I guess I can do that and then start changing all tests.

Thanks for pointing out the GNU-extension, didn't realize that.

- ssp

[LTP] [RFC PATCH 1/1] open_posix_testsuite/pthread_sigmask: fix return value checks

[Cyril Hrubis]

Hi!
> > The error(3) is GNU extension we cannot use it in a POSIX testsuite as
> > such, I guess that we will have to add custom error reporting functions
> > (in a separate patch) to the open_posix_testsuite/include/posixtest.h
> > header...
> > 
> > Other than that the patch is obviously correct.
> 
> Thanks you want me to add an error(3)-like function there?
> I guess I can do that and then start changing all tests.

If you think that error(3) like API is best fit then we can go ahead and
use it for posix testsuite. You can also think about it a bit and maybe
create something that will fit the purpose slightly better.

Only thing I would have avoided is to give the function names that are
likely to collide with test code. I would expect that there are several
tests that use error as a variable, so if we decide to name the function
that way we should prefix it with pts_ e.g. pts_error().

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH 0/1] Fix return value checks for posix apis

[Cyril Hrubis]

Hi!
> This is an RFC patch to demonstrate that most of the pthread_* calls
> in the open_posix_testsuite seem to be doing the return value checks
> incorrectly. Most posix_* apis return positive errno on failure and 0 on
> success.

This is unfortunately common misconception, also I guess you meant
pthread_* APIs here.

> PATCH 1/1 demonstrates the fixes needed for pthread_sigmask/6-1 for
> example. This pattern is fairly widespread in open_posix_testsuite.
> 
> After going through the documentation in the project, I wasn't sure if I
> tested this correctly. After building, I tried the following
> 
> $ cd testcases/open_posix_testsuite
> $ ./bin/run_tesit.sh conformance/interfaces/pthread_sigmask pthread_sigmask_6-1.run-test
> 
> ...and that keeps running into test being skipped due to missing file.
> (I do have pthread_sigmask_6-1.run-test in place).

Actually I always just run the binary, so in this case doing
./pthread_sigmask_6-1.run-test should suffice.

> So, consider this build tested only for now. I'd love to know how we can
> test this and if we are still using the testsuite, then I am happy to
> fix rest of the places where its broken.

Sounds good.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH 0/1] Fix return value checks for posix apis

[Sandeep Patil]

On Mon, May 20, 2019 at 11:49:49AM +0200, Cyril Hrubis wrote:
> Hi!
> > This is an RFC patch to demonstrate that most of the pthread_* calls
> > in the open_posix_testsuite seem to be doing the return value checks
> > incorrectly. Most posix_* apis return positive errno on failure and 0 on
> > success.
> 
> This is unfortunately common misconception, also I guess you meant
> pthread_* APIs here.

yes, pthread_* APIs.

> 
> > PATCH 1/1 demonstrates the fixes needed for pthread_sigmask/6-1 for
> > example. This pattern is fairly widespread in open_posix_testsuite.
> > 
> > After going through the documentation in the project, I wasn't sure if I
> > tested this correctly. After building, I tried the following
> > 
> > $ cd testcases/open_posix_testsuite
> > $ ./bin/run_tesit.sh conformance/interfaces/pthread_sigmask pthread_sigmask_6-1.run-test
> > 
> > ...and that keeps running into test being skipped due to missing file.
> > (I do have pthread_sigmask_6-1.run-test in place).
> 
> Actually I always just run the binary, so in this case doing
> ./pthread_sigmask_6-1.run-test should suffice.

Ok, I'll try that, but IIRC it did nothing on my laptop. I'll retry.

> 
> > So, consider this build tested only for now. I'd love to know how we can
> > test this and if we are still using the testsuite, then I am happy to
> > fix rest of the places where its broken.
> 
> Sounds good.

Thanks for the review.

- ssp

[LTP] [RFC PATCH 0/1] Fix return value checks for posix apis

[Cyril Hrubis]

Hi!
> > > PATCH 1/1 demonstrates the fixes needed for pthread_sigmask/6-1 for
> > > example. This pattern is fairly widespread in open_posix_testsuite.
> > > 
> > > After going through the documentation in the project, I wasn't sure if I
> > > tested this correctly. After building, I tried the following
> > > 
> > > $ cd testcases/open_posix_testsuite
> > > $ ./bin/run_tesit.sh conformance/interfaces/pthread_sigmask pthread_sigmask_6-1.run-test
> > > 
> > > ...and that keeps running into test being skipped due to missing file.
> > > (I do have pthread_sigmask_6-1.run-test in place).
> > 
> > Actually I always just run the binary, so in this case doing
> > ./pthread_sigmask_6-1.run-test should suffice.
> 
> Ok, I'll try that, but IIRC it did nothing on my laptop. I'll retry.

Looks like the test does not print anything unless there is a failure.

Well the convention in openposix testsuite I started to follow is to
print "Test PASSED" in such case, so this should be fixed (in a
separate patch) as well.

-- 
Cyril Hrubis
chrubis@suse.cz