[roland/sigvec] Remove sigvec.

Message ID 20141009193327.B7BD02C3ACF@topped-with-meat.com
State Not applicable
Headers

Commit Message

Roland McGrath Oct. 9, 2014, 7:33 p.m. UTC
  > Uhm, the current lv package in Fedora and Debian uses it:

Indeed.  Surprising.  The use of SV_INTERRUPT means it would actually
change the behavior to simply start failing that configure check and use
plain signal instead of sigvec.  (signal by default is equivalent to
sigaction using SA_RESTART; SV_INTERRUPT is the inverse of SA_RESTART.)

It's of course a simple 5-minute job to convert any old use of sigvec to
use sigaction instead.  I did it for lv and the patch is below.  Would you
like to take care of sending that to the lv maintainer and/or using it in
the Fedora package?

This is a case where the plain removal of sigvec would not have broken the
package's build and would instead have subtly changed the behavior of the
program.  So it certainly seems like a red flag.  But I'm not sure that a
staged deprecation plan would do any better on that score.  There would be
compile-time warnings about using a deprecated interface, but that would
not have broken the package's build either.  Perhaps distros will (should?
do?) build with -Werror=deprecated-declarations specifically to ensure such
things get caught.

Do you think we should do a staged deprecation for sigvec instead?


Thanks,
Roland
  

Comments

Florian Weimer Oct. 13, 2014, 8:59 a.m. UTC | #1
On 10/09/2014 09:33 PM, Roland McGrath wrote:
>> Uhm, the current lv package in Fedora and Debian uses it:
>
> Indeed.  Surprising.  The use of SV_INTERRUPT means it would actually
> change the behavior to simply start failing that configure check and use
> plain signal instead of sigvec.  (signal by default is equivalent to
> sigaction using SA_RESTART; SV_INTERRUPT is the inverse of SA_RESTART.)
>
> It's of course a simple 5-minute job to convert any old use of sigvec to
> use sigaction instead.  I did it for lv and the patch is below.  Would you
> like to take care of sending that to the lv maintainer and/or using it in
> the Fedora package?

I've submitted your patch to Fedora:

   <https://bugzilla.redhat.com/show_bug.cgi?id=1151982>

> Do you think we should do a staged deprecation for sigvec instead?

Probably not, especially since we aren't actually removing sigvec as 
such (the subject got me worried for a moment).
  
Roland McGrath Oct. 13, 2014, 8:24 p.m. UTC | #2
> I've submitted your patch to Fedora:
> 
>    <https://bugzilla.redhat.com/show_bug.cgi?id=1151982>

Thanks.

> > Do you think we should do a staged deprecation for sigvec instead?
> 
> Probably not, especially since we aren't actually removing sigvec as 
> such (the subject got me worried for a moment).

We are indeed removing sigvec from the API.
Of course we are not breaking ABI compatibility; that goes without saying.


Thanks,
Roland
  
Florian Weimer Nov. 14, 2014, 12:34 p.m. UTC | #3
On 10/13/2014 10:24 PM, Roland McGrath wrote:
>> I've submitted your patch to Fedora:
>>
>>     <https://bugzilla.redhat.com/show_bug.cgi?id=1151982>
>
> Thanks.

Also reported to Debian

   <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=769546>

>>> Do you think we should do a staged deprecation for sigvec instead?
>>
>> Probably not, especially since we aren't actually removing sigvec as
>> such (the subject got me worried for a moment).
>
> We are indeed removing sigvec from the API.
> Of course we are not breaking ABI compatibility; that goes without saying.

For the record, I think this change can go ahead.
  
Roland McGrath Nov. 14, 2014, 7:07 p.m. UTC | #4
Committed.  check-abi passes and there are no changes to generated code
except for the static sigvec.o going away.

Thanks,
Roland
  
Stefan Liebler Nov. 17, 2014, 7:53 a.m. UTC | #5
Hi,

make install fails due:

  LANGUAGE=C LC_ALL=C makeinfo -P build-dir/manual/ 
--output=build-dir/manual/libc.info libc.texinfo
src-dir/manual//signal.texi:2142: Cross reference to nonexistent node 
`BSD Handler' (perhaps incorrect sectioning?).
makeinfo: Removing output file `build-dir/manual/libc.info' due to 
errors; use --force to preserve.

Bye Stefan

On 11/14/2014 08:07 PM, Roland McGrath wrote:
> Committed.  check-abi passes and there are no changes to generated code
> except for the static sigvec.o going away.
>
> Thanks,
> Roland
>
  

Patch

--- ./src/configure.in.~1~	2004-01-04 22:35:44.000000000 -0800
+++ ./src/configure.in	2014-10-09 11:14:47.782210631 -0700
@@ -34,7 +34,7 @@  AC_CHECK_HEADERS(fcntl.h sys/ioctl.h sys
 dnl Checks for typedefs, structures, and compiler characteristics.
 
 dnl Checks for library functions.
-AC_CHECK_FUNCS(sigvec tgetnum setlocale)
+AC_CHECK_FUNCS(sigaction tgetnum setlocale)
 AC_FUNC_GETPGRP
 AC_PROG_GCC_TRADITIONAL
 AC_TYPE_SIGNAL
--- ./src/console.c.~1~	2004-01-04 23:27:46.000000000 -0800
+++ ./src/console.c	2014-10-09 11:16:59.627943378 -0700
@@ -158,9 +158,9 @@  private RETSIGTYPE InterruptHandler( int
 {
   kb_interrupted = TRUE;
 
-#ifndef HAVE_SIGVEC
+#ifndef HAVE_SIGACTION
   signal( SIGINT, InterruptHandler );
-#endif /* HAVE_SIGVEC */
+#endif /* HAVE_SIGACTION */
 }
 
 public void ConsoleEnableInterrupt()
@@ -235,9 +235,9 @@  private RETSIGTYPE WindowChangeHandler(
 
   ConsoleGetWindowSize();
 
-#ifndef HAVE_SIGVEC
+#ifndef HAVE_SIGACTION
   signal( SIGWINCH, WindowChangeHandler );
-#endif /* HAVE_SIGVEC */
+#endif /* HAVE_SIGACTION */
 }
 #endif /* UNIX */
 
@@ -388,24 +388,24 @@  public void ConsoleSetUp()
   signal( SIGINT, InterruptIgnoreHandler );
 #endif /* MSDOS */
 
-#ifdef HAVE_SIGVEC
-  struct sigvec sigVec;
+#ifdef HAVE_SIGACTION
+  struct sigaction sa;
 
-  sigVec.sv_handler = WindowChangeHandler;
-  sigVec.sv_mask = sigmask( SIGWINCH );
-  sigVec.sv_flags = SV_INTERRUPT;
-  sigvec( SIGWINCH, &sigVec, NULL );
-
-  sigVec.sv_handler = InterruptHandler;
-  sigVec.sv_mask = sigmask( SIGINT );
-  sigVec.sv_flags = SV_INTERRUPT;
-  sigvec( SIGINT, &sigVec, NULL );
+  sa.sa_handler = WindowChangeHandler;
+  sigemptyset( &sa.sa_mask );
+  sa.sa_flags = 0;           /* No SA_RESTART means interrupt syscalls.  */
+  sigaction( SIGWINCH, &sa, NULL );
+
+  sa.sa_handler = InterruptHandler;
+  sigemptyset( &sa.sa_mask );
+  sa.sa_flags = 0;           /* No SA_RESTART means interrupt syscalls.  */
+  sigaction( SIGINT, &sa, NULL );
 #else
 # ifdef SIGWINCH
   signal( SIGWINCH, WindowChangeHandler );
 # endif 
   signal( SIGINT, InterruptHandler );
-#endif /* HAVE_SIGVEC */
+#endif /* HAVE_SIGACTION */
 
 #ifdef UNIX
 #ifdef HAVE_TERMIOS_H