Re: simplify sys___thrsigdivert a bit
On Sat, Oct 23, 2021 at 05:47:58PM +0200, Mark Kettenis wrote:
> > Date: Sat, 23 Oct 2021 17:29:36 +0200
> > From: Claudio Jeker
> >
> > The sys___thrsigdivert code can be simplified a bit. It is possible to
> > set the error before the loop and then have the loop exit after polling
> > for pending signals. IMO the results looks nicer than what we have now.
> >
> > OK?
>
> That does not look right:
>
> The function sigtimedwait() behaves the same as sigwaitinfo() except
> that if none of the signals specified by set are pending,
> sigtimedwait() waits for the time interval specified in the timespec
> structure referenced by timeout. If the timespec structure pointed
> to by timeout is zero-valued and if none of the signals specified by
> set are pending, then sigtimedwait() returns immediately with an
> error. If timeout is the NULL pointer, the behaviour is unspecified.
>
> So you need to check for signals first.
The code does the check first. It just sets the error before doing the
pending signal check. If a signal is pending the error is reset to 0 and
the function returns. Else it will break out of the loop with the error
and not hit the tsleep.
> The current code looks fine to me.
>
>
> > Index: kern/kern_sig.c
> > ===
> > RCS file: /cvs/src/sys/kern/kern_sig.c,v
> > retrieving revision 1.286
> > diff -u -p -r1.286 kern_sig.c
> > --- kern/kern_sig.c 23 Oct 2021 14:56:55 - 1.286
> > +++ kern/kern_sig.c 23 Oct 2021 15:15:21 -
> > @@ -1761,7 +1761,6 @@ sys___thrsigdivert(struct proc *p, void
> > sigset_t mask = SCARG(uap, sigmask) &~ sigcantmask;
> > siginfo_t si;
> > uint64_t nsecs = INFSLP;
> > - int timeinvalid = 0;
> > int error = 0;
> >
> > memset(, 0, sizeof(si));
> > @@ -1774,12 +1773,21 @@ sys___thrsigdivert(struct proc *p, void
> > if (KTRPOINT(p, KTR_STRUCT))
> > ktrreltimespec(p, );
> > #endif
> > - if (!timespecisvalid())
> > - timeinvalid = 1;
> > - else
> > + if (timespecisvalid()) {
> > nsecs = TIMESPEC_TO_NSEC();
> > + } else {
> > + /*
> > +* per-POSIX, delay this error until
> > +* after checking for signals
> > +*/
> > + error = EINVAL;
> > + }
> > }
> >
> > + /* per-POSIX, return immediatly if timeout is zero-valued */
> > + if (nsecs == 0)
> > + error = EAGAIN;
> > +
> > dosigsuspend(p, p->p_sigmask &~ mask);
> > for (;;) {
> > si.si_signo = cursig(p);
> > @@ -1791,13 +1799,6 @@ sys___thrsigdivert(struct proc *p, void
> > break;
> > }
> > }
> > -
> > - /* per-POSIX, delay this error until after the above */
> > - if (timeinvalid)
> > - error = EINVAL;
> > - /* per-POSIX, return immediatly if timeout is zero-valued */
> > - if (nsecs == 0)
> > - error = EAGAIN;
> >
> > if (error != 0)
> > break;
> >
> >
--
:wq Claudio
Re: simplify sys___thrsigdivert a bit
> Date: Sat, 23 Oct 2021 17:29:36 +0200
> From: Claudio Jeker
>
> The sys___thrsigdivert code can be simplified a bit. It is possible to
> set the error before the loop and then have the loop exit after polling
> for pending signals. IMO the results looks nicer than what we have now.
>
> OK?
That does not look right:
The function sigtimedwait() behaves the same as sigwaitinfo() except
that if none of the signals specified by set are pending,
sigtimedwait() waits for the time interval specified in the timespec
structure referenced by timeout. If the timespec structure pointed
to by timeout is zero-valued and if none of the signals specified by
set are pending, then sigtimedwait() returns immediately with an
error. If timeout is the NULL pointer, the behaviour is unspecified.
So you need to check for signals first.
The current code looks fine to me.
> Index: kern/kern_sig.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.286
> diff -u -p -r1.286 kern_sig.c
> --- kern/kern_sig.c 23 Oct 2021 14:56:55 - 1.286
> +++ kern/kern_sig.c 23 Oct 2021 15:15:21 -
> @@ -1761,7 +1761,6 @@ sys___thrsigdivert(struct proc *p, void
> sigset_t mask = SCARG(uap, sigmask) &~ sigcantmask;
> siginfo_t si;
> uint64_t nsecs = INFSLP;
> - int timeinvalid = 0;
> int error = 0;
>
> memset(, 0, sizeof(si));
> @@ -1774,12 +1773,21 @@ sys___thrsigdivert(struct proc *p, void
> if (KTRPOINT(p, KTR_STRUCT))
> ktrreltimespec(p, );
> #endif
> - if (!timespecisvalid())
> - timeinvalid = 1;
> - else
> + if (timespecisvalid()) {
> nsecs = TIMESPEC_TO_NSEC();
> + } else {
> + /*
> + * per-POSIX, delay this error until
> + * after checking for signals
> + */
> + error = EINVAL;
> + }
> }
>
> + /* per-POSIX, return immediatly if timeout is zero-valued */
> + if (nsecs == 0)
> + error = EAGAIN;
> +
> dosigsuspend(p, p->p_sigmask &~ mask);
> for (;;) {
> si.si_signo = cursig(p);
> @@ -1791,13 +1799,6 @@ sys___thrsigdivert(struct proc *p, void
> break;
> }
> }
> -
> - /* per-POSIX, delay this error until after the above */
> - if (timeinvalid)
> - error = EINVAL;
> - /* per-POSIX, return immediatly if timeout is zero-valued */
> - if (nsecs == 0)
> - error = EAGAIN;
>
> if (error != 0)
> break;
>
>
simplify sys___thrsigdivert a bit
The sys___thrsigdivert code can be simplified a bit. It is possible to
set the error before the loop and then have the loop exit after polling
for pending signals. IMO the results looks nicer than what we have now.
OK?
--
:wq Claudio
Index: kern/kern_sig.c
===
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.286
diff -u -p -r1.286 kern_sig.c
--- kern/kern_sig.c 23 Oct 2021 14:56:55 - 1.286
+++ kern/kern_sig.c 23 Oct 2021 15:15:21 -
@@ -1761,7 +1761,6 @@ sys___thrsigdivert(struct proc *p, void
sigset_t mask = SCARG(uap, sigmask) &~ sigcantmask;
siginfo_t si;
uint64_t nsecs = INFSLP;
- int timeinvalid = 0;
int error = 0;
memset(, 0, sizeof(si));
@@ -1774,12 +1773,21 @@ sys___thrsigdivert(struct proc *p, void
if (KTRPOINT(p, KTR_STRUCT))
ktrreltimespec(p, );
#endif
- if (!timespecisvalid())
- timeinvalid = 1;
- else
+ if (timespecisvalid()) {
nsecs = TIMESPEC_TO_NSEC();
+ } else {
+ /*
+* per-POSIX, delay this error until
+* after checking for signals
+*/
+ error = EINVAL;
+ }
}
+ /* per-POSIX, return immediatly if timeout is zero-valued */
+ if (nsecs == 0)
+ error = EAGAIN;
+
dosigsuspend(p, p->p_sigmask &~ mask);
for (;;) {
si.si_signo = cursig(p);
@@ -1791,13 +1799,6 @@ sys___thrsigdivert(struct proc *p, void
break;
}
}
-
- /* per-POSIX, delay this error until after the above */
- if (timeinvalid)
- error = EINVAL;
- /* per-POSIX, return immediatly if timeout is zero-valued */
- if (nsecs == 0)
- error = EAGAIN;
if (error != 0)
break;
