PTHREAD_CANCEL_DEFERRED

[Kostik Belousov]

On Fri, Aug 20, 2010 at 01:14:55AM +0000, David Xu wrote:
> Kostik Belousov wrote:
> 
> >
> >What would happen in the following situation:
> >thr_wake() is called;
> >some syscall is started executing and slept, assume that SA_RESTART is
> >for SIGHUP (just an example);
> >SIGHUP is sent to the process and the thread is selected
> >for delivery, also assume that handler is installed.
> >
> >As I understand, in this situation, EINTR is returned from syscall.
> 
> Yes, because cancellation should have higher priority over signals,
> if signal always causes ERESTART to be returned, and system call
> is always restarted, the system call may not return forever because
> its event it is waiting never happens, for example, it is reading a byte
> from a socket, but it is never available, we end up not being able to
> cancel the thread, this is incorrect.

Again, this is not exactly what I mean. Assume that the cancellation is
not requested at all. I see no reason to return EINTR.

As I see, you do not check errno when seeing syscall error return
in the committed patch.

Anyway, below is mostly cosmetic change to reduce code duplication,
saved from the reverted patch of mine. Any objections ?

diff --git a/lib/libthr/thread/thr_sig.c b/lib/libthr/thread/thr_sig.c
index eed6fdb..115aa65 100644
--- a/lib/libthr/thread/thr_sig.c
+++ b/lib/libthr/thread/thr_sig.c
@@ -268,23 +268,26 @@ _pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)
 
 __weak_reference(__sigsuspend, sigsuspend);
 
-int
-_sigsuspend(const sigset_t * set)
+static const sigset_t *
+thr_remove_thr_signals(const sigset_t *set, sigset_t *newset)
 {
-	sigset_t newset;
 	const sigset_t *pset;
-	int ret;
 
 	if (SIGISMEMBER(*set, SIGCANCEL)) {
-		newset = *set;
-		SIGDELSET(newset, SIGCANCEL);
-		pset = &newset;
+		*newset = *set;
+		SIGDELSET(*newset, SIGCANCEL);
+		pset = newset;
 	} else
 		pset = set;
+	return (pset);
+}
 
-	ret = __sys_sigsuspend(pset);
+int
+_sigsuspend(const sigset_t * set)
+{
+	sigset_t newset;
 
-	return (ret);
+	return (__sys_sigsuspend(thr_remove_thr_signals(set, &newset)));
 }
 
 int
@@ -292,18 +295,10 @@ __sigsuspend(const sigset_t * set)
 {
 	struct pthread *curthread = _get_curthread();
 	sigset_t newset;
-	const sigset_t *pset;
 	int ret;
 
-	if (SIGISMEMBER(*set, SIGCANCEL)) {
-		newset = *set;
-		SIGDELSET(newset, SIGCANCEL);
-		pset = &newset;
-	} else
-		pset = set;
-
 	_thr_cancel_enter(curthread);
-	ret = __sys_sigsuspend(pset);
+	ret = __sys_sigsuspend(thr_remove_thr_signals(set, &newset));
 	_thr_cancel_leave(curthread);
 
 	return (ret);
@@ -318,17 +313,9 @@ _sigtimedwait(const sigset_t *set, siginfo_t *info,
 	const struct timespec * timeout)
 {
 	sigset_t newset;
-	const sigset_t *pset;
-	int ret;
 
-	if (SIGISMEMBER(*set, SIGCANCEL)) {
-		newset = *set;
-		SIGDELSET(newset, SIGCANCEL);
-		pset = &newset;
-	} else
-		pset = set;
-	ret = __sys_sigtimedwait(pset, info, timeout);
-	return (ret);
+	return (__sys_sigtimedwait(thr_remove_thr_signals(set, &newset), info,
+	    timeout));
 }
 
 /*
@@ -342,17 +329,11 @@ __sigtimedwait(const sigset_t *set, siginfo_t *info,
 {
 	struct pthread	*curthread = _get_curthread();
 	sigset_t newset;
-	const sigset_t *pset;
 	int ret;
 
-	if (SIGISMEMBER(*set, SIGCANCEL)) {
-		newset = *set;
-		SIGDELSET(newset, SIGCANCEL);
-		pset = &newset;
-	} else
-		pset = set;
 	_thr_cancel_enter_defer(curthread, 1);
-	ret = __sys_sigtimedwait(pset, info, timeout);
+	ret = __sys_sigtimedwait(thr_remove_thr_signals(set, &newset), info,
+	    timeout);
 	_thr_cancel_leave_defer(curthread, (ret == -1));
 	return (ret);
 }
@@ -361,18 +342,8 @@ int
 _sigwaitinfo(const sigset_t *set, siginfo_t *info)
 {
 	sigset_t newset;
-	const sigset_t *pset;
-	int ret;
-
-	if (SIGISMEMBER(*set, SIGCANCEL)) {
-		newset = *set;
-		SIGDELSET(newset, SIGCANCEL);
-		pset = &newset;
-	} else
-		pset = set;
 
-	ret = __sys_sigwaitinfo(pset, info);
-	return (ret);
+	return (__sys_sigwaitinfo(thr_remove_thr_signals(set, &newset), info));
 }
 
 /*
@@ -385,18 +356,10 @@ __sigwaitinfo(const sigset_t *set, siginfo_t *info)
 {
 	struct pthread	*curthread = _get_curthread();
 	sigset_t newset;
-	const sigset_t *pset;
 	int ret;
 
-	if (SIGISMEMBER(*set, SIGCANCEL)) {
-		newset = *set;
-		SIGDELSET(newset, SIGCANCEL);
-		pset = &newset;
-	} else
-		pset = set;
-
 	_thr_cancel_enter_defer(curthread, 1);
-	ret = __sys_sigwaitinfo(pset, info);
+	ret = __sys_sigwaitinfo(thr_remove_thr_signals(set, &newset), info);
 	_thr_cancel_leave_defer(curthread, ret == -1);
 	return (ret);
 }
@@ -405,18 +368,8 @@ int
 _sigwait(const sigset_t *set, int *sig)
 {
 	sigset_t newset;
-	const sigset_t *pset;
-	int ret;
-
-	if (SIGISMEMBER(*set, SIGCANCEL)) {
-		newset = *set;
-		SIGDELSET(newset, SIGCANCEL);
-		pset = &newset;
-	} else 
-		pset = set;
 
-	ret = __sys_sigwait(pset, sig);
-	return (ret);
+	return (__sys_sigwait(thr_remove_thr_signals(set, &newset), sig));
 }
 
 /*
@@ -429,18 +382,10 @@ __sigwait(const sigset_t *set, int *sig)
 {
 	struct pthread	*curthread = _get_curthread();
 	sigset_t newset;
-	const sigset_t *pset;
 	int ret;
 
-	if (SIGISMEMBER(*set, SIGCANCEL)) {
-		newset = *set;
-		SIGDELSET(newset, SIGCANCEL);
-		pset = &newset;
-	} else 
-		pset = set;
-
 	_thr_cancel_enter_defer(curthread, 1);
-	ret = __sys_sigwait(pset, sig);
+	ret = __sys_sigwait(thr_remove_thr_signals(set, &newset), sig);
 	_thr_cancel_leave_defer(curthread, (ret != 0));
 	return (ret);
 }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-threads/attachments/20100820/331f70ff/attachment.pgp