[LTP] [PATCH 0/2] Updates to pthread_{barrier,cond}_destroy()

[LTP] [PATCH 0/2] Updates to pthread_{barrier,cond}_destroy()

[Richard Palethorpe]

Hello,

Both these tests are a little suspect as they are testing undefined
behavior. They also use calls to sleep() for sychronisation and assume setting
or getting an integer with the '=' operator is atomic.

Even though they are testing undefined behavior, I think they are still useful
just to check how a particular implementation deals with these scenarios. I
have tried to relax the test as much as possible to avoid spurious
failures. Also, even though it is probably allowed by the spec, I would
consider a segfault to be a failure.

These tests do naturally include race conditions and waste time in calls to
sleep, we could mitigate them by using another synchronisation mechanism (e.g
TST_CHECKPOINT or fzsync), but not remove them completely. I'm not sure whether
introducing LTP library functions into the POSIX tests is a good idea or not.

Richard Palethorpe (2):
  POSIX: Allow pthread_barrier_destroy() to block
  POSIX: Allow pthread_cond_destroy to block

 .../interfaces/pthread_barrier_destroy/2-1.c       | 101 +++++++++++++--------
 .../pthread_cond_destroy/speculative/4-1.c         |  81 +++++++++--------
 2 files changed, 105 insertions(+), 77 deletions(-)

-- 
2.14.2

[LTP] [PATCH 1/2] POSIX: Allow pthread_barrier_destroy() to block

[Richard Palethorpe]

Newer glibc versions choose to block until a barrier's users have all
synchronised with it. This caused the test to hang forever, so a watchdog has
been introduced to complete the barrier after a timeout.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 .../interfaces/pthread_barrier_destroy/2-1.c       | 101 +++++++++++++--------
 1 file changed, 61 insertions(+), 40 deletions(-)

diff --git a/testcases/open_posix_testsuite/conformance/interfaces/pthread_barrier_destroy/2-1.c b/testcases/open_posix_testsuite/conformance/interfaces/pthread_barrier_destroy/2-1.c
index 7abb08ff8..a89b87f6e 100644
--- a/testcases/open_posix_testsuite/conformance/interfaces/pthread_barrier_destroy/2-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/pthread_barrier_destroy/2-1.c
@@ -1,23 +1,25 @@
 /*
  * Copyright (c) 2002, Intel Corporation. All rights reserved.
+ * Copyright (c) 2017, Richard Palethorpe <rpalethorpe@suse.com>
  * This file is licensed under the GPL license.  For the full content
  * of this license, see the COPYING file at the top level of this
  * source tree.
  *
  * pthread_barrier_destroy()
  *
- * The pthread_barrier_destroy() function may fail if:
- * [EBUSY] The implementation has detected an attempt to destroy a barrier while it is in
- * use (for example, while being used in a pthread_barrier_wait() call) by another
- * thread.
+ * Attempt to destroy a barrier which has a waiter. The standard recommends
+ * that EBUSY is returned, but the required behaviour is undefined. At the
+ * time of writing, glibc blocks until there are no threads waiting on the
+ * barrier.
  *
- * Note: This case will always PASS
+ * Hypothetically, on some implementations, the child could segfault or
+ * 'barrier_wait() could return success or EINVAL when 'barrier_destroy() is
+ * called. It is difficult to determine whether 'barrier_wait() has returned
+ * due to 'barrier_destroy() or some other reason. So we just report what has
+ * happened. Obviously a segfault will cause the test to crash, which is
+ * highly undesirable behaviour regardless of whether it is POSIX compliant.
  *
- * Steps:
- * 1. Main initialize barrier with count 2
- * 2. Main create a child thread
- * 3. Child thread call pthread_barrier_wait(), should block
- * 4. Main call pthread_barrier_destroy(), while child is blocking on the barrier
+ * Also see pthread_cond_destroy 4-1.
  */
 
 #define _XOPEN_SOURCE 600
@@ -28,13 +30,15 @@
 #include <signal.h>
 #include <string.h>
 #include <errno.h>
+#include <sched.h>
 #include "posixtest.h"
 
 static pthread_barrier_t barrier;
-static int thread_state;
+static volatile int thread_state;
 #define NOT_CREATED_THREAD 1
 #define ENTERED_THREAD 2
 #define EXITING_THREAD 3
+#define WAIT_LOOP 0xFFFFFF
 
 static void *fn_chld(void *arg)
 {
@@ -42,73 +46,90 @@ static void *fn_chld(void *arg)
 
 	pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
 
+	printf("child: barrier wait\n");
 	thread_state = ENTERED_THREAD;
+	rc = pthread_barrier_wait(&barrier);
+	if (rc == PTHREAD_BARRIER_SERIAL_THREAD)
+		printf("child: got PTHREAD_BARRIER_SERIAL_THREAD\n");
+	else if (rc != 0)
+		perror("child: pthread_barrier_wait");
+	else
+		printf("child: pthread_barrier_wait returned success");
 
-	/* Child should block here */
-	printf("child: barrier wait\n");
+	thread_state = EXITING_THREAD;
+	return arg;
+}
+
+static void *watchdog(void *arg)
+{
+	int rc = 0;
+
+	pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
+
+	sleep(1);
+	printf("watchdog: It appears pthread_barrier_destroy() is blocking\n");
 	rc = pthread_barrier_wait(&barrier);
 	if (rc != 0 && rc != PTHREAD_BARRIER_SERIAL_THREAD) {
-		printf("Error: child: pthread_barrier_wait() get unexpected "
-		       "return code : %d\n", rc);
+		perror("watchdog: pthread_barrier_wait");
 		exit(PTS_UNRESOLVED);
-	} else if (rc == PTHREAD_BARRIER_SERIAL_THREAD) {
-		printf("child: get PTHREAD_BARRIER_SERIAL_THREAD\n");
 	}
 
-	thread_state = EXITING_THREAD;
-	pthread_exit(0);
-	return NULL;
+	return arg;
 }
 
 int main(void)
 {
 	int cnt = 0;
 	int rc;
-	pthread_t child_thread;
+	pthread_t child_thread, watchdog_thread;
 
 	printf("main: Initialize barrier with count = 2\n");
 	if (pthread_barrier_init(&barrier, NULL, 2) != 0) {
-		printf("main: Error at pthread_barrier_init()\n");
+		perror("main: pthread_barrier_init");
 		return PTS_UNRESOLVED;
 	}
 
 	printf("main: create child thread\n");
 	thread_state = NOT_CREATED_THREAD;
 	if (pthread_create(&child_thread, NULL, fn_chld, NULL) != 0) {
-		printf("main: Error at pthread_create()\n");
+		perror("main: pthread_create");
+		return PTS_UNRESOLVED;
+	}
+
+	printf("main: create watchdog thread\n");
+	if (pthread_create(&watchdog_thread, NULL, watchdog, NULL) != 0) {
+		perror("main: Error at pthread_create");
 		return PTS_UNRESOLVED;
 	}
 
-	/* Expect the child to block */
 	cnt = 0;
-	do {
-		sleep(1);
-	} while (thread_state != EXITING_THREAD && cnt++ < 2);
+	for (cnt = 0; thread_state < ENTERED_THREAD && cnt < WAIT_LOOP; cnt++)
+		sched_yield();
+	/* Yield once more to increase the probability that the child thread
+	 * will call pthread_barrier_wait() before this thread reaches
+	 * pthread_barrier_destroy().
+	 */
+	sched_yield();
 
 	if (thread_state == EXITING_THREAD) {
-		/* child thread did not block */
-		printf("Test FAILED: child thread did not block on "
-		       "pthread_barrier_wait()\n");
+		printf("Test FAILED: child thread did not block on pthread_barrier_wait()\n");
 		exit(PTS_FAIL);
-	} else if (thread_state != ENTERED_THREAD) {
-		printf("Unexpected thread state\n");
+	} else if (thread_state == NOT_CREATED_THREAD) {
+		printf("Child thread did not start (quick enough)\n");
 		exit(PTS_UNRESOLVED);
 	}
 
 	printf("main: destroy barrier while child is waiting\n");
-
 	rc = pthread_barrier_destroy(&barrier);
 
-	if (rc == EBUSY) {
-		printf("main: correctly got EBUSY\n");
-		printf("Test PASSED\n");
-	} else {
+	if (rc != EBUSY) {
 		printf("main: got return code: %d, %s\n", rc, strerror(rc));
-		printf
-		    ("Test PASSED: Note*: Expected EBUSY, but standard says 'may' fail.\n");
+		printf("Note: POSIX recommends returning EBUSY\n");
 	}
 
-	/* Cleanup (cancel thread in case it is still blocking */
+	printf("Test PASSED\n");
+
+	pthread_cancel(watchdog_thread);
 	pthread_cancel(child_thread);
 
 	return PTS_PASS;
-- 
2.14.2

[LTP] [PATCH 2/2] POSIX: Allow pthread_cond_destroy to block

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 .../pthread_cond_destroy/speculative/4-1.c         | 81 ++++++++++++----------
 1 file changed, 44 insertions(+), 37 deletions(-)

diff --git a/testcases/open_posix_testsuite/conformance/interfaces/pthread_cond_destroy/speculative/4-1.c b/testcases/open_posix_testsuite/conformance/interfaces/pthread_cond_destroy/speculative/4-1.c
index 9df7c1993..6c4464cf4 100644
--- a/testcases/open_posix_testsuite/conformance/interfaces/pthread_cond_destroy/speculative/4-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/pthread_cond_destroy/speculative/4-1.c
@@ -1,19 +1,17 @@
 /*
  * Copyright (c) 2004, QUALCOMM Inc. All rights reserved.
+ * Copyright (c) 2017, Richard Palethorpe <rpalethorpe@suse.com>
  * Created by:  abisain REMOVE-THIS AT qualcomm DOT com
  * This file is licensed under the GPL license.  For the full content
  * of this license, see the COPYING file at the top level of this
  * source tree.
-
- * Test that when  pthread_cond_destroy()
- *   is called on a cond that some thread is waiting, then it returns
- *   EBUSY
-
- * Steps:
- * 1. Create a condvar
- * 2. Create a thread and make it wait on the condvar
- * 3. Try to destroy the cond var in main
- * 4. Check that pthread_cond_destroy returns EBUSY
+ *
+ * Test that EBUSY is returned when pthread_cond_destroy() is called on a cond
+ * var that has waiters. POSIX only recommends this behaviour, the required
+ * behaviour is undefined.
+ *
+ * This test is very similar to pthread_barrier_destroy 2-1 which has more
+ * explanation attached.
  */
 
 #include <pthread.h>
@@ -21,68 +19,77 @@
 #include <stdlib.h>
 #include <errno.h>
 #include <unistd.h>
+#include <string.h>
 #include "posixtest.h"
 
-#define TEST "4-1"
-#define FUNCTION "pthread_cond_destroy"
-#define ERROR_PREFIX "unexpected error: " FUNCTION " " TEST ": "
-
-/* cond used by the two threads */
 pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
-
-/* cond used by the two threads */
 pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
 
 void *thread(void *tmp)
 {
 	int rc = 0;
 
-	/* acquire the mutex */
+	pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
+
 	rc = pthread_mutex_lock(&mutex);
 	if (rc != 0) {
-		printf(ERROR_PREFIX "pthread_mutex_lock\n");
+		perror("child: pthread_mutex_lock");
 		exit(PTS_UNRESOLVED);
 	}
 
-	/* Wait on the cond var. This will not return, as nobody signals */
 	rc = pthread_cond_wait(&cond, &mutex);
-	if (rc != 0) {
-		printf(ERROR_PREFIX "pthread_cond_wait\n");
-		exit(PTS_UNRESOLVED);
-	}
+	if (rc != 0)
+		perror("child: pthread_cond_wait");
 
 	rc = pthread_mutex_unlock(&mutex);
-	if (rc != 0) {
-		printf(ERROR_PREFIX "pthread_mutex_unlock\n");
+	if (rc != 0)
+		perror("child: pthread_mutex_unlock");
+
+	return tmp;
+}
+
+void *watchdog(void *arg)
+{
+	pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
+
+	sleep(1);
+	printf("watchdog: pthread_cond_destroy() appears to be blocking\n");
+	if (pthread_cond_signal(&cond)) {
+		perror("watchdog: pthread_cond_signal()");
 		exit(PTS_UNRESOLVED);
 	}
-	return NULL;
+
+	return arg;
 }
 
 int main(void)
 {
-	pthread_t low_id;
+	pthread_t low_id, watchdog_thread;
 	int rc = 0;
 
-	/* Create a new thread with default attributes */
 	rc = pthread_create(&low_id, NULL, thread, NULL);
 	if (rc != 0) {
-		printf(ERROR_PREFIX "pthread_create\n");
+		perror("main: pthread_create");
 		exit(PTS_UNRESOLVED);
 	}
 
-	/* Let the other thread run */
-	sleep(2);
+	sleep(1);
+
+	rc = pthread_create(&watchdog_thread, NULL, watchdog, NULL);
+	if (rc != 0) {
+		perror("main: pthread_create");
+		exit(PTS_UNRESOLVED);
+	}
 
-	/* Try to destroy the cond var. This should return an error */
 	rc = pthread_cond_destroy(&cond);
 	if (rc != EBUSY) {
-		printf(ERROR_PREFIX "Test PASS: Expected %d(EBUSY) got %d, "
-		       "though the standard states 'may' fail\n", EBUSY, rc);
-
-		exit(PTS_PASS);
+		printf("The standard recommends returning %d, EBUSY, but got %d, %s\n",
+		       EBUSY, rc, strerror(rc));
 	}
 
+	pthread_cancel(watchdog_thread);
+	pthread_cancel(low_id);
+
 	printf("Test PASSED\n");
 	exit(PTS_PASS);
 }
-- 
2.14.2

[LTP] [PATCH 0/2] Updates to pthread_{barrier,cond}_destroy()

[Cyril Hrubis]

Hi!
> These tests do naturally include race conditions and waste time in calls to
> sleep, we could mitigate them by using another synchronisation mechanism (e.g
> TST_CHECKPOINT or fzsync), but not remove them completely. I'm not sure whether
> introducing LTP library functions into the POSIX tests is a good idea or not.

We try to avoid any Linux specific code in the Open Posix Testsuite and
generally try to stick only to POSIX APIs there. This is not always
possible though, so we have some code in include/ that is not portable
and has to be reimplemented per each Unix platform, for instance the
affinity.h. But we try to avoid that as much as we can, hence we do not
make use any of the LTP test library here, since quite a lot of the code
we have in there is Linux specific.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] POSIX: Allow pthread_barrier_destroy() to block

[Cyril Hrubis]

Hi!
>  static pthread_barrier_t barrier;
> -static int thread_state;
> +static volatile int thread_state;
>  #define NOT_CREATED_THREAD 1
>  #define ENTERED_THREAD 2
>  #define EXITING_THREAD 3
> +#define WAIT_LOOP 0xFFFFFF
>  
>  static void *fn_chld(void *arg)
>  {
> @@ -42,73 +46,90 @@ static void *fn_chld(void *arg)
>  
>  	pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
>  
> +	printf("child: barrier wait\n");
>  	thread_state = ENTERED_THREAD;
> +	rc = pthread_barrier_wait(&barrier);
> +	if (rc == PTHREAD_BARRIER_SERIAL_THREAD)
> +		printf("child: got PTHREAD_BARRIER_SERIAL_THREAD\n");
> +	else if (rc != 0)
> +		perror("child: pthread_barrier_wait");
> +	else
> +		printf("child: pthread_barrier_wait returned success");
>  
> -	/* Child should block here */
> -	printf("child: barrier wait\n");
> +	thread_state = EXITING_THREAD;
> +	return arg;
> +}
> +
> +static void *watchdog(void *arg)
> +{
> +	int rc = 0;
> +
> +	pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
> +
> +	sleep(1);
> +	printf("watchdog: It appears pthread_barrier_destroy() is blocking\n");
>  	rc = pthread_barrier_wait(&barrier);
>  	if (rc != 0 && rc != PTHREAD_BARRIER_SERIAL_THREAD) {
> -		printf("Error: child: pthread_barrier_wait() get unexpected "
> -		       "return code : %d\n", rc);
> +		perror("watchdog: pthread_barrier_wait");
>  		exit(PTS_UNRESOLVED);
> -	} else if (rc == PTHREAD_BARRIER_SERIAL_THREAD) {
> -		printf("child: get PTHREAD_BARRIER_SERIAL_THREAD\n");
>  	}
>  
> -	thread_state = EXITING_THREAD;
> -	pthread_exit(0);
> -	return NULL;
> +	return arg;
>  }
>  
>  int main(void)
>  {
>  	int cnt = 0;
>  	int rc;
> -	pthread_t child_thread;
> +	pthread_t child_thread, watchdog_thread;
>  
>  	printf("main: Initialize barrier with count = 2\n");
>  	if (pthread_barrier_init(&barrier, NULL, 2) != 0) {
> -		printf("main: Error at pthread_barrier_init()\n");
> +		perror("main: pthread_barrier_init");
>  		return PTS_UNRESOLVED;
>  	}
>  
>  	printf("main: create child thread\n");
>  	thread_state = NOT_CREATED_THREAD;
>  	if (pthread_create(&child_thread, NULL, fn_chld, NULL) != 0) {
> -		printf("main: Error at pthread_create()\n");
> +		perror("main: pthread_create");
> +		return PTS_UNRESOLVED;
> +	}
> +
> +	printf("main: create watchdog thread\n");
> +	if (pthread_create(&watchdog_thread, NULL, watchdog, NULL) != 0) {
> +		perror("main: Error at pthread_create");
>  		return PTS_UNRESOLVED;
>  	}
>  
> -	/* Expect the child to block */
>  	cnt = 0;
> -	do {
> -		sleep(1);
> -	} while (thread_state != EXITING_THREAD && cnt++ < 2);
> +	for (cnt = 0; thread_state < ENTERED_THREAD && cnt < WAIT_LOOP; cnt++)
> +		sched_yield();
> +	/* Yield once more to increase the probability that the child thread
> +	 * will call pthread_barrier_wait() before this thread reaches
> +	 * pthread_barrier_destroy().
> +	 */
> +	sched_yield();
>  
>  	if (thread_state == EXITING_THREAD) {
> -		/* child thread did not block */
> -		printf("Test FAILED: child thread did not block on "
> -		       "pthread_barrier_wait()\n");
> +		printf("Test FAILED: child thread did not block on pthread_barrier_wait()\n");
>  		exit(PTS_FAIL);
> -	} else if (thread_state != ENTERED_THREAD) {
> -		printf("Unexpected thread state\n");
> +	} else if (thread_state == NOT_CREATED_THREAD) {
> +		printf("Child thread did not start (quick enough)\n");
>  		exit(PTS_UNRESOLVED);
>  	}
>  
>  	printf("main: destroy barrier while child is waiting\n");
> -
>  	rc = pthread_barrier_destroy(&barrier);
>  
> -	if (rc == EBUSY) {
> -		printf("main: correctly got EBUSY\n");
> -		printf("Test PASSED\n");
> -	} else {
> +	if (rc != EBUSY) {
>  		printf("main: got return code: %d, %s\n", rc, strerror(rc));
> -		printf
> -		    ("Test PASSED: Note*: Expected EBUSY, but standard says 'may' fail.\n");
> +		printf("Note: POSIX recommends returning EBUSY\n");
>  	}
>  
> -	/* Cleanup (cancel thread in case it is still blocking */
> +	printf("Test PASSED\n");

What about we fail the test in a case that we do not get either of
success or EBUSY? I doubt that there is any harm in making this kind of
check here with the watchdog thread in place.

Otherwise the patch looks good.

> +	pthread_cancel(watchdog_thread);
>  	pthread_cancel(child_thread);
>  
>  	return PTS_PASS;
> -- 
> 2.14.2
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] POSIX: Allow pthread_cond_destroy to block

[Cyril Hrubis]

Hi!
Do I understand this correctly that this is a preemptive fix? Since this
test seems to work fine for me, while the barrier one blocks.

Can we have this kind of information included in the commit message as
well?

>  int main(void)
>  {
> -	pthread_t low_id;
> +	pthread_t low_id, watchdog_thread;
>  	int rc = 0;
>  
> -	/* Create a new thread with default attributes */
>  	rc = pthread_create(&low_id, NULL, thread, NULL);
>  	if (rc != 0) {
> -		printf(ERROR_PREFIX "pthread_create\n");
> +		perror("main: pthread_create");
>  		exit(PTS_UNRESOLVED);
>  	}
>  
> -	/* Let the other thread run */
> -	sleep(2);
> +	sleep(1);
> +
> +	rc = pthread_create(&watchdog_thread, NULL, watchdog, NULL);
> +	if (rc != 0) {
> +		perror("main: pthread_create");
> +		exit(PTS_UNRESOLVED);
> +	}
>  
> -	/* Try to destroy the cond var. This should return an error */
>  	rc = pthread_cond_destroy(&cond);
>  	if (rc != EBUSY) {
> -		printf(ERROR_PREFIX "Test PASS: Expected %d(EBUSY) got %d, "
> -		       "though the standard states 'may' fail\n", EBUSY, rc);
> -
> -		exit(PTS_PASS);
> +		printf("The standard recommends returning %d, EBUSY, but got %d, %s\n",
> +		       EBUSY, rc, strerror(rc));
>  	}

Here as well, I would consider checking either for succes or for EBUSY,
it does not make any sense to return any other error than the one
specified in the standard in a case that we do not return 0.

> +	pthread_cancel(watchdog_thread);
> +	pthread_cancel(low_id);
> +
>  	printf("Test PASSED\n");
>  	exit(PTS_PASS);
>  }
> -- 
> 2.14.2
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] POSIX: Allow pthread_barrier_destroy() to block

[Richard Palethorpe]

Hello,

Cyril Hrubis writes:

>> -	/* Cleanup (cancel thread in case it is still blocking */
>> +	printf("Test PASSED\n");
>
> What about we fail the test in a case that we do not get either of
> success or EBUSY? I doubt that there is any harm in making this kind of
> check here with the watchdog thread in place.
>

I suppose it would be misleading if it returned EINVAL, but it would
only be an error if it returned EINTR because that is specifically
prohibited by the spec.

Thinking about it, it is all undefined behaviour, but the test is to see
if it complies with the recommended behviour. So maybe we should fail
the test if it does not return EBUSY. If someone doesn't agree with the
recommended behaviour they can ignore the test.

I don't have a strong opinion on whether the test should pass or
fail. However I would like to have some clear rules to follow for the
POSIX tests. Like "If it is undefined behaviour then anything except a
null pointer derefernce or buffer overflow is acceptable". So basically
any behaviour that isn't obviously a security/integrity concern is OK. Or
something like "If it is undefined behaviour then we check if it follows
the recommended behaviour", and make it clear in the test description
that this test may fail on a compliant implementation.

-- 
Thank you,
Richard.

[LTP] [PATCH 2/2] POSIX: Allow pthread_cond_destroy to block

[Richard Palethorpe]

hello,

Cyril Hrubis writes:

> Hi!
> Do I understand this correctly that this is a preemptive fix? Since this
> test seems to work fine for me, while the barrier one blocks.
>
> Can we have this kind of information included in the commit message as
> well?
>

It deadlocks on my system. The two functions share a lot of code in
glibc as well, so the same code is responsible for the blocking.

--
Thank you,
Richard.

[LTP] [PATCH 2/2] POSIX: Allow pthread_cond_destroy to block

[Cyril Hrubis]

Hi!
> > Do I understand this correctly that this is a preemptive fix? Since this
> > test seems to work fine for me, while the barrier one blocks.
> >
> > Can we have this kind of information included in the commit message as
> > well?
> >
> 
> It deadlocks on my system. The two functions share a lot of code in
> glibc as well, so the same code is responsible for the blocking.

Strange, for me this one worked fine. I guess that it depends on actual
glibc version.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] POSIX: Allow pthread_barrier_destroy() to block

[Cyril Hrubis]

Hi!
> >> -	/* Cleanup (cancel thread in case it is still blocking */
> >> +	printf("Test PASSED\n");
> >
> > What about we fail the test in a case that we do not get either of
> > success or EBUSY? I doubt that there is any harm in making this kind of
> > check here with the watchdog thread in place.
> >
> 
> I suppose it would be misleading if it returned EINVAL, but it would
> only be an error if it returned EINTR because that is specifically
> prohibited by the spec.
> 
> Thinking about it, it is all undefined behaviour, but the test is to see
> if it complies with the recommended behviour. So maybe we should fail
> the test if it does not return EBUSY. If someone doesn't agree with the
> recommended behaviour they can ignore the test.

The open posix testsuite has PTS_UNSUPPORTED status that is used when
optional part of the POSIX standard is not implemented, that makes much
more sense than fail in this case.

> I don't have a strong opinion on whether the test should pass or
> fail. However I would like to have some clear rules to follow for the
> POSIX tests. Like "If it is undefined behaviour then anything except a
> null pointer derefernce or buffer overflow is acceptable". So basically
> any behaviour that isn't obviously a security/integrity concern is OK. Or
> something like "If it is undefined behaviour then we check if it follows
> the recommended behaviour", and make it clear in the test description
> that this test may fail on a compliant implementation.

My intent with these testcases was always to make them first class
citizen on Linux but at the same time keep them working on any other
Unix. The fact that Linux does not implement most of the optional
features makes it a bit more complicated though.

So in this case I would go for passing the test only if the optional
EBUSY error is implemented and return UNSUPPORTED otherwise. I would
still keep the code with watchdog there so that the case that the call
blocks until the barrier is in use blocks is handled gracefully though.

Does that sound reasonable?

And yes I think that keeping the test just to see if we cannot get
SegFault or something in this case is valuable even though POSIX
technically allows such behavior.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] POSIX: Allow pthread_barrier_destroy() to block

[Richard Palethorpe]

Hello,

Cyril Hrubis writes:

> Hi!
>> >> -	/* Cleanup (cancel thread in case it is still blocking */
>> >> +	printf("Test PASSED\n");
>> >
>> > What about we fail the test in a case that we do not get either of
>> > success or EBUSY? I doubt that there is any harm in making this kind of
>> > check here with the watchdog thread in place.
>> >
>> 
>> I suppose it would be misleading if it returned EINVAL, but it would
>> only be an error if it returned EINTR because that is specifically
>> prohibited by the spec.
>> 
>> Thinking about it, it is all undefined behaviour, but the test is to see
>> if it complies with the recommended behviour. So maybe we should fail
>> the test if it does not return EBUSY. If someone doesn't agree with the
>> recommended behaviour they can ignore the test.
>
> The open posix testsuite has PTS_UNSUPPORTED status that is used when
> optional part of the POSIX standard is not implemented, that makes much
> more sense than fail in this case.
>
>> I don't have a strong opinion on whether the test should pass or
>> fail. However I would like to have some clear rules to follow for the
>> POSIX tests. Like "If it is undefined behaviour then anything except a
>> null pointer derefernce or buffer overflow is acceptable". So basically
>> any behaviour that isn't obviously a security/integrity concern is OK. Or
>> something like "If it is undefined behaviour then we check if it follows
>> the recommended behaviour", and make it clear in the test description
>> that this test may fail on a compliant implementation.
>
> My intent with these testcases was always to make them first class
> citizen on Linux but at the same time keep them working on any other
> Unix. The fact that Linux does not implement most of the optional
> features makes it a bit more complicated though.
>
> So in this case I would go for passing the test only if the optional
> EBUSY error is implemented and return UNSUPPORTED otherwise. I would
> still keep the code with watchdog there so that the case that the call
> blocks until the barrier is in use blocks is handled gracefully though.
>
> Does that sound reasonable?
>
> And yes I think that keeping the test just to see if we cannot get
> SegFault or something in this case is valuable even though POSIX
> technically allows such behavior.

Yes, this makes perfect sense!

-- 
Thank you,
Richard.