Package: coreutils;
Reported by: Arkadiusz Miśkiewicz <arekm <at> maven.pl>
Date: Wed, 7 Dec 2011 17:47:02 UTC
Severity: normal
Found in version 8.14
Done: Jim Meyering <jim <at> meyering.net>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Jim Meyering <jim <at> meyering.net> To: Arkadiusz Miśkiewicz <arekm <at> maven.pl> Cc: 10243 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com> Subject: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and no network fs in use) Date: Wed, 07 Dec 2011 23:04:40 +0100
Arkadiusz Miśkiewicz wrote:
> On Wednesday 07 of December 2011, Pádraig Brady wrote:
>> On 12/07/2011 08:16 PM, Arkadiusz Miśkiewicz wrote:
>> > On Wednesday 07 of December 2011, Pádraig Brady wrote:
>> >> On 12/07/2011 05:56 PM, Jim Meyering wrote:
>> >>> Arkadiusz Miśkiewicz wrote:
>> >>>> When doing "ls --color=tty" or "ls --color=auto" on directory then ls
>> >>>> ignores (?) ctrl+c or ctrl+z signals. Basically I'm unable to
>> >>>> interrupt ls in such case. Easily reproducible with bigger
>> >>>> directories.
>> >>>
>> >>> Thanks for the report.
>> >>>
>> >>> I reproduced it starting in an empty directory like this:
>> >>> seq 100000|xargs touch
>> >>> env ls --color -1
>> >>>
>> >>> and tried to interrupt that.
>> >>> Failed to interrupt every time.
>> >>>
>> >>> Here's one way to fix it:
>> >>>
>> >>> diff --git a/src/ls.c b/src/ls.c
>> >>> index 8be9b6a..58bb196 100644
>> >>> --- a/src/ls.c
>> >>> +++ b/src/ls.c
>> >>> @@ -4060,9 +4060,9 @@ print_name_with_quoting (const struct fileinfo
>> >>> *f,
>> >>>
>> >>> if (stack)
>> >>>
>> >>> PUSH_CURRENT_DIRED_POS (stack);
>> >>>
>> >>> + process_signals ();
>> >>>
>> >>> if (used_color_this_time)
>> >>>
>> >>> {
>> >>>
>> >>> - process_signals ();
>> >>>
>> >>> prep_non_filename_text ();
>> >>> if (start_col / line_length != (start_col + width - 1) /
>> >>> line_length)
>> >>>
>> >>> put_indicator (&color_indicator[C_CLR_TO_EOL]);
>> >>
>> >> Looks like a good fix.
>> >> It works here and had negligible impact on performance.
>> >
>> > That part works for me too. Unfortunately more changes is needed since
>> > before printing happens it it still not possible to interrupt ls (and
>> > for huge dirs it can take a while).
>> >
>> > Moving code that enables special signal handling just before actuall
>> > printing starts?
>>
>> Probably, as long as there are no long blocking calls when
>> processing large dirs, after we've starting printing.
> Don't know why that should matter.
>
> IMO before printing happens there should be no difference in signal handling
> behaviour between ls --color and ls (since the whole signal handling is just
> to "protect" printing from what I understand).
>
> I was thinking about something like:
> - do ususal things
> - setup special signal handling
> - print + process_signals () at print_name_with_quoting
> - back to original signal handling
> - do the rest of things
>
> so most of the time there won't be any special signal handling (== will be the
> same as ls without --color).
>
>> Do you get the delays with -U too?
>
> Yes, too.
>
> ls doesn't call process_signals() at all before printing starts in --color
> mode thus making ls uninterruptible in that period.
Thanks for the feedback.
Here's a more complete patch, but I haven't re-reviewed it yet,
so caveat emptor.
It may write a test, too... but it will have to be racy,
so maybe not.
Oh, and I've just realized this needs a NEWS entry, too.
From 54d7dafb0b10daa54d9beb8e1020c2e1bfbe0370 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Wed, 7 Dec 2011 23:00:42 +0100
Subject: [PATCH] ls: do not inhibit interrupts when listing large directories
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Starting with commit adc30a83, ls inhibited interrupts to avoid
corrupting the state of the terminal, but for very large directories,
that inhibition renders ls uninterruptible for too long, including
a potentially long period even before any output is generated.
Act on interrupts as much as possible.
* src/ls.c (Sig, nsigs): New globals, moved here from "main".
Global Sig renamed from sig, to avoid shadowing "int sig".
Now used in install_signal_handlers and in main.
(install_signal_handlers): New function, extracted from...
(main): ...here.
The top-level function for printing file names is print_current_files.
There are two execution paths by which to actually print a possibly-
colorized name from this function: via print_file_name_and_frills
or via the "case long_format". We cannot simply install signal
handlers at the top, because both print_many_per_line and
print_horizontal end up calling calculate_columns, which would
cause a time-consuming and uninterruptible prelude to printing (when
there are many file names and many columns). Thus, we opt to install
the signal handlers in two places: that "case long_format", and upon
the first call to print_file_name_and_frills.
Reported by Arkadiusz Miśkiewicz in http://bugs.gnu.org/10243
---
THANKS.in | 1 +
src/ls.c | 165 +++++++++++++++++++++++++++++++++---------------------------
2 files changed, 92 insertions(+), 74 deletions(-)
diff --git a/THANKS.in b/THANKS.in
index 5ecc29e..afed5d4 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -59,6 +59,7 @@ Anthony Thyssen anthony <at> griffith.edu.au
Antonio Rendas ajrendas <at> yahoo.com
Ariel Faigon ariel <at> cthulhu.engr.sgi.com
Arjan Opmeer arjan.opmeer <at> gmail.com
+Arkadiusz Miśkiewicz arekm <at> maven.pl
Arne Henrik Juul arnej <at> imf.unit.no
Arnold Robbins arnold <at> skeeve.com
Arthur Pool pool <at> commerce.uq.edu.au
diff --git a/src/ls.c b/src/ls.c
index 8be9b6a..69e40a9 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -155,6 +155,36 @@
# define st_author st_uid
#endif
+/* The signals that are trapped, and the number of such signals. */
+static int const Sig[] =
+ {
+ /* This one is handled specially. */
+ SIGTSTP,
+
+ /* The usual suspects. */
+ SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
+#ifdef SIGPOLL
+ SIGPOLL,
+#endif
+#ifdef SIGPROF
+ SIGPROF,
+#endif
+#ifdef SIGVTALRM
+ SIGVTALRM,
+#endif
+#ifdef SIGXCPU
+ SIGXCPU,
+#endif
+#ifdef SIGXFSZ
+ SIGXFSZ,
+#endif
+ };
+enum { nsigs = ARRAY_CARDINALITY (Sig) };
+
+#if ! SA_NOCLDSTOP
+static bool caught_sig[nsigs];
+#endif
+
enum filetype
{
unknown,
@@ -1229,6 +1259,53 @@ process_signals (void)
}
}
+static void
+install_signal_handlers (void)
+{
+ if (print_with_color)
+ {
+ /* If the standard output is a controlling terminal, watch out
+ for signals, so that the colors can be restored to the
+ default state if "ls" is suspended or interrupted. */
+
+ if (0 <= tcgetpgrp (STDOUT_FILENO))
+ {
+ int j;
+#if SA_NOCLDSTOP
+ struct sigaction act;
+
+ sigemptyset (&caught_signals);
+ for (j = 0; j < nsigs; j++)
+ {
+ sigaction (Sig[j], NULL, &act);
+ if (act.sa_handler != SIG_IGN)
+ sigaddset (&caught_signals, Sig[j]);
+ }
+
+ act.sa_mask = caught_signals;
+ act.sa_flags = SA_RESTART;
+
+ for (j = 0; j < nsigs; j++)
+ if (sigismember (&caught_signals, Sig[j]))
+ {
+ act.sa_handler = Sig[j] == SIGTSTP ? stophandler : sighandler;
+ sigaction (Sig[j], &act, NULL);
+ }
+#else
+ for (j = 0; j < nsigs; j++)
+ {
+ caught_sig[j] = (signal (Sig[j], SIG_IGN) != SIG_IGN);
+ if (caught_sig[j])
+ {
+ signal (Sig[j], Sig[j] == SIGTSTP ? stophandler : sighandler);
+ siginterrupt (Sig[j], 0);
+ }
+ }
+#endif
+ }
+ }
+}
+
int
main (int argc, char **argv)
{
@@ -1236,36 +1313,6 @@ main (int argc, char **argv)
struct pending *thispend;
int n_files;
- /* The signals that are trapped, and the number of such signals. */
- static int const sig[] =
- {
- /* This one is handled specially. */
- SIGTSTP,
-
- /* The usual suspects. */
- SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
-#ifdef SIGPOLL
- SIGPOLL,
-#endif
-#ifdef SIGPROF
- SIGPROF,
-#endif
-#ifdef SIGVTALRM
- SIGVTALRM,
-#endif
-#ifdef SIGXCPU
- SIGXCPU,
-#endif
-#ifdef SIGXFSZ
- SIGXFSZ,
-#endif
- };
- enum { nsigs = ARRAY_CARDINALITY (sig) };
-
-#if ! SA_NOCLDSTOP
- bool caught_sig[nsigs];
-#endif
-
initialize_main (&argc, &argv);
set_program_name (argv[0]);
setlocale (LC_ALL, "");
@@ -1299,46 +1346,6 @@ main (int argc, char **argv)
|| (is_colored (C_EXEC) && color_symlink_as_referent)
|| (is_colored (C_MISSING) && format == long_format))
check_symlink_color = true;
-
- /* If the standard output is a controlling terminal, watch out
- for signals, so that the colors can be restored to the
- default state if "ls" is suspended or interrupted. */
-
- if (0 <= tcgetpgrp (STDOUT_FILENO))
- {
- int j;
-#if SA_NOCLDSTOP
- struct sigaction act;
-
- sigemptyset (&caught_signals);
- for (j = 0; j < nsigs; j++)
- {
- sigaction (sig[j], NULL, &act);
- if (act.sa_handler != SIG_IGN)
- sigaddset (&caught_signals, sig[j]);
- }
-
- act.sa_mask = caught_signals;
- act.sa_flags = SA_RESTART;
-
- for (j = 0; j < nsigs; j++)
- if (sigismember (&caught_signals, sig[j]))
- {
- act.sa_handler = sig[j] == SIGTSTP ? stophandler : sighandler;
- sigaction (sig[j], &act, NULL);
- }
-#else
- for (j = 0; j < nsigs; j++)
- {
- caught_sig[j] = (signal (sig[j], SIG_IGN) != SIG_IGN);
- if (caught_sig[j])
- {
- signal (sig[j], sig[j] == SIGTSTP ? stophandler : sighandler);
- siginterrupt (sig[j], 0);
- }
- }
-#endif
- }
}
if (dereference == DEREF_UNDEFINED)
@@ -1468,12 +1475,12 @@ main (int argc, char **argv)
/* Restore the default signal handling. */
#if SA_NOCLDSTOP
for (j = 0; j < nsigs; j++)
- if (sigismember (&caught_signals, sig[j]))
- signal (sig[j], SIG_DFL);
+ if (sigismember (&caught_signals, Sig[j]))
+ signal (Sig[j], SIG_DFL);
#else
for (j = 0; j < nsigs; j++)
if (caught_sig[j])
- signal (sig[j], SIG_DFL);
+ signal (Sig[j], SIG_DFL);
#endif
/* Act on any signals that arrived before the default was restored.
@@ -3483,6 +3490,7 @@ print_current_files (void)
break;
case long_format:
+ install_signal_handlers ();
for (i = 0; i < cwd_n_used; i++)
{
set_normal_color ();
@@ -4060,9 +4068,9 @@ print_name_with_quoting (const struct fileinfo *f,
if (stack)
PUSH_CURRENT_DIRED_POS (stack);
+ process_signals ();
if (used_color_this_time)
{
- process_signals ();
prep_non_filename_text ();
if (start_col / line_length != (start_col + width - 1) / line_length)
put_indicator (&color_indicator[C_CLR_TO_EOL]);
@@ -4092,6 +4100,15 @@ static size_t
print_file_name_and_frills (const struct fileinfo *f, size_t start_col)
{
char buf[MAX (LONGEST_HUMAN_READABLE + 1, INT_BUFSIZE_BOUND (uintmax_t))];
+ static bool first = true;
+ if (first)
+ {
+ /* handle interrupts so that ls cannot be made to output an
+ incomplete multi-byte sequence or a color-change escape
+ sequence that could leave the terminal messed up. */
+ install_signal_handlers ();
+ first = false;
+ }
set_normal_color ();
--
1.7.8
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.