Date: Sat, 27 Aug 2011 18:07:19 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Jilles Tjoelker <jilles@stack.nl> Cc: freebsd-stable@freebsd.org, Slawa Olhovchenkov <slw@zxy.spb.ru> Subject: Re: sigwait return 4 Message-ID: <20110827150719.GJ17489@deviant.kiev.zoral.com.ua> In-Reply-To: <20110827142536.GA66167@stack.nl> References: <20110824181907.GA48394@zxy.spb.ru> <20110824190703.GY17489@deviant.kiev.zoral.com.ua> <20110824205609.GA96070@stack.nl> <20110824212929.GC17489@deviant.kiev.zoral.com.ua> <20110827142536.GA66167@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
--5HcqcVqIBwN2S5gT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Aug 27, 2011 at 04:25:36PM +0200, Jilles Tjoelker wrote: > On Thu, Aug 25, 2011 at 12:29:29AM +0300, Kostik Belousov wrote: > > On Wed, Aug 24, 2011 at 10:56:09PM +0200, Jilles Tjoelker wrote: > > > sigwait() was fixed not to return EINTR in 9-current in r212405 (fixed > > > up in r219709). The discussion started at > > > http://lists.freebsd.org/pipermail/freebsd-threads/2010-September/004= 892.html > > >=20 > > > Solaris is simply wrong in the same way we were wrong. Although POSIX > > > may not be as clear on this as one may like, its intention is clear a= nd > > > additionally not returning EINTR reduces subtle portability problems. >=20 > > Can you, please, describe why do you consider the behaviour prohibiting > > return of EINTR reasonable ? I do consider that the Solaris behaviour is > > useful. >=20 > Applications need to cope with EINTR returns (usually by retrying the > call); if they do not do this, bugs arise in uncommon cases. >=20 > In the case of sigwait(), applications do not really need EINTR: they > can include the respective signal into the signal set and do the work > inline that was originally in the signal handler. This might require > additional pthread_sigmask() calls. This also fixes the race condition > almost always associated with EINTR. >=20 > Historically, this is because sigwait() came with POSIX threads, which > also explains why it returns an error number rather than setting errno. > The threads group considered EINTR errors not useful enough, given that > they may lead to subtle bugs. This is fully standardized for functions > like pthread_cond_wait() and pthread_mutex_lock(). >=20 > In the case of sigwait(), it also plays a role that glibc has decided > not to return EINTR, so that returning EINTR may lead to subtle bugs > appearing on FreeBSD in software originally written for GNU/Linux. >=20 > The functions sigwaitinfo() and sigtimedwait() came with POSIX realtime > and therefore follow different conventions. I think I finally realized what was the problem Slawa searched the fix for. The fix from r212405 indeed does not allow EINTR to be returned from the sigwait() for new libc, but it still leaves the compat libc and libthr with EINTR. Below is the patch that I provided to Slawa to handle EINTR condition in kernel. The meat is in kern_sig.c two lines, everything else is the r212405 revert. diff --git a/lib/libc/sys/Makefile.inc b/lib/libc/sys/Makefile.inc index fe5061d..aa0959b 100644 --- a/lib/libc/sys/Makefile.inc +++ b/lib/libc/sys/Makefile.inc @@ -24,9 +24,6 @@ SRCS+=3D ${SYSCALL_COMPAT_SRCS} NOASM+=3D ${SYSCALL_COMPAT_SRCS:S/.c/.o/} PSEUDO+=3D _fcntl.o .endif -SRCS+=3D sigwait.c -NOASM+=3D sigwait.o -PSEUDO+=3D _sigwait.o =20 # Add machine dependent asm sources: SRCS+=3D${MDASM} diff --git a/lib/libc/sys/Symbol.map b/lib/libc/sys/Symbol.map index 095751a..2ba1f8f 100644 --- a/lib/libc/sys/Symbol.map +++ b/lib/libc/sys/Symbol.map @@ -937,7 +937,6 @@ FBSDprivate_1.0 { _sigtimedwait; __sys_sigtimedwait; _sigwait; - __sigwait; __sys_sigwait; _sigwaitinfo; __sys_sigwaitinfo; diff --git a/lib/libc/sys/sigwait.c b/lib/libc/sys/sigwait.c deleted file mode 100644 index 2fdffdd..0000000 --- a/lib/libc/sys/sigwait.c +++ /dev/null @@ -1,46 +0,0 @@ -/*- - * Copyright (c) 2010 davidxu@freebsd.org - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions - * are met: - * 1. Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * 2. Redistributions in binary form must reproduce the above copyright - * notice, this list of conditions and the following disclaimer in the - * documentation and/or other materials provided with the distribution. - * - * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURP= OSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE - * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENT= IAL - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS - * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) - * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STR= ICT - * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY W= AY - * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF - * SUCH DAMAGE. - */ - -#include <sys/cdefs.h> -__FBSDID("$FreeBSD$"); - -#include <errno.h> -#include <signal.h> - -int __sys_sigwait(const sigset_t * restrict, int * restrict); - -__weak_reference(__sigwait, sigwait); - -int -__sigwait(const sigset_t * restrict set, int * restrict sig) -{ - int ret; - - /* POSIX does not allow EINTR to be returned */ - do { - ret =3D __sys_sigwait(set, sig); - } while (ret =3D=3D EINTR); - return (ret); -} diff --git a/lib/libthr/thread/thr_sig.c b/lib/libthr/thread/thr_sig.c index 66358db..daf1871 100644 --- a/lib/libthr/thread/thr_sig.c +++ b/lib/libthr/thread/thr_sig.c @@ -67,7 +67,7 @@ int _sigtimedwait(const sigset_t *set, siginfo_t *info, const struct timespec * timeout); int __sigwaitinfo(const sigset_t *set, siginfo_t *info); int _sigwaitinfo(const sigset_t *set, siginfo_t *info); -int ___sigwait(const sigset_t *set, int *sig); +int __sigwait(const sigset_t *set, int *sig); int _sigwait(const sigset_t *set, int *sig); int __sigsuspend(const sigset_t *sigmask); int _sigaction(int, const struct sigaction *, struct sigaction *); @@ -628,7 +628,7 @@ __sigsuspend(const sigset_t * set) return (ret); } =20 -__weak_reference(___sigwait, sigwait); +__weak_reference(__sigwait, sigwait); __weak_reference(__sigtimedwait, sigtimedwait); __weak_reference(__sigwaitinfo, sigwaitinfo); =20 @@ -702,17 +702,15 @@ _sigwait(const sigset_t *set, int *sig) * it is not canceled. */=20 int -___sigwait(const sigset_t *set, int *sig) +__sigwait(const sigset_t *set, int *sig) { struct pthread *curthread =3D _get_curthread(); sigset_t newset; int ret; =20 - do { - _thr_cancel_enter(curthread); - ret =3D __sys_sigwait(thr_remove_thr_signals(set, &newset), sig); - _thr_cancel_leave(curthread, (ret !=3D 0)); - } while (ret =3D=3D EINTR); + _thr_cancel_enter(curthread); + ret =3D __sys_sigwait(thr_remove_thr_signals(set, &newset), sig); + _thr_cancel_leave(curthread, (ret !=3D 0)); return (ret); } =20 diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 26ef0d7..4655246 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -1094,6 +1094,8 @@ sigwait(struct thread *td, struct sigwait_args *uap) =20 error =3D kern_sigtimedwait(td, set, &ksi, NULL); if (error) { + if (error =3D=3D EINTR) + error =3D ERESTART; if (error =3D=3D ERESTART) return (error); td->td_retval[0] =3D error; --5HcqcVqIBwN2S5gT Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk5ZCCYACgkQC3+MBN1Mb4hAywCeJRxVlNjJccjxcpeoJaP3R/Vx RP8Anj0Gwn+W1hc2WKdYs0kWewzfypcU =mEV+ -----END PGP SIGNATURE----- --5HcqcVqIBwN2S5gT--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110827150719.GJ17489>