GNU bug report logs -
#9076
coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally
Previous Next
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9076 in the body.
You can then email your comments to 9076 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Thu, 14 Jul 2011 06:56:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
"Joachim Schmitz" <jojo <at> schmitz-digital.de>:
New bug report received and forwarded. Copy sent to
bug-coreutils <at> gnu.org.
(Thu, 14 Jul 2011 06:56:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi folks
coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally and on HP
NonStop these don't exist.
SA_RESETHAND is used in csplit.c and dd.c
SA_RESTART is used in ls.c and timeout.c
Bye, Jojo
[Message part 2 (text/html, inline)]
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Thu, 14 Jul 2011 11:24:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 9076 <at> debbugs.gnu.org (full text, mbox):
On 14/07/11 07:54, Joachim Schmitz wrote:
> Hi folks
>
>
>
> coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally and on HP
> NonStop these don't exist.
Ok we'll need to add the above info to:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=doc/posix-functions/sigaction.texi;hb=HEAD
> SA_RESETHAND is used in csplit.c and dd.c
> SA_RESTART is used in ls.c and timeout.c
Well we can define these to 0 to get the compile to succeed.
However can you provide info on how to achieve the behavior
of SA_RESETHAND and SA_RESTART on your platform.
cheers,
Pádraig.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Thu, 14 Jul 2011 16:47:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 9076 <at> debbugs.gnu.org (full text, mbox):
I'm afraid I have no idea how or whether HP NonStop implements this
functionality.
I can point to the manual, Open System Services System CallsReferenceManual,
at:
http://bizsupport2.austin.hp.com/bc/docs/support/SupportManual/c02128682/c02
128682.pdf
Bye, Jojo
-----Original Message-----
From: Pádraig Brady [mailto:P <at> draigBrady.com]
Sent: Thursday, July 14, 2011 1:22 PM
To: Joachim Schmitz
Cc: 9076 <at> debbugs.gnu.org
Subject: Re: bug#9076: coreutils-8.12 uses SA_RESETHAND and SA_RESTART
unconditionally
On 14/07/11 07:54, Joachim Schmitz wrote:
> Hi folks
>
>
>
> coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally and on
> HP NonStop these don't exist.
Ok we'll need to add the above info to:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=doc/posix-function
s/sigaction.texi;hb=HEAD
> SA_RESETHAND is used in csplit.c and dd.c SA_RESTART is used in ls.c
> and timeout.c
Well we can define these to 0 to get the compile to succeed.
However can you provide info on how to achieve the behavior of SA_RESETHAND
and SA_RESTART on your platform.
cheers,
Pádraig.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Thu, 14 Jul 2011 22:34:03 GMT)
Full text and
rfc822 format available.
Message #14 received at 9076 <at> debbugs.gnu.org (full text, mbox):
[CC'ing bug-gnulib re <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=9076>.]
On 07/13/11 23:54, Joachim Schmitz wrote:
> coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally and on HP
> NonStop these don't exist.
Thanks for reporting this. I think
coreutils used to be portable to hosts lacking those symbols,
and there's a bit of code in dd.c that still assumes the
old backward-compatibility stuff:
static void
interrupt_handler (int sig)
{
if (! SA_RESETHAND)
signal (sig, SIG_DFL);
interrupt_signal = sig;
}
but I guess it got ripped out at some point, under the mistaken
assumption that everybody has SA_RESETHAND nowadays. Here's a proposed patch
to gnulib to address this porting problem.
* lib/signal.in.h (SA_RESETHAND, SA_RESTART): Default to 0.
* doc/posix-functions/sigaction.texi (sigaction): Document this,
and document NonStop portability issues. See Joachim Schmitz in
<http://lists.gnu.org/archive/html/bug-coreutils/2011-07/msg00062.html>.
diff --git a/doc/posix-functions/sigaction.texi b/doc/posix-functions/sigaction.texi
index d03e516..583d112 100644
--- a/doc/posix-functions/sigaction.texi
+++ b/doc/posix-functions/sigaction.texi
@@ -11,6 +11,11 @@ Portability problems fixed by Gnulib:
@item
This function is missing on some platforms:
mingw.
+
+@item
+Some systems do not define the flag SA_NODEFER.
+NonStop does not define the flags SA_RESETHAND and SA_RESTART.
+Gnulib defines missing flags to zero, which means they have no effect.
@end itemize
Portability problems not fixed by Gnulib:
@@ -33,8 +38,12 @@ missing on some platforms:
mingw.
@item
+Support for SA_RESETHAND is missing on some platforms:
+NonStop.
+
+@item
Support for SA_RESTART is missing on some platforms:
-mingw.
+mingw, NonStop.
@item
In spite of having SA_SIGACTION, struct sigaction lacks the
diff --git a/lib/signal.in.h b/lib/signal.in.h
index 93787f7..6765fa3 100644
--- a/lib/signal.in.h
+++ b/lib/signal.in.h
@@ -417,10 +417,16 @@ _GL_WARN_ON_USE (sigaction, "sigaction is unportable - "
# endif
#endif
-/* Some systems don't have SA_NODEFER. */
+/* Some systems don't have flags. They default to 0, i.e., no effect. */
#ifndef SA_NODEFER
# define SA_NODEFER 0
#endif
+#ifndef SA_RESETHAND
+# define SA_RESETHAND 0
+#endif
+#ifndef SA_RESTART
+# define SA_RESTART 0
+#endif
#endif /* _ <at> GUARD_PREFIX <at> _SIGNAL_H */
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Fri, 15 Jul 2011 00:28:03 GMT)
Full text and
rfc822 format available.
Message #17 received at 9076 <at> debbugs.gnu.org (full text, mbox):
On 14/07/11 23:33, Paul Eggert wrote:
> [CC'ing bug-gnulib re <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=9076>.]
>
> On 07/13/11 23:54, Joachim Schmitz wrote:
>
>> coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally and on HP
>> NonStop these don't exist.
>
> Thanks for reporting this. I think
> coreutils used to be portable to hosts lacking those symbols,
> and there's a bit of code in dd.c that still assumes the
> old backward-compatibility stuff:
>
> static void
> interrupt_handler (int sig)
> {
> if (! SA_RESETHAND)
> signal (sig, SIG_DFL);
> interrupt_signal = sig;
> }
>
> but I guess it got ripped out at some point, under the mistaken
> assumption that everybody has SA_RESETHAND nowadays. Here's a proposed patch
> to gnulib to address this porting problem.
I'm not sure about defining these to 0 in gnulib.
That will silently ignore the intent of a program on certain platforms.
Wouldn't it be better to fail to compile so that each program then does:
#ifndef SA_RESETHAND
/* do something else */
#endif
cheers,
Pádraig.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Fri, 15 Jul 2011 07:51:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 9076 <at> debbugs.gnu.org (full text, mbox):
On 07/14/11 17:25, Pádraig Brady wrote:
> I'm not sure about defining these to 0 in gnulib.
> That will silently ignore the intent of a program on certain platforms.
> Wouldn't it be better to fail to compile so that each program then does:
>
> #ifndef SA_RESETHAND
> /* do something else */
> #endif
Well, as a matter of style, I prefer this:
if (! SA_RESETHAND)
do_something_else ();
as that's much more likely to detect bitrot in the "do something else"
part. (Of course this is all assuming the application writer knows
about platforms where SA_RESETHAND isn't implemented.)
On NonStop, if you invoke signal(), it uses the SA_RESETHAND semantics
(POSIX allows this). Conversely, if you invoke sigaction(), NonStop
always behaves as if SA_RESTART and SA_RESETHAND are zero, i.e., it
doesn't support either feature with sigaction.
I just now checked coreutils. Two programs use SA_RESETHAND. The
first (csplit) is clearly using it incorrectly, and won't work
properly on any host that implements SA_RESETHAND according to POSIX.
The second (dd) is using the above idiom already, that is, it is
assuming SA_RESETHAND is #defined to 0 on hosts that don't support it.
(It has some obsolete comments though, that need fixing.)
Conversely, the two programs that depend on SA_RESTART will
probably misbehave if a signal interrupts a system call on
NonStop. I see no easy fix for this, and I expect the gnulib
change may be the best we can do.
Here are a couple of patches that address the SA_RESETHAND problems
mentioned above. They assume the gnulib change. They fix the csplit
bug (and also allow it to work on NonStop, assuming the gnulib
change).
diff --git a/src/csplit.c b/src/csplit.c
index 438d888..4a0914e 100644
--- a/src/csplit.c
+++ b/src/csplit.c
@@ -225,6 +225,9 @@ static void
interrupt_handler (int sig)
{
delete_all_files (true);
+
+ signal (sig, SIG_DFL);
+
/* The signal has been reset to SIG_DFL, but blocked during this
handler. Force the default action of this signal once the
handler returns and the block is removed. */
@@ -1421,7 +1424,7 @@ main (int argc, char **argv)
act.sa_handler = interrupt_handler;
act.sa_mask = caught_signals;
- act.sa_flags = SA_NODEFER | SA_RESETHAND;
+ act.sa_flags = 0;
for (i = 0; i < nsigs; i++)
if (sigismember (&caught_signals, sig[i]))
diff --git a/src/dd.c b/src/dd.c
index 45aa9b9..c30c78a 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -45,7 +45,7 @@
proper_name ("Stuart Kemp")
/* Use SA_NOCLDSTOP as a proxy for whether the sigaction machinery is
- present. SA_NODEFER and SA_RESETHAND are XSI extensions. */
+ present. */
#ifndef SA_NOCLDSTOP
# define SA_NOCLDSTOP 0
# define sigprocmask(How, Set, Oset) /* empty */
@@ -726,9 +726,6 @@ install_signal_handlers (void)
if (sigismember (&caught_signals, SIGINT))
{
- /* POSIX 1003.1-2001 says SA_RESETHAND implies SA_NODEFER,
- but this is not true on Solaris 8 at least. It doesn't
- hurt to use SA_NODEFER here, so leave it in. */
act.sa_handler = interrupt_handler;
act.sa_flags = SA_NODEFER | SA_RESETHAND;
sigaction (SIGINT, &act, NULL);
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Fri, 15 Jul 2011 10:30:04 GMT)
Full text and
rfc822 format available.
Message #23 received at 9076 <at> debbugs.gnu.org (full text, mbox):
Paul Eggert wrote:
> --- a/doc/posix-functions/sigaction.texi
> +++ b/doc/posix-functions/sigaction.texi
A documentation update for the header file, in doc/posix-headers/signal.texi,
would also be useful.
Bruno
--
In memoriam Natalya Estemirova <http://en.wikipedia.org/wiki/Natalya_Estemirova>
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Fri, 15 Jul 2011 10:31:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 9076 <at> debbugs.gnu.org (full text, mbox):
On 15/07/11 08:50, Paul Eggert wrote:
> On 07/14/11 17:25, Pádraig Brady wrote:
>> I'm not sure about defining these to 0 in gnulib.
>> That will silently ignore the intent of a program on certain platforms.
>> Wouldn't it be better to fail to compile so that each program then does:
>>
>> #ifndef SA_RESETHAND
>> /* do something else */
>> #endif
>
> Well, as a matter of style, I prefer this:
>
> if (! SA_RESETHAND)
> do_something_else ();
>
> as that's much more likely to detect bitrot in the "do something else"
> part.
Absolutely. What I was getting was that it's probably better to leave
the following to the app too:
#ifndef SA_RESETHAND
# define SA_RESETHAND 0
/* Now the app writer knows they need to handle this case */
#endif
Otherwise new users of sigaction() or whatever may not be aware
of portability issues. They're all documented, but they're too
easy to miss there IMHO.
> (Of course this is all assuming the application writer knows
> about platforms where SA_RESETHAND isn't implemented.)
>
> On NonStop, if you invoke signal(), it uses the SA_RESETHAND semantics
> (POSIX allows this). Conversely, if you invoke sigaction(), NonStop
> always behaves as if SA_RESTART and SA_RESETHAND are zero, i.e., it
> doesn't support either feature with sigaction.
Thanks for checking that.
> I just now checked coreutils. Two programs use SA_RESETHAND. The
> first (csplit) is clearly using it incorrectly, and won't work
> properly on any host that implements SA_RESETHAND according to POSIX.
agreed
> The second (dd) is using the above idiom already, that is, it is
> assuming SA_RESETHAND is #defined to 0 on hosts that don't support it.
> (It has some obsolete comments though, that need fixing.)
> Conversely, the two programs that depend on SA_RESTART will
> probably misbehave if a signal interrupts a system call on
> NonStop. I see no easy fix for this, and I expect the gnulib
> change may be the best we can do.
Again, that gnulib change would only paper over the issue.
cheers,
Pádraig.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Fri, 15 Jul 2011 12:56:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 9076 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 07/15/2011 04:28 AM, Pádraig Brady wrote:
> On 15/07/11 08:50, Paul Eggert wrote:
>> On 07/14/11 17:25, Pádraig Brady wrote:
>>> I'm not sure about defining these to 0 in gnulib.
>>> That will silently ignore the intent of a program on certain platforms.
> Absolutely. What I was getting was that it's probably better to leave
> the following to the app too:
>
> #ifndef SA_RESETHAND
> # define SA_RESETHAND 0
> /* Now the app writer knows they need to handle this case */
> #endif
Can't the gnulib sigaction module be taught to fake SA_RESETHAND, by
wrapping the user's signal-handler inside a gnulib shim, where the shim
resets signals correctly before calling the user's handler? Granted,
it's not trivial, but I think that it is still within the realm of
technical possibilities. At which point, SA_RESETHAND would always be
non-zero once you use the gnulib <signal.h>.
>> On NonStop, if you invoke signal(), it uses the SA_RESETHAND semantics
>> (POSIX allows this). Conversely, if you invoke sigaction(), NonStop
>> always behaves as if SA_RESTART and SA_RESETHAND are zero, i.e., it
>> doesn't support either feature with sigaction.
>
> Thanks for checking that.
Seems like there's some room for improvement in the gnulib sigaction
module then.
--
Eric Blake eblake <at> redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Sat, 16 Jul 2011 00:52:01 GMT)
Full text and
rfc822 format available.
Message #32 received at 9076 <at> debbugs.gnu.org (full text, mbox):
On 07/15/11 03:28, Pádraig Brady wrote:
> What I was getting was that it's probably better to leave
> the following to the app too:
>
> #ifndef SA_RESETHAND
> # define SA_RESETHAND 0
> /* Now the app writer knows they need to handle this case */
> #endif
Yes, you're probably right. I'll send some patches along
those lines shortly (but not to bug-gnulib since that's not
relevant to these coreutils-specific patches).
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Sat, 16 Jul 2011 01:12:01 GMT)
Full text and
rfc822 format available.
Message #35 received at 9076 <at> debbugs.gnu.org (full text, mbox):
Here are three proposed patches to coreutils to address
the SA_RESETHAND and SA_RESTART issues. I hope that
if you combine them with the patch I already installed:
http://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=837e1f55196f826b92d660808f594fde36651655
these should fix all the issues mentioned here.
Joachim, could you please try these? Thanks.
From bbd4c9edfa4bd5650106261b7d9b1dd434d91581 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 15 Jul 2011 17:38:32 -0700
Subject: [PATCH 1/3] dd: port to NonStop (Bug#9076)
* src/dd.c (SA_RESETHAND): Define to 0 if not defined.
---
src/dd.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/src/dd.c b/src/dd.c
index 3e75412..0824f6c 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -55,6 +55,11 @@
# endif
#endif
+/* NonStop circa 2011 lacks SA_RESETHAND; see Bug#9076. */
+#ifndef SA_RESETHAND
+# define SA_RESETHAND 0
+#endif
+
#ifndef SIGINFO
# define SIGINFO SIGUSR1
#endif
--
1.7.4.4
From 995299ff155bef70a3b153dc8cef11ed9b1d8904 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 15 Jul 2011 17:39:28 -0700
Subject: [PATCH 2/3] ls: port to NonStop (Bug#9076)
* src/ls.c (SA_RESTART): Define to 0 if not defined.
---
src/ls.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/src/ls.c b/src/ls.c
index c604e14..680a7c3 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -74,6 +74,14 @@
# endif
#endif
+/* NonStop circa 2011 lacks both SA_RESTART and siginterrupt, so don't
+ restart syscalls after a signal handler fires. This may cause
+ colors to get messed up on the screen if 'ls' is interrupted, but
+ that's the best we can do on such a platform. */
+#ifndef SA_RESTART
+# define SA_RESTART 0
+#endif
+
#include "system.h"
#include <fnmatch.h>
--
1.7.4.4
From 638e670d76b3bf573f6a9930b376811b5663881a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 15 Jul 2011 17:48:38 -0700
Subject: [PATCH 3/3] timeout: port to NonStop (Bug#9077)
* src/timeout.c (SA_RESTART): Define to 0 if not defined.
(main): Don't assume signal handling uses SA_RESTART.
---
src/timeout.c | 21 ++++++++++++---------
1 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/timeout.c b/src/timeout.c
index ef660a7..895d720 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -64,6 +64,11 @@
# include <sys/resource.h>
#endif
+/* NonStop circa 2011 lacks both SA_RESTART and siginterrupt. */
+#ifndef SA_RESTART
+# define SA_RESTART 0
+#endif
+
#define PROGRAM_NAME "timeout"
#define AUTHORS proper_name_utf8 ("Padraig Brady", "P\303\241draig Brady")
@@ -256,7 +261,8 @@ install_signal_handlers (int sigterm)
struct sigaction sa;
sigemptyset (&sa.sa_mask); /* Allow concurrent calls to handler */
sa.sa_handler = cleanup;
- sa.sa_flags = SA_RESTART; /* restart syscalls (like wait() below) */
+ sa.sa_flags = SA_RESTART; /* Restart syscalls if possible, as that's
+ more likely to work cleanly. */
sigaction (SIGALRM, &sa, NULL); /* our timeout. */
sigaction (SIGINT, &sa, NULL); /* Ctrl-C at terminal for example. */
@@ -354,18 +360,15 @@ main (int argc, char **argv)
}
else
{
+ pid_t wait_result;
int status;
alarm (timeout);
- /* We're just waiting for a single process here, so wait() suffices.
- Note the signal() calls above on GNU/Linux and BSD at least,
- essentially call the lower level sigaction() with the SA_RESTART flag
- set, which ensures the following wait call will only return if the
- child exits, not on this process receiving a signal. Also we're not
- passing WUNTRACED | WCONTINUED to a waitpid() call and so will not get
- indication that the child has stopped or continued. */
- if (wait (&status) == -1)
+ while ((wait_result = wait (&status)) < 0 && errno == EINTR)
+ continue;
+
+ if (wait_result < 0)
{
/* shouldn't happen. */
error (0, errno, _("error waiting for command"));
--
1.7.4.4
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Sat, 16 Jul 2011 08:39:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 9076 <at> debbugs.gnu.org (full text, mbox):
2011/7/15 Eric Blake <eblake <at> redhat.com>:
> On 07/15/2011 04:28 AM, Pádraig Brady wrote:
>> On 15/07/11 08:50, Paul Eggert wrote:
>>> On 07/14/11 17:25, Pádraig Brady wrote:
>>>> I'm not sure about defining these to 0 in gnulib.
>>>> That will silently ignore the intent of a program on certain platforms.
>
>> Absolutely. What I was getting was that it's probably better to leave
>> the following to the app too:
>>
>> #ifndef SA_RESETHAND
>> # define SA_RESETHAND 0
>> /* Now the app writer knows they need to handle this case */
>> #endif
>
> Can't the gnulib sigaction module be taught to fake SA_RESETHAND, by
> wrapping the user's signal-handler inside a gnulib shim, where the shim
> resets signals correctly before calling the user's handler? Granted,
> it's not trivial, but I think that it is still within the realm of
> technical possibilities. At which point, SA_RESETHAND would always be
> non-zero once you use the gnulib <signal.h>.
>
>>> On NonStop, if you invoke signal(), it uses the SA_RESETHAND semantics
>>> (POSIX allows this). Conversely, if you invoke sigaction(), NonStop
>>> always behaves as if SA_RESTART and SA_RESETHAND are zero, i.e., it
>>> doesn't support either feature with sigaction.
>>
>> Thanks for checking that.
>
> Seems like there's some room for improvement in the gnulib sigaction
> module then.
Could help to implement pipe to self for pselect... I favor this.
Bastien
Reply sent
to
Paul Eggert <eggert <at> cs.ucla.edu>:
You have taken responsibility.
(Sat, 16 Jul 2011 19:20:03 GMT)
Full text and
rfc822 format available.
Notification sent
to
"Joachim Schmitz" <jojo <at> schmitz-digital.de>:
bug acknowledged by developer.
(Sat, 16 Jul 2011 19:20:04 GMT)
Full text and
rfc822 format available.
Message #43 received at 9076-done <at> debbugs.gnu.org (full text, mbox):
Thanks for reporting that. I merged that into some
other timeout stuff I was already looking at, and pushed
the following set of patches. They also should fix Bug#9076,
so I'm CC'ing that and marking both bugs "done".
I have a couple of other ideas for improving "timeout" but
figure they could use some review so I'll post them as
separate bug reports.
From bbd4c9edfa4bd5650106261b7d9b1dd434d91581 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 15 Jul 2011 17:38:32 -0700
Subject: [PATCH 1/6] dd: port to NonStop (Bug#9076)
* src/dd.c (SA_RESETHAND): Define to 0 if not defined.
---
src/dd.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/src/dd.c b/src/dd.c
index 3e75412..0824f6c 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -55,6 +55,11 @@
# endif
#endif
+/* NonStop circa 2011 lacks SA_RESETHAND; see Bug#9076. */
+#ifndef SA_RESETHAND
+# define SA_RESETHAND 0
+#endif
+
#ifndef SIGINFO
# define SIGINFO SIGUSR1
#endif
--
1.7.4.4
From 995299ff155bef70a3b153dc8cef11ed9b1d8904 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 15 Jul 2011 17:39:28 -0700
Subject: [PATCH 2/6] ls: port to NonStop (Bug#9076)
* src/ls.c (SA_RESTART): Define to 0 if not defined.
---
src/ls.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/src/ls.c b/src/ls.c
index c604e14..680a7c3 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -74,6 +74,14 @@
# endif
#endif
+/* NonStop circa 2011 lacks both SA_RESTART and siginterrupt, so don't
+ restart syscalls after a signal handler fires. This may cause
+ colors to get messed up on the screen if 'ls' is interrupted, but
+ that's the best we can do on such a platform. */
+#ifndef SA_RESTART
+# define SA_RESTART 0
+#endif
+
#include "system.h"
#include <fnmatch.h>
--
1.7.4.4
From 638e670d76b3bf573f6a9930b376811b5663881a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 15 Jul 2011 17:48:38 -0700
Subject: [PATCH 3/6] timeout: port to NonStop (Bug#9077)
* src/timeout.c (SA_RESTART): Define to 0 if not defined.
(main): Don't assume signal handling uses SA_RESTART.
---
src/timeout.c | 21 ++++++++++++---------
1 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/timeout.c b/src/timeout.c
index ef660a7..895d720 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -64,6 +64,11 @@
# include <sys/resource.h>
#endif
+/* NonStop circa 2011 lacks both SA_RESTART and siginterrupt. */
+#ifndef SA_RESTART
+# define SA_RESTART 0
+#endif
+
#define PROGRAM_NAME "timeout"
#define AUTHORS proper_name_utf8 ("Padraig Brady", "P\303\241draig Brady")
@@ -256,7 +261,8 @@ install_signal_handlers (int sigterm)
struct sigaction sa;
sigemptyset (&sa.sa_mask); /* Allow concurrent calls to handler */
sa.sa_handler = cleanup;
- sa.sa_flags = SA_RESTART; /* restart syscalls (like wait() below) */
+ sa.sa_flags = SA_RESTART; /* Restart syscalls if possible, as that's
+ more likely to work cleanly. */
sigaction (SIGALRM, &sa, NULL); /* our timeout. */
sigaction (SIGINT, &sa, NULL); /* Ctrl-C at terminal for example. */
@@ -354,18 +360,15 @@ main (int argc, char **argv)
}
else
{
+ pid_t wait_result;
int status;
alarm (timeout);
- /* We're just waiting for a single process here, so wait() suffices.
- Note the signal() calls above on GNU/Linux and BSD at least,
- essentially call the lower level sigaction() with the SA_RESTART flag
- set, which ensures the following wait call will only return if the
- child exits, not on this process receiving a signal. Also we're not
- passing WUNTRACED | WCONTINUED to a waitpid() call and so will not get
- indication that the child has stopped or continued. */
- if (wait (&status) == -1)
+ while ((wait_result = wait (&status)) < 0 && errno == EINTR)
+ continue;
+
+ if (wait_result < 0)
{
/* shouldn't happen. */
error (0, errno, _("error waiting for command"));
--
1.7.4.4
From 1f97794a3acfad6d169c60adec5a7ce916baa8f9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Sat, 16 Jul 2011 05:57:19 -0700
Subject: [PATCH 4/6] * src/timeout.c (main): Use waitpid, not wait
(Bug#9098).
Reported by Andreas Schwab.
* src/timeout.c (SA_RESTART): Define to 0 if not defined.
---
src/timeout.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/timeout.c b/src/timeout.c
index 895d720..2d6dad8 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -365,7 +365,8 @@ main (int argc, char **argv)
alarm (timeout);
- while ((wait_result = wait (&status)) < 0 && errno == EINTR)
+ while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0
+ && errno == EINTR)
continue;
if (wait_result < 0)
--
1.7.4.4
From 22e9697c795148548410673260595745d4e8d764 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Sat, 16 Jul 2011 06:03:47 -0700
Subject: [PATCH 5/6] Fix capiTalization in comments.
---
src/timeout.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/timeout.c b/src/timeout.c
index 2d6dad8..33bb8e4 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -35,7 +35,7 @@
This can be seen with `timeout 10 dd&` for example.
However if one brings this group to the foreground with the `fg`
command before the timer expires, the command will remain
- in the sTop state as the shell doesn't send a SIGCONT
+ in the stop state as the shell doesn't send a SIGCONT
because the timeout process (group leader) is already running.
To get the command running again one can Ctrl-Z, and do fg again.
Note one can Ctrl-C the whole job when in this state.
@@ -333,9 +333,9 @@ main (int argc, char **argv)
/* Setup handlers before fork() so that we
handle any signals caused by child, without races. */
install_signal_handlers (term_signal);
- signal (SIGTTIN, SIG_IGN); /* don't sTop if background child needs tty. */
- signal (SIGTTOU, SIG_IGN); /* don't sTop if background child needs tty. */
- signal (SIGCHLD, SIG_DFL); /* Don't inherit CHLD handling from parent. */
+ signal (SIGTTIN, SIG_IGN); /* Don't stop if background child needs tty. */
+ signal (SIGTTOU, SIG_IGN); /* Don't stop if background child needs tty. */
+ signal (SIGCHLD, SIG_DFL); /* Don't inherit CHLD handling from parent. */
monitored_pid = fork ();
if (monitored_pid == -1)
--
1.7.4.4
From f9bcef4765dc7c3ecb8d7c58ae410087195e6fb1 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Sat, 16 Jul 2011 12:07:46 -0700
Subject: [PATCH 6/6] timeout: treat seconds counts like 'sleep' does
Treat fractions as a request to round up to the next representable
value, and treat out-of-range values as maximal ones. This is
consistent with how "sleep" works. And this way, "timeout
999999999999999999d FOO" and "timeout 4.5 foo" are more likely to
do what the user wants.
* src/timeout.c: Include c-strtod.h and xstrtod.h, not xstrtol.h.
(apply_time_suffix): Change it to the way sleep.c's time_suffix
does things. Maybe this function (identical in both programs,
other than its name) should be moved to a library?
(parse_duration): Return a maximal value on overflow. Return
unsigned int, not unsigned long. Allow fractions, which round
up to the next integer value.
* tests/misc/timeout-parameters: Adjust tests to match new behavior.
Add a very large number.
---
src/timeout.c | 60 ++++++++++++++++++++++++-----------------
tests/misc/timeout-parameters | 16 +++++++++-
2 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/src/timeout.c b/src/timeout.c
index 33bb8e4..ccb4f85 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -52,7 +52,8 @@
#include <sys/wait.h>
#include "system.h"
-#include "xstrtol.h"
+#include "c-strtod.h"
+#include "xstrtod.h"
#include "sig2str.h"
#include "operand2sig.h"
#include "error.h"
@@ -196,54 +197,51 @@ use the KILL (9) signal, since this signal cannot be caught.\n"), stdout);
exit (status);
}
-/* Given a long integer value *X, and a suffix character, SUFFIX_CHAR,
+/* Given a floating point value *X, and a suffix character, SUFFIX_CHAR,
scale *X by the multiplier implied by SUFFIX_CHAR. SUFFIX_CHAR may
be the NUL byte or `s' to denote seconds, `m' for minutes, `h' for
hours, or `d' for days. If SUFFIX_CHAR is invalid, don't modify *X
- and return false. If *X would overflow an integer, don't modify *X
- and return false. Otherwise return true. */
+ and return false. Otherwise return true. */
static bool
-apply_time_suffix (unsigned long *x, char suffix_char)
+apply_time_suffix (double *x, char suffix_char)
{
- unsigned int multiplier = 1;
+ int multiplier;
switch (suffix_char)
{
case 0:
case 's':
- return true;
- case 'd':
- multiplier *= 24;
- case 'h':
- multiplier *= 60;
+ multiplier = 1;
+ break;
case 'm':
- if (multiplier > UINT_MAX / 60) /* 16 bit overflow */
- return false;
- multiplier *= 60;
+ multiplier = 60;
+ break;
+ case 'h':
+ multiplier = 60 * 60;
+ break;
+ case 'd':
+ multiplier = 60 * 60 * 24;
break;
default:
return false;
}
- if (*x > UINT_MAX / multiplier)
- return false;
-
*x *= multiplier;
return true;
}
-static unsigned long
+static unsigned int
parse_duration (const char* str)
{
- unsigned long duration;
- char *ep;
+ double duration;
+ const char *ep;
- if (xstrtoul (str, &ep, 10, &duration, NULL)
- /* Invalid interval. Note 0 disables timeout */
- || (duration > UINT_MAX)
- /* Extra chars after the number and an optional s,m,h,d char. */
+ if (!xstrtod (str, &ep, &duration, c_strtod)
+ /* Nonnegative interval. */
+ || ! (0 <= duration)
+ /* No extra chars after the number and an optional s,m,h,d char. */
|| (*ep && *(ep + 1))
/* Check any suffix char and update timeout based on the suffix. */
|| !apply_time_suffix (&duration, *ep))
@@ -252,7 +250,19 @@ parse_duration (const char* str)
usage (EXIT_CANCELED);
}
- return duration;
+ /* Return the requested duration, rounded up to the next representable value.
+ Treat out-of-range values as if they were maximal,
+ as that's more useful in practice than reporting an error.
+
+ FIXME: Use dtotimespec + setitimer if setitimer is available,
+ as that has higher resolution. */
+ if (UINT_MAX <= duration)
+ return UINT_MAX;
+ else
+ {
+ unsigned int duration_floor = duration;
+ return duration_floor + (duration_floor < duration);
+ }
}
static void
diff --git a/tests/misc/timeout-parameters b/tests/misc/timeout-parameters
index 6e9152b..56804a8 100755
--- a/tests/misc/timeout-parameters
+++ b/tests/misc/timeout-parameters
@@ -37,11 +37,23 @@ test $? = 125 || fail=1
# timeout overflow
timeout $UINT_OFLOW sleep 0
-test $? = 125 || fail=1
+test $? = 0 || fail=1
# timeout overflow
timeout $(expr $UINT_MAX / 86400 + 1)d sleep 0
-test $? = 125 || fail=1
+test $? = 0 || fail=1
+
+# timeout overflow
+timeout 999999999999999999999999999999999999999999999999999999999999d sleep 0
+test $? = 0 || fail=1
+
+# floating point notation
+timeout 2.34 sleep 0
+test $? = 0 || fail=1
+
+# floating point notation
+timeout 2.34e+5d sleep 0
+test $? = 0 || fail=1
# invalid signal spec
timeout --signal=invalid 1 sleep 0
--
1.7.4.4
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Sun, 17 Jul 2011 09:45:03 GMT)
Full text and
rfc822 format available.
Message #46 received at 9076 <at> debbugs.gnu.org (full text, mbox):
Paul Eggert wrote:
...
> Subject: [PATCH 4/6] * src/timeout.c (main): Use waitpid, not wait
> (Bug#9098).
>
> Reported by Andreas Schwab.
>
> * src/timeout.c (SA_RESTART): Define to 0 if not defined.
> ---
> src/timeout.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/src/timeout.c b/src/timeout.c
> index 895d720..2d6dad8 100644
> --- a/src/timeout.c
> +++ b/src/timeout.c
> @@ -365,7 +365,8 @@ main (int argc, char **argv)
>
> alarm (timeout);
>
> - while ((wait_result = wait (&status)) < 0 && errno == EINTR)
> + while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0
> + && errno == EINTR)
Thanks for doing that.
Since this fixes a bug, would you please add a note to NEWS and
add a test case to exercise the bug?
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Sun, 17 Jul 2011 19:55:02 GMT)
Full text and
rfc822 format available.
Message #49 received at 9076 <at> debbugs.gnu.org (full text, mbox):
On 07/17/11 02:44, Jim Meyering wrote:
> would you please add a note to NEWS and
> add a test case to exercise the bug?
Sure, done as follows:
From fb266dea471f442ecd09813d3e009242135aa1ab Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Sun, 17 Jul 2011 12:47:22 -0700
Subject: [PATCH 1/2] timeout: add regression test (Bug#9098)
* tests/misc/timeout: Check that 'timeout' is not confused when
starting off with a child.
---
tests/misc/timeout | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/tests/misc/timeout b/tests/misc/timeout
index 7506e7c..04465e1 100755
--- a/tests/misc/timeout
+++ b/tests/misc/timeout
@@ -51,4 +51,9 @@ test $? = 124 && fail=1
exec timeout 10 true
) || fail=1
+# Don't be confused when starting off with a child (Bug#9098).
+out=$(sleep 1 & exec timeout 2 sh -c 'sleep 3; echo foo')
+status=$?
+test "$out" = "" && test $status = 124 || fail=1
+
Exit $fail
--
1.7.4.4
From 5cda9009fc68485b1bd04a845e68c103170f6be8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Sun, 17 Jul 2011 12:51:56 -0700
Subject: [PATCH 2/2] * NEWS: Mention fix for Bug#9098.
---
NEWS | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/NEWS b/NEWS
index c382200..61e6e63 100644
--- a/NEWS
+++ b/NEWS
@@ -15,7 +15,8 @@ GNU coreutils NEWS -*- outline -*-
[bug introduced in coreutils-8.8]
timeout now sends signals to commands that create their own process group.
- [bug introduced in coreutils-7.0]
+ timeout is no longer confused when starting off with a child process.
+ [bugs introduced in coreutils-7.0]
** Changes in behavior
--
1.7.4.4
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Tue, 19 Jul 2011 16:18:02 GMT)
Full text and
rfc822 format available.
Message #52 received at 9076 <at> debbugs.gnu.org (full text, mbox):
> -----Original Message-----
> From: Paul Eggert [mailto:eggert <at> cs.ucla.edu]
> Sent: Saturday, July 16, 2011 3:12 AM
Sorry for the delay, got distracted with $DAYJOB
> To: Pádraig Brady
> Cc: Joachim Schmitz; 9076 <at> debbugs.gnu.org
> Subject: Re: bug#9076: coreutils-8.12 uses SA_RESETHAND and SA_RESTART
> unconditionally
>
> Here are three proposed patches to coreutils to address the SA_RESETHAND
> and SA_RESTART issues. I hope that if you combine them with the patch I
> already installed:
>
> http://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=837e1f55196f826b92
> d660808f594fde36651655
>
> these should fix all the issues mentioned here.
> Joachim, could you please try these? Thanks.
Yes they work for me, thanks!
Bye, Jojo
Message #53 received at 9076-done <at> debbugs.gnu.org (full text, mbox):
Good to hear it's working for you.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Wed, 20 Jul 2011 10:14:02 GMT)
Full text and
rfc822 format available.
Message #56 received at 9076 <at> debbugs.gnu.org (full text, mbox):
On 16/07/11 01:51, Paul Eggert wrote:
> On 07/15/11 03:28, Pádraig Brady wrote:
>> What I was getting was that it's probably better to leave
>> the following to the app too:
>>
>> #ifndef SA_RESETHAND
>> # define SA_RESETHAND 0
>> /* Now the app writer knows they need to handle this case */
>> #endif
>
> Yes, you're probably right. I'll send some patches along
> those lines shortly (but not to bug-gnulib since that's not
> relevant to these coreutils-specific patches).
I notice that make syntax-check is now failing in coreutils.
src/dd.c:# define SA_RESETHAND 0
src/ls.c:# define SA_RESTART 0
src/timeout.c:# define SA_RESTART 0
maint.mk: define the above via some gnulib .h file
make: *** [sc_prohibit_always-defined_macros] Error 1
This is because those are defined in signal.h.in which were
added as part of cfb3906f to add sigaction support to mingw.
Eric do you think we should define those in gnulib?
My thinking was that the onus should be on the apps
to #define these, as otherwise it's too easy for
compiles to just proceed on a platform while ignoring
requested operations.
cheers,
Pádraig.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9076; Package
coreutils.
(Wed, 27 Jul 2011 16:50:02 GMT)
Full text and
rfc822 format available.
Message #59 received at 9076 <at> debbugs.gnu.org (full text, mbox):
Pádraig Brady wrote:
> On 16/07/11 01:51, Paul Eggert wrote:
>> On 07/15/11 03:28, Pádraig Brady wrote:
>>> What I was getting was that it's probably better to leave
>>> the following to the app too:
>>>
>>> #ifndef SA_RESETHAND
>>> # define SA_RESETHAND 0
>>> /* Now the app writer knows they need to handle this case */
>>> #endif
>>
>> Yes, you're probably right. I'll send some patches along
>> those lines shortly (but not to bug-gnulib since that's not
>> relevant to these coreutils-specific patches).
>
> I notice that make syntax-check is now failing in coreutils.
>
> src/dd.c:# define SA_RESETHAND 0
> src/ls.c:# define SA_RESTART 0
> src/timeout.c:# define SA_RESTART 0
> maint.mk: define the above via some gnulib .h file
> make: *** [sc_prohibit_always-defined_macros] Error 1
>
> This is because those are defined in signal.h.in which were
> added as part of cfb3906f to add sigaction support to mingw.
>
> Eric do you think we should define those in gnulib?
> My thinking was that the onus should be on the apps
> to #define these, as otherwise it's too easy for
> compiles to just proceed on a platform while ignoring
> requested operations.
Either way it's not a big deal, but I want coreutils' "make syntax-check"
test to pass once again, so I'll probably push this to gnulib later today
and update coreutils to use it:
diff --git a/top/maint.mk b/top/maint.mk
index 9357728..6c1bf44 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -753,6 +753,7 @@ gl_extract_significant_defines_ = \
/^\# *define ([^_ (][^ (]*)(\s*\(|\s+\w+)/\
&& $$2 !~ /(?:rpl_|_used_without_)/\
&& $$1 !~ /^(?:NSIG)$$/\
+ && $$1 !~ /^(?:SA_RESETHAND|SA_RESTART)$$/\
and print $$1
# Create a list of regular expressions matching the names
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 25 Aug 2011 11:24:04 GMT)
Full text and
rfc822 format available.
This bug report was last modified 13 years and 359 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.