Re: [PATCH 04/15] Hurd signals: refactor check_pending_signals

[Jeremie Koenig]

Hi, thanks for your thorough review.

On Sun, Jul 03, 2011 at 12:34:22AM +0200, Samuel Thibault wrote:
> Jeremie Koenig, le Wed 29 Jun 2011 18:30:16 +0200, a écrit :
> > * hurd/hurdsig.c (check_pending_signals): Split into pending_signals,
> > post_pending and post_all_pending_signals.
> > (_hurd_internal_post_signal): Handle the distinction between poll
> > requests and real signals there.
> 
> I believe it is correct.
> 
> There is just one change that would be worth mentioning in the changelog:
> 
> > -                 /* We "deliver" immediately pending blocked signals whose
> > -                    action might be to ignore, so that if ignored they are
> > -                    dropped right away.  */
> > -                 || ss->actions[signo].sa_handler == SIG_IGN
> > -                 || ss->actions[signo].sa_handler == SIG_DFL))
> 
> This is not done any more.  Actually I believe it is more correct.

I don't think POSIX has a strong requirement either way. For instance,

(http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html)
| If the action associated with a blocked signal is anything other than to
| ignore the signal, and if that signal is generated for the thread, the
| signal shall remain pending until it is unblocked, it is accepted when
| it is selected and returned by a call to the sigwait() function, or the
| action associated with it is set to ignore the signal.

(http://pubs.opengroup.org/onlinepubs/009604499/xrat/xsh_chap02.html)
| In the 4.3 BSD system, signals that are blocked and set to SIG_IGN are
| discarded immediately upon generation. For a signal that is ignored as
| its default action, if the action is SIG_DFL and the signal is blocked,
| a generated signal remains pending. In the 4.1 BSD system and in System
| V Release 3 (two other implementations that support a somewhat similar
| signal mechanism), all ignored blocked signals remain pending if
| generated. Because it is not normally useful for an application to
| simultaneously ignore and block the same signal, it was unnecessary for
| POSIX.1 to specify behavior that would invalidate any of the historical
| implementations.

> For instance, I believe that the following
> 
>       signal(SIGINT, SIG_IGN);
>       sighold(SIGINT);
>       raise(SIGINT);
>       signal(SIGINT, SIG_DFL);
>       sigrelse(SIGINT);
>       printf("foo\n");
> 
> is indeed supposed to terminate on sigrelse() (and it does on Linux),
> and not ignore the signal just because the handler is currently SIGIGN.
> It can indeed be useful to be able to block the signal while twiddling
> with the signal handler.

Note that this example still does not terminate after the patch, because
ignored signals are never actually marked pending:

(hurd/hurdsig.c)
|   /* Handle receipt of a blocked signal, or any signal while stopped.  */
|   if (act != ignore &&          /* Signals ignored now are forgotten now.  */
|       __sigismember (&ss->blocked, signo) ||
|       (signo != SIGKILL && _hurd_stopped))
|     { 
|       mark_pending ();
|       act = ignore;
|     }

The removed code changed anything only if a previously effectful,
blocked signal has been marked pending, then reset to SIG_IGN (or a
benign SIG_DFL) in the meantime. If a poll request happens, the signal
will merely be given a new opportunity to be forgotten.

So the patch probably changes behaviour only in corner cases, if at all.

-- 
Jeremie Koenig <jk@jk.fr.eu.org>
http://jk.fr.eu.org