usr1-signal, usr2-signal, etc.

usr1-signal, usr2-signal, etc.

[Kim F. Storm]

IMHO, the new signal events in emacs 22 are not done the right way.

To me, it would be cleaner to define them as [signal usr1], 
[signal usr2], etc.

Ok, there is currently no "etc" in emacs 22, but if we later
want to be able to catch more signals, this would be a cleaner
way to extend the interface.

We could even allow numeric bindings, like [signal 10] for SIGUSR1,
and also variants like [signal SIGUSR1].

This is the last chance to change the interface, if people agree that
this is cleaner.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[Richard Stallman]

    IMHO, the new signal events in emacs 22 are not done the right way.

    To me, it would be cleaner to define them as [signal usr1], 
    [signal usr2], etc.

Do you mean that it would generate a sequence of two events,
`signal' and `usr1', so that `signal' would act as a prefix char?
I agree.  Please change it.

    Ok, there is currently no "etc" in emacs 22, but if we later
    want to be able to catch more signals, this would be a cleaner
    way to extend the interface.

I don't understand what that refers to.

    We could even allow numeric bindings, like [signal 10] for SIGUSR1,
    and also variants like [signal SIGUSR1].

I don't see how that would work.  The signal has to generate a certain
sequence of events.  If it generates `signal' and `usr1',
the second event is not `SIGUSR1' and it is not 10 (C-h).

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     IMHO, the new signal events in emacs 22 are not done the right way.
>
>     To me, it would be cleaner to define them as [signal usr1], 
>     [signal usr2], etc.
>
> Do you mean that it would generate a sequence of two events,
> `signal' and `usr1', so that `signal' would act as a prefix char?

Exactly.

> I agree.  Please change it.

I'll do that.

>     Ok, there is currently no "etc" in emacs 22, but if we later
>     want to be able to catch more signals, this would be a cleaner
>     way to extend the interface.
>
> I don't understand what that refers to.

I was referring to the "etc." in the list in the previous paragraph.
Currently we only have usr1 and usr2 signals, but if we add more (the
"etc") later, this would be a cleaner interface for such extensions.

>
>     We could even allow numeric bindings, like [signal 10] for SIGUSR1,
>     and also variants like [signal SIGUSR1].
>
> I don't see how that would work.  The signal has to generate a certain
> sequence of events.  If it generates `signal' and `usr1',
> the second event is not `SIGUSR1' and it is not 10 (C-h).

You are right ... it looked like a good idea when I wrote it, but I
must have been sleepy.  :-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

>>     To me, it would be cleaner to define them as [signal usr1], 
>>     [signal usr2], etc.

>> I agree.  Please change it.
>
> I'll do that.

Done.

> Currently we only have usr1 and usr2 signals, but if we add more (the
> "etc") later, this would be a cleaner interface for such extensions.

For example, we could handle SIGTERM, SIGHUP, and SIGQUIT like this
to allow a user to handle those signals in some other way:

The only real difference with the existing code is that 
a) this will run the kill-emacs-hook
b) you don't get a "Fatal Error (sig=nn) message

I don't see either as a problem!


*** bindings.el	04 Dec 2006 11:18:25 +0100	1.174
--- bindings.el	04 Dec 2006 13:59:24 +0100	
***************
*** 1066,1071 ****
--- 1066,1074 ----
  
  ;; Signal handlers
  (define-key global-map [signal] (make-sparse-keymap))
+ (define-key global-map [signal term] 'kill-emacs)
+ (define-key global-map [signal quit] 'kill-emacs)
+ (define-key global-map [signal hup] 'kill-emacs)
  (define-key global-map [signal t] 'ignore)
  
  ;; Don't look for autoload cookies in this file.


*** emacs.c	04 Dec 2006 10:56:50 +0100	1.393
--- emacs.c	04 Dec 2006 13:57:45 +0100	
***************
*** 1192,1198 ****
  	 That makes nohup work.  */
        if (! noninteractive
  	  || signal (SIGHUP, SIG_IGN) != SIG_IGN)
! 	signal (SIGHUP, fatal_error_signal);
        sigunblock (sigmask (SIGHUP));
      }
  
--- 1192,1198 ----
  	 That makes nohup work.  */
        if (! noninteractive
  	  || signal (SIGHUP, SIG_IGN) != SIG_IGN)
! 	signal (SIGHUP, handle_user_signal);
        sigunblock (sigmask (SIGHUP));
      }
  
***************
*** 1207,1213 ****
        /* Don't catch these signals in batch mode if dumping.
  	 On some machines, this sets static data that would make
  	 signal fail to work right when the dumped Emacs is run.  */
!       signal (SIGQUIT, fatal_error_signal);
        signal (SIGILL, fatal_error_signal);
        signal (SIGTRAP, fatal_error_signal);
  #ifdef SIGUSR1
--- 1207,1213 ----
        /* Don't catch these signals in batch mode if dumping.
  	 On some machines, this sets static data that would make
  	 signal fail to work right when the dumped Emacs is run.  */
!       signal (SIGQUIT, handle_user_signal);
        signal (SIGILL, fatal_error_signal);
        signal (SIGTRAP, fatal_error_signal);
  #ifdef SIGUSR1
***************
*** 1252,1258 ****
  #ifdef SIGSYS
        signal (SIGSYS, fatal_error_signal);
  #endif
!       signal (SIGTERM, fatal_error_signal);
  #ifdef SIGXCPU
        signal (SIGXCPU, fatal_error_signal);
  #endif
--- 1252,1258 ----
  #ifdef SIGSYS
        signal (SIGSYS, fatal_error_signal);
  #endif
!       signal (SIGTERM, handle_user_signal);
  #ifdef SIGXCPU
        signal (SIGXCPU, fatal_error_signal);
  #endif

*** keyboard.c	04 Dec 2006 13:35:04 +0100	1.882
--- keyboard.c	04 Dec 2006 13:58:08 +0100	
***************
*** 5952,5957 ****
--- 5952,5965 ----
  	{
  	case 0:
  	  return Qsignal;
+ 	case SIGQUIT:
+ 	  return Qquit;
+ 	case SIGHUP:
+ 	  return intern ("hup");
+ #ifdef SIGTERM
+ 	case SIGTERM:
+ 	  return intern ("term");
+ #endif
  #ifdef SIGUSR1
  	case SIGUSR1:
  	  return intern ("usr1");

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[Richard Stallman]

I have nothing against this change in handling other signals, except
that this is not the right sort of thing to do at this stage of
pretest.

Re: usr1-signal, usr2-signal, etc.

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 04 Dec 2006 14:08:09 +0100, storm@cua.dk (Kim F. Storm) said:

>> Currently we only have usr1 and usr2 signals, but if we add more
>> (the "etc") later, this would be a cleaner interface for such
>> extensions.

> For example, we could handle SIGTERM, SIGHUP, and SIGQUIT like this
> to allow a user to handle those signals in some other way:

I'm concerned about the case that kbd_buffer_store_event_hold is
called from the signal-handler context while it is also executed in
the normal context.  This has already existed for SIGUSR1 and SIGUSR2
before your change, though.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: usr1-signal, usr2-signal, etc.

[David Kastrup]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>>>>>> On Mon, 04 Dec 2006 14:08:09 +0100, storm@cua.dk (Kim F. Storm) said:
>
>>> Currently we only have usr1 and usr2 signals, but if we add more
>>> (the "etc") later, this would be a cleaner interface for such
>>> extensions.
>
>> For example, we could handle SIGTERM, SIGHUP, and SIGQUIT like this
>> to allow a user to handle those signals in some other way:
>
> I'm concerned about the case that kbd_buffer_store_event_hold is
> called from the signal-handler context while it is also executed in
> the normal context.  This has already existed for SIGUSR1 and SIGUSR2
> before your change, though.

SIGUSR1 and SIGUSR2 are, IIRC, new in Emacs 22.  So we have no "nobody
complained about that before" reason for allowing problems in that
manner.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
>
>>>>>>> On Mon, 04 Dec 2006 14:08:09 +0100, storm@cua.dk (Kim F. Storm) said:
>>
>>>> Currently we only have usr1 and usr2 signals, but if we add more
>>>> (the "etc") later, this would be a cleaner interface for such
>>>> extensions.
>>
>>> For example, we could handle SIGTERM, SIGHUP, and SIGQUIT like this
>>> to allow a user to handle those signals in some other way:
>>
>> I'm concerned about the case that kbd_buffer_store_event_hold is
>> called from the signal-handler context while it is also executed in
>> the normal context.  This has already existed for SIGUSR1 and SIGUSR2
>> before your change, though.
>
> SIGUSR1 and SIGUSR2 are, IIRC, new in Emacs 22.  So we have no "nobody
> complained about that before" reason for allowing problems in that
> manner.

They have been there (in some form) since 1997.
Whether anybody actually used them is another thing.

AFAICS, kbd_buffer_store_event[_hold] is usually called inside
BLOCK_INPUT, but there may be a few places where this is not the case,
notably in keyboard.c (and in the signal handler).  

Would someone pls. check [and fix] this??  One way could be to abort in
kbd_buffer_store_event_hold if called without input blocked.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 05 Dec 2006 23:51:03 +0100, storm@cua.dk (Kim F. Storm) said:

> AFAICS, kbd_buffer_store_event[_hold] is usually called inside
> BLOCK_INPUT, but there may be a few places where this is not the
> case, notably in keyboard.c (and in the signal handler).

> Would someone pls. check [and fix] this??  One way could be to abort
> in kbd_buffer_store_event_hold if called without input blocked.

All indirect calls from (*read_socket_hook), Fx_file_dialog,
xmenu_show, or xdialog_show are inside BLOCK_INPUT.  But some calls
are not, because the current code (except handle_user_signal) assumes
that at most one signal handler will call kbd_buffer_store_event_hold
asynchronously, I think.

  * record_asynch_buffer_change either blocks SIGIO or stops polling
    before calling kbd_buffer_store_event_hold.

  * The remaining case is the calls from read_avail_input:

    - gobble_input -> read_avail_input:
      Like record_asynch_buffer_change above.

    - input_available_signal -> handle_async_input -> read_avail_input:
      Inside the SIGIO handler and not interrupted by the same signal.

    - Other handle_async_input -> read_avail_input:
      Inside #ifdef SYNC_INPUT.

    If polling used:

    - poll_for_input -> poll_for_input_1 -> read_avail_input:
      Inside the SIGALRM handler and not interrupted by the same signal.

    - x_make_frame_visible -> poll_for_input_1 -> read_avail_input:
      poll_suppress_count is set.

    - read_char -> kbd_buffer_get_event -> read_avail_input:
      After STOP_POLLING.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

Thank you very much for the analysis.

Could you please try to fix the problematic cases?


YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>>>>>> On Tue, 05 Dec 2006 23:51:03 +0100, storm@cua.dk (Kim F. Storm) said:
>
>> AFAICS, kbd_buffer_store_event[_hold] is usually called inside
>> BLOCK_INPUT, but there may be a few places where this is not the
>> case, notably in keyboard.c (and in the signal handler).
>
>> Would someone pls. check [and fix] this??  One way could be to abort
>> in kbd_buffer_store_event_hold if called without input blocked.
>
> All indirect calls from (*read_socket_hook), Fx_file_dialog,
> xmenu_show, or xdialog_show are inside BLOCK_INPUT.  But some calls
> are not, because the current code (except handle_user_signal) assumes
> that at most one signal handler will call kbd_buffer_store_event_hold
> asynchronously, I think.
>
>   * record_asynch_buffer_change either blocks SIGIO or stops polling
>     before calling kbd_buffer_store_event_hold.
>
>   * The remaining case is the calls from read_avail_input:
>
>     - gobble_input -> read_avail_input:
>       Like record_asynch_buffer_change above.
>
>     - input_available_signal -> handle_async_input -> read_avail_input:
>       Inside the SIGIO handler and not interrupted by the same signal.
>
>     - Other handle_async_input -> read_avail_input:
>       Inside #ifdef SYNC_INPUT.
>
>     If polling used:
>
>     - poll_for_input -> poll_for_input_1 -> read_avail_input:
>       Inside the SIGALRM handler and not interrupted by the same signal.
>
>     - x_make_frame_visible -> poll_for_input_1 -> read_avail_input:
>       poll_suppress_count is set.
>
>     - read_char -> kbd_buffer_get_event -> read_avail_input:
>       After STOP_POLLING.
>
> 				     YAMAMOTO Mitsuharu
> 				mituharu@math.s.chiba-u.ac.jp
>
>

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 11 Dec 2006 10:41:30 +0100, storm@cua.dk (Kim F. Storm) said:

> Could you please try to fix the problematic cases?

I don't think I understand your whole plan to fix the
handle_user_signal issue.  Could you explain that?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>>>>>> On Mon, 11 Dec 2006 10:41:30 +0100, storm@cua.dk (Kim F. Storm) said:
>
>> Could you please try to fix the problematic cases?
>
> I don't think I understand your whole plan to fix the
> handle_user_signal issue.  Could you explain that?

I guess the safest thing would be if the signal handler didn't
stuff the keyboard events itself.

What if the signal handlers just increment a global counter (one for
each signal type), and the main loop in keyboard.c checked those
counters and added the pending signal events in the "safe context"?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 12 Dec 2006 10:47:09 +0100, storm@cua.dk (Kim F. Storm) said:

> What if the signal handlers just increment a global counter (one for
> each signal type), and the main loop in keyboard.c checked those
> counters and added the pending signal events in the "safe context"?

There're two problems I can think of with this approach.

First, it will cause a race condition.  I guess the global counters
will be checked just before `select' is called in
wait_reading_process_output.  But if SIGUSR1 is delivered between the
check of the counters and the call to `select', it does not interrupt
`select' and will not be noticed until another input/signal becomes
available or the timeout expires.  This issue is discussed in [1].

Second, the current `select' emulation code in the Carbon port does
not wait for signals, but only for window system events and process
outputs via sockets.

In principle, both of them can be solved by converting signal delivery
to data delivery on a pipe or a UNIX domain socket as in [1].  But
this will require not-so-small changes, and I fear this becomes
another sit-for case.

Is it possible to postpone this feature to after the release?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

[1] Unix Network Programming, Volume 1: The Sockets Networking API,
3rd Edition, Chapter 20, Section 5.

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>>>>>> On Tue, 12 Dec 2006 10:47:09 +0100, storm@cua.dk (Kim F. Storm) said:
>
>> What if the signal handlers just increment a global counter (one for
>> each signal type), and the main loop in keyboard.c checked those
>> counters and added the pending signal events in the "safe context"?
>
> There're two problems I can think of with this approach.

You are absolutely right about those problems, but they both existed
before.  In both cases, they only mean a delay in delivering the
signal event, which already was a problem in the original code.

The problem I would like to solve before the release is that of
a SIGUSRx potentially messing up the keyboard event queue.

Using a global flag or counter could solve that.

> In principle, both of them can be solved by converting signal delivery
> to data delivery on a pipe or a UNIX domain socket as in [1].  But
> this will require not-so-small changes, and I fear this becomes
> another sit-for case.
>
> Is it possible to postpone this feature to after the release?

Yes, we should just solve the event mixup, not the delay.

Can you work on that, please?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 12 Dec 2006 14:54:06 +0100, storm@cua.dk (Kim F. Storm) said:

>>> What if the signal handlers just increment a global counter (one
>>> for each signal type), and the main loop in keyboard.c checked
>>> those counters and added the pending signal events in the "safe
>>> context"?
>> 
>> There're two problems I can think of with this approach.

> You are absolutely right about those problems, but they both existed
> before.  In both cases, they only mean a delay in delivering the
> signal event, which already was a problem in the original code.

I tried minimizing the first problem, but the second one still
remains.

BTW, is it necessary for us to read these events by read-key-sequence?
If not, it looks natural to bind them in special-event-map.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Index: src/emacs.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/emacs.c,v
retrieving revision 1.394
diff -c -p -r1.394 emacs.c
*** src/emacs.c	8 Dec 2006 00:20:17 -0000	1.394
--- src/emacs.c	13 Dec 2006 09:35:28 -0000
*************** pthread_t main_thread;
*** 361,384 ****
  #endif
  
  
- #if defined (SIGUSR1) || defined (SIGUSR2)
- SIGTYPE
- handle_user_signal (sig)
-      int sig;
- {
-   struct input_event buf;
- 
-   SIGNAL_THREAD_CHECK (sig);
-   bzero (&buf, sizeof buf);
-   buf.kind = USER_SIGNAL_EVENT;
-   buf.frame_or_window = selected_frame;
- 
-   kbd_buffer_store_event (&buf);
-   buf.code = sig;
-   kbd_buffer_store_event (&buf);
- }
- #endif
- 
  /* Handle bus errors, invalid instruction, etc.  */
  SIGTYPE
  fatal_error_signal (sig)
--- 361,366 ----
*************** main (argc, argv
*** 1211,1220 ****
        signal (SIGILL, fatal_error_signal);
        signal (SIGTRAP, fatal_error_signal);
  #ifdef SIGUSR1
!       signal (SIGUSR1, handle_user_signal);
  #endif
  #ifdef SIGUSR2
!       signal (SIGUSR2, handle_user_signal);
  #endif
  #ifdef SIGABRT
        signal (SIGABRT, fatal_error_signal);
--- 1193,1202 ----
        signal (SIGILL, fatal_error_signal);
        signal (SIGTRAP, fatal_error_signal);
  #ifdef SIGUSR1
!       add_user_signal (SIGUSR1, "usr1");
  #endif
  #ifdef SIGUSR2
!       add_user_signal (SIGUSR2, "usr2");
  #endif
  #ifdef SIGABRT
        signal (SIGABRT, fatal_error_signal);
Index: src/keyboard.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/keyboard.c,v
retrieving revision 1.882
diff -c -p -r1.882 keyboard.c
*** src/keyboard.c	4 Dec 2006 12:37:55 -0000	1.882
--- src/keyboard.c	13 Dec 2006 09:35:29 -0000
*************** static SIGTYPE interrupt_signal P_ ((int
*** 699,704 ****
--- 699,707 ----
  static void timer_start_idle P_ ((void));
  static void timer_stop_idle P_ ((void));
  static void timer_resume_idle P_ ((void));
+ static SIGTYPE handle_user_signal P_ ((int));
+ static char *find_user_signal_name P_ ((int));
+ static int store_user_signal_events P_ ((void));
  
  /* Nonzero means don't try to suspend even if the operating system seems
     to support it.  */
*************** make_lispy_event (event)
*** 5948,5967 ****
  
      case USER_SIGNAL_EVENT:
        /* A user signal.  */
!       switch (event->code)
  	{
! 	case 0:
! 	  return Qsignal;
! #ifdef SIGUSR1
! 	case SIGUSR1:
! 	  return intern ("usr1");
! #endif
! #ifdef SIGUSR2
! 	case SIGUSR2:
! 	  return intern ("usr2");
! #endif
! 	default:
! 	  return make_number (event->code);
  	}
  
      case SAVE_SESSION_EVENT:
--- 5951,5966 ----
  
      case USER_SIGNAL_EVENT:
        /* A user signal.  */
!       if (event->code == 0)
! 	return Qsignal;
!       else
  	{
! 	  char *name = find_user_signal_name (event->code);
! 
! 	  if (name)
! 	    return intern (name);
! 	  else
! 	    return make_number (event->code);
  	}
  
      case SAVE_SESSION_EVENT:
*************** read_avail_input (expected)
*** 6799,6804 ****
--- 6798,6807 ----
    register int i;
    int nread = 0;
  
+   /* Store pending user signal events, if any.  */
+   if (store_user_signal_events ())
+     expected = 0;
+ 
    if (read_socket_hook)
      {
        int nr;
*************** reinvoke_input_signal ()
*** 7022,7027 ****
--- 7025,7157 ----
  
  
  \f
+ /* User signal events.  */
+ 
+ struct user_signal_info
+ {
+   /* Signal number.  */
+   int sig;
+ 
+   /* Name of the signal.  */
+   char *name;
+ 
+   /* Number of pending signals.  */
+   int npending;
+ 
+   struct user_signal_info *next;
+ };
+ 
+ /* List of user signals. */
+ static struct user_signal_info *user_signals = NULL;
+ 
+ void
+ add_user_signal (sig, name)
+      int sig;
+      const char *name;
+ {
+   struct user_signal_info *p;
+ 
+   for (p = user_signals; p; p = p->next)
+     if (p->sig == sig)
+       /* Already added.  */
+       return;
+ 
+   p = xmalloc (sizeof (struct user_signal_info));
+   p->sig = sig;
+   p->name = xstrdup (name);
+   p->npending = 0;
+   p->next = user_signals;
+   user_signals = p;
+ 
+   signal (sig, handle_user_signal);
+ }
+ 
+ static SIGTYPE
+ handle_user_signal (sig)
+      int sig;
+ {
+   int old_errno = errno;
+   struct user_signal_info *p;
+ 
+ #if defined (USG) && !defined (POSIX_SIGNALS)
+   /* USG systems forget handlers when they are used;
+      must reestablish each time */
+   signal (sig, handle_user_signal);
+ #endif
+ 
+   SIGNAL_THREAD_CHECK (sig);
+ 
+   for (p = user_signals; p; p = p->next)
+     if (p->sig == sig)
+       {
+ 	p->npending++;
+ #ifdef SIGIO
+ 	if (interrupt_input)
+ 	  kill (getpid (), SIGIO);
+ 	else
+ #endif
+ 	  {
+ 	    /* Tell wait_reading_process_output that it needs to wake
+ 	       up and look around.  */
+ 	    if (input_available_clear_time)
+ 	      EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
+ 	  }
+ 	break;
+       }
+ 
+   errno = old_errno;
+ }
+ 
+ static char *
+ find_user_signal_name (sig)
+      int sig;
+ {
+   struct user_signal_info *p;
+ 
+   for (p = user_signals; p; p = p->next)
+     if (p->sig == sig)
+       return p->name;
+ 
+   return NULL;
+ }
+ 
+ static int
+ store_user_signal_events ()
+ {
+   struct user_signal_info *p;
+   struct input_event buf;
+   int nstored = 0;
+ 
+   for (p = user_signals; p; p = p->next)
+     if (p->npending > 0)
+       {
+ 	SIGMASKTYPE mask;
+ 
+ 	if (nstored == 0)
+ 	  {
+ 	    bzero (&buf, sizeof buf);
+ 	    buf.kind = USER_SIGNAL_EVENT;
+ 	    buf.frame_or_window = selected_frame;
+ 	  }
+ 	nstored += p->npending;
+ 
+ 	mask = sigblock (sigmask (p->sig));
+ 	do
+ 	  {
+ 	    buf.code = 0;
+ 	    kbd_buffer_store_event (&buf);
+ 	    buf.code = p->sig;
+ 	    kbd_buffer_store_event (&buf);
+ 	    p->npending--;
+ 	  }
+ 	while (p->npending > 0);
+ 	sigsetmask (mask);
+       }
+ 
+   return nstored;
+ }
+ 
+ \f
  static void menu_bar_item P_ ((Lisp_Object, Lisp_Object, Lisp_Object, void*));
  static Lisp_Object menu_bar_one_keymap_changed_items;
  
Index: src/keyboard.h
===================================================================
RCS file: /cvsroot/emacs/emacs/src/keyboard.h,v
retrieving revision 1.73
diff -c -p -r1.73 keyboard.h
*** src/keyboard.h	27 Aug 2006 07:09:14 -0000	1.73
--- src/keyboard.h	13 Dec 2006 09:35:29 -0000
*************** extern void gen_help_event P_ ((Lisp_Obj
*** 344,349 ****
--- 344,350 ----
  extern void kbd_buffer_store_help_event P_ ((Lisp_Object, Lisp_Object));
  extern Lisp_Object menu_item_eval_property P_ ((Lisp_Object));
  extern int  kbd_buffer_events_waiting P_ ((int));
+ extern void add_user_signals P_ ((int, const char *));
  
  /* arch-tag: 769cbade-1ba9-4950-b886-db265b061aa3
     (do not change this comment) */

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>>>>>> On Tue, 12 Dec 2006 14:54:06 +0100, storm@cua.dk (Kim F. Storm) said:
>
>>>> What if the signal handlers just increment a global counter (one
>>>> for each signal type), and the main loop in keyboard.c checked
>>>> those counters and added the pending signal events in the "safe
>>>> context"?
>>> 
>>> There're two problems I can think of with this approach.
>
>> You are absolutely right about those problems, but they both existed
>> before.  In both cases, they only mean a delay in delivering the
>> signal event, which already was a problem in the original code.
>
> I tried minimizing the first problem, but the second one still
> remains.

Thank you very much.

The changes look good, so please install them.

>
> BTW, is it necessary for us to read these events by read-key-sequence?
> If not, it looks natural to bind them in special-event-map.

You have a good point there!  If we bind signals in special-event-map,
we don't really have to care about them being mixed up with the rest
of the keyboard events ...

OTOH, if we put them in the special-event-map, we make it practically
impossible for a (global) minor-mode to setup catching a signal
through its "private" keymaps.  So keeping signals in the
read-key-sequence loop is definitely more flexible.

Also, although the definition of special-event-map doesn't explicitly
say so, it only allows bindings for single events.  This means that we
would have to revert to having just a single event for signals.

So I suggest installing your changes, and otherwise keep things as they are.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 13 Dec 2006 11:26:53 +0100, storm@cua.dk (Kim F. Storm) said:

>> I tried minimizing the first problem, but the second one still
>> remains.

> Thank you very much.

> The changes look good, so please install them.

Done.

>> BTW, is it necessary for us to read these events by
>> read-key-sequence?  If not, it looks natural to bind them in
>> special-event-map.

> You have a good point there!  If we bind signals in
> special-event-map, we don't really have to care about them being
> mixed up with the rest of the keyboard events ...

Yes.  The Emacs Lisp info says:

  `signal usr1'
  `signal usr2'
       These event sequences are generated when the Emacs process receives
       the signals `SIGUSR1' and `SIGUSR2'.  They contain no additional
       data because signals do not carry additional information.

     If one of these events arrives in the middle of a key sequence--that
  is, after a prefix key--then Emacs reorders the events so that this
  event comes either before or after the multi-event key sequence, not
  within it.

But currently they do not behave as above.

> OTOH, if we put them in the special-event-map, we make it
> practically impossible for a (global) minor-mode to setup catching a
> signal through its "private" keymaps.  So keeping signals in the
> read-key-sequence loop is definitely more flexible.

> Also, although the definition of special-event-map doesn't
> explicitly say so, it only allows bindings for single events.  This
> means that we would have to revert to having just a single event for
> signals.

One possible way would be to generate an intermediate single event
that carries a signal number, and bind such an event to a dispatcher
command in special-event-map.  Then the dispatcher looks up the
corresponding (multiple) key sequence and executes the associated
command.  `mac-dispatch-apple-event' in term/mac-win.el handles Apple
Events in such a way so these events may not be mixed up with a usual
key sequence.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>      If one of these events arrives in the middle of a key sequence--that
>   is, after a prefix key--then Emacs reorders the events so that this
>   event comes either before or after the multi-event key sequence, not
>   within it.
>
> But currently they do not behave as above.

That's no good!

This is a good reason for handling signals through special-event-map.

> One possible way would be to generate an intermediate single event
> that carries a signal number, and bind such an event to a dispatcher
> command in special-event-map.  Then the dispatcher looks up the
> corresponding (multiple) key sequence and executes the associated
> command.  `mac-dispatch-apple-event' in term/mac-win.el handles Apple
> Events in such a way so these events may not be mixed up with a usual
> key sequence.

IMO, signal handling is not important enough for that complexity.

So I would rather redefine them to single events (named sigusr1 and
sigusr2 as those names are already known to the signal-process
function), and handle them through special-event-map as you suggested.

The docs you quote above should be changed to say that signals should
be handled through special-event-map, or signals may risk messing up the
normal event processing.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[Chong Yidong]

storm@cua.dk (Kim F. Storm) writes:

> So I would rather redefine them to single events (named sigusr1 and
> sigusr2 as those names are already known to the signal-process
> function), and handle them through special-event-map as you suggested.

In that case, is there a rationale for the rather large changes
checked into emacs.c, keyboard.c, and process.c over the last two
weeks?  Should they not be reverted?

Re: usr1-signal, usr2-signal, etc.

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 18 Dec 2006 11:38:21 -0500, Chong Yidong <cyd@stupidchicken.com> said:

> storm@cua.dk (Kim F. Storm) writes:
>> So I would rather redefine them to single events (named sigusr1 and
>> sigusr2 as those names are already known to the signal-process
>> function), and handle them through special-event-map as you
>> suggested.

> In that case, is there a rationale for the rather large changes
> checked into emacs.c, keyboard.c, and process.c over the last two
> weeks?  Should they not be reverted?

Do the "large changes" you think should be reverted include my
2006-12-14 changes?  These changes are not for the event format but
for fixing a problem that has existed for a long time.  That is, about
calling a non-reentrant function in a signal handler context while it
is executed in a normal context.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>>>>>> On Mon, 18 Dec 2006 11:38:21 -0500, Chong Yidong <cyd@stupidchicken.com> said:
>
>> storm@cua.dk (Kim F. Storm) writes:
>>> So I would rather redefine them to single events (named sigusr1 and
>>> sigusr2 as those names are already known to the signal-process
>>> function), and handle them through special-event-map as you
>>> suggested.
>
>> In that case, is there a rationale for the rather large changes
>> checked into emacs.c, keyboard.c, and process.c over the last two
>> weeks?  Should they not be reverted?
>
> Do the "large changes" you think should be reverted include my
> 2006-12-14 changes?  These changes are not for the event format but
> for fixing a problem that has existed for a long time.  That is, about
> calling a non-reentrant function in a signal handler context while it
> is executed in a normal context.

Yes.  Those changes are very important on their own.

It still think that handling signals through special-event-map is
the proper (or the least problematic) thing to do.  So unless I
hear any objections, I'll make that change later this week.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:

> It still think that handling signals through special-event-map is
> the proper (or the least problematic) thing to do.  So unless I
> hear any objections, I'll make that change later this week.

"later this week" => this afternoon :-)

Done!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[Richard Stallman]

    It still think that handling signals through special-event-map is
    the proper (or the least problematic) thing to do.  So unless I
    hear any objections, I'll make that change later this week.

Ok with me.

Re: usr1-signal, usr2-signal, etc.

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     It still think that handling signals through special-event-map is
>     the proper (or the least problematic) thing to do.  So unless I
>     hear any objections, I'll make that change later this week.
>
> Ok with me.

Done!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: usr1-signal, usr2-signal, etc.

[Richard Stallman]

The feature of handling these signals is not terribly important.
If fixing it is hard, we can drop the feature.  I don't like
dropping features, but this one being so minor is ok to drop.

Re: usr1-signal, usr2-signal, etc.

[Richard Stallman]

    I'm concerned about the case that kbd_buffer_store_event_hold is
    called from the signal-handler context while it is also executed in
    the normal context.  This has already existed for SIGUSR1 and SIGUSR2
    before your change, though.

Since people hardly ever use SIGUSR1 and SIGUSR2,
a bug that happens only a small fraction of the time when they
are called might never have happened.  But if it happens for
signals that are more common, it will start to occur.
So it is good that you noticed this.

We should fix it now.

Re: usr1-signal, usr2-signal, etc.

[Johan Bockgård]

storm@cua.dk (Kim F. Storm) writes:

> Done.

There's an item in NEWS that should be updated:

    ** When Emacs receives a USR1 or USR2 signal, this generates an
    input event: usr1-signal or usr2-signal.

-- 
Johan Bockgård