GNU bug report logs - #10243
8.14: ls --color is uninterruptible with ctrl+c (and no network fs in use)

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 10243 in the body.
You can then email your comments to 10243 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Arkadiusz Miśkiewicz <arekm <at> maven.pl>
To: bug-coreutils <at> gnu.org
Subject: 8.14: ls --color is uninterruptible with ctrl+c (and no network fs in
	use)
Date: Wed, 7 Dec 2011 11:07:30 +0100

Hello,

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.

# ls --color=tty
^C^C^C^C^C^C^C^C^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z
(no reaction)

There is no such problem if ls is being straced (strace -f -F -s 200 ls) at 
that time or if pipe is used ("ls --color=tty | less") - in such cases ctrl+c 
works immediately. If --color is not used then there is no problem, too.

First I thought that this is filesystem issue but it is not. One of xfs 
filesystem developers was also able to reproduce the problem and confirm that 
most likely ls is doing something incorrect.

# LC_ALL=C ls --version
ls (GNU coreutils) 8.14
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Richard M. Stallman and David MacKenzie.

Linux 3.0.9, glibc 2.14 here.
-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Message #8 received at 10243 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Arkadiusz Miśkiewicz <arekm <at> maven.pl>
Cc: 10243 <at> debbugs.gnu.org
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Wed, 07 Dec 2011 18:56:05 +0100

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.
>
> # ls --color=tty
> ^C^C^C^C^C^C^C^C^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z^Z
> (no reaction)
>
> There is no such problem if ls is being straced (strace -f -F -s 200 ls) at
> that time or if pipe is used ("ls --color=tty | less") - in such cases ctrl+c
> works immediately. If --color is not used then there is no problem, too.
>
> First I thought that this is filesystem issue but it is not. One of xfs
> filesystem developers was also able to reproduce the problem and confirm that
> most likely ls is doing something incorrect.
>
> # LC_ALL=C ls --version
> ls (GNU coreutils) 8.14
> Copyright (C) 2011 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
>
> Written by Richard M. Stallman and David MacKenzie.
>
> Linux 3.0.9, glibc 2.14 here.

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]);

Message #11 received at 10243 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 10243 <at> debbugs.gnu.org,
	Arkadiusz Miśkiewicz <arekm <at> maven.pl>
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Wed, 07 Dec 2011 20:11:58 +0000

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.

cheers,
Pádraig.

Message #14 received at 10243 <at> debbugs.gnu.org (full text, mbox):

From: Arkadiusz Miśkiewicz <arekm <at> maven.pl>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 10243 <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Wed, 7 Dec 2011 21:16:28 +0100

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?

> 
> cheers,
> Pádraig.


-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Message #17 received at 10243 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Arkadiusz Miśkiewicz <arekm <at> maven.pl>
Cc: 10243 <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Wed, 07 Dec 2011 20:41:49 +0000

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.
Do you get the delays with -U too?
I guess we should test over NFS too.

cheers,
Pádraig.

Message #20 received at 10243 <at> debbugs.gnu.org (full text, mbox):

From: Arkadiusz Miśkiewicz <arekm <at> maven.pl>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 10243 <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Wed, 7 Dec 2011 21:54:50 +0100

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.

> I guess we should test over NFS too.

> cheers,
> Pádraig.


-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Message #23 received at 10243 <at> debbugs.gnu.org (full text, mbox):

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: Re: 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

Message #26 received at 10243 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 10243 <at> debbugs.gnu.org,
	Arkadiusz Miśkiewicz <arekm <at> maven.pl>
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Wed, 07 Dec 2011 22:53:43 +0000

From a cursory glance the change looks sensible.

Note a separate issue is there is a small chance that the ^C
representation output to the terminal, will mess things up:
  http://debbugs.gnu.org/9620#14
I.E. even when signals are blocked the ^C etc. is output,
which can mess up ANSI codes and multibyte characters
(I have seen such corruption).

I'm just noting it here, as it's probably overkill/problematic
to disable echoing to the terminal, and explicitly
output ^C when we process_signals() like readline does.

cheers,
Pádraig.

Message #29 received at 10243 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 10243 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>,
	Arkadiusz Miśkiewicz <arekm <at> maven.pl>
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Wed, 07 Dec 2011 16:16:49 -0700

[Message part 1 (text/plain, inline)]

On 12/07/2011 03:04 PM, Jim Meyering wrote:
>> 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.

Question - is ls processing entirely two-phase (collect all names, then
format them), or is this a repetitive loop?  Seeing as how long listings
of two directories on the command line produces differing column widths
between the two directories, I have to think it is the latter (besides,
that makes better sense from a memory management perspective - it's
cheaper to sort one directory at a time than to store the sorting of all
directories at once).  In which case, consider:

ls --color=always largedir1 largedir2

If we enable our signal handle just before outputting the sorted,
columnized, and colred largedir1 listings, only to then start our
readdir() of largedir2, we haven't solved the problem - the second
readdir() and/or calculate_columns() between the two directories will be
lengthy enough to be noticeably interrupt-starved.

> @@ -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.  */

That is, I think a one-shot static is wrong, and that you will have to
repeatedly install and uninstall the signal handlers, according to which
phase of the processing loop you are in.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #34 received at 10243-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Eric Blake <eblake <at> redhat.com>
Cc: Pádraig Brady <P <at> draigbrady.com>,
	Arkadiusz Miśkiewicz <arekm <at> maven.pl>,
	10243-done <at> debbugs.gnu.org
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Thu, 08 Dec 2011 11:05:52 +0100

Eric Blake wrote:
> On 12/07/2011 03:04 PM, Jim Meyering wrote:
...
>> Thanks for the feedback.
>> Here's a more complete patch, but I haven't re-reviewed it yet,
>> so caveat emptor.
>
> Question - is ls processing entirely two-phase (collect all names, then
> format them), or is this a repetitive loop?  Seeing as how long listings
> of two directories on the command line produces differing column widths
> between the two directories, I have to think it is the latter (besides,
> that makes better sense from a memory management perspective - it's
> cheaper to sort one directory at a time than to store the sorting of all
> directories at once).  In which case, consider:
>
> ls --color=always largedir1 largedir2

Thanks, you're right.
This is my first use of the new Co-authored-by: syntax,
so I confirmed that our updated gitlog-to-changelog does produce
the expected ChangeLog in the distribution tarball.

Here's a simpler patch.  I've tested it by running

    ls -R --color big something-distinctive big2

where big has 500K entries and big2 has 2M (on a tmpfs).
Before (using my initial patch), I could interrupt any time
before the ls' readdir loop for big2, but not during that loop,
which would run for a few seconds after ls printed its "/t/big2:"
Now, it's interruptible, as one would expect.

From aaf5b61e9999d0ece522faa4508a6565d51a8446 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Thu, 8 Dec 2011 10:49:03 +0100
Subject: [PATCH] ls: be responsive to interrupts when color-listing large
 directories
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Starting with commit adc30a83, when using --color, ls inhibited
interrupts to avoid corrupting the state of an output terminal.
However, for very large directories, that inhibition rendered ls
uninterruptible for too long, including a potentially long period
even before any output is generated.
* src/ls.c: There are two phases of processing that are time-
consuming enough that they can cause this sort of trouble:
the readdir loop and the printing loop.  The printing side of things
was nominally covered by a call to process_signals in
(print_name_with_quoting): ... but that call was mistakenly guarded
by a condition that might be false for many or even all files being
processed.  Call process_signals unconditionally.
(print_dir): Also call process_signals in the readdir loop.
* NEWS (Bug fixes): Mention it.
Reported by Arkadiusz Miśkiewicz in http://bugs.gnu.org/10243
Co-authored-by: Eric Blake <eblake <at> redhat.com>
---
 NEWS      |    3 +++
 THANKS.in |    1 +
 src/ls.c  |    7 ++++++-
 3 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index de3888d..0d4c83b 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@ GNU coreutils NEWS                                    -*- outline -*-

 ** Bug fixes

+  ls --color many-entry-directory was uninterruptible for too long
+  [bug introduced in coreutils-5.2.1]
+
   ls's -k option no longer affects how ls -l outputs file sizes.
   It now affects only the per-directory block counts written by -l,
   and the sizes written by -s.  This is for compatibility with BSD
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..0d64bab 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -2595,6 +2595,11 @@ print_dir (char const *name, char const *realname, bool command_line_arg)
         }
       else
         break;
+
+      /* When processing a very large directory, and since we've inhibited
+         interrupts, this loop would take so long that ls would be annoyingly
+         uninterruptible.  This ensures that it handles signals promptly.  */
+      process_signals ();
     }

   if (closedir (dirp) != 0)
@@ -4060,9 +4065,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]);
--
1.7.8.110.g4cb5d1

Message #35 received at 10243-done <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: Pádraig Brady <P <at> draigbrady.com>,
	Arkadiusz Miśkiewicz <arekm <at> maven.pl>,
	10243-done <at> debbugs.gnu.org
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Thu, 08 Dec 2011 06:02:38 -0700

[Message part 1 (text/plain, inline)]

On 12/08/2011 03:05 AM, Jim Meyering wrote:
> Starting with commit adc30a83, when using --color, ls inhibited
> interrupts to avoid corrupting the state of an output terminal.
> However, for very large directories, that inhibition rendered ls
> uninterruptible for too long, including a potentially long period
> even before any output is generated.
> * src/ls.c: There are two phases of processing that are time-
> consuming enough that they can cause this sort of trouble:
> the readdir loop and the printing loop.  The printing side of things
> was nominally covered by a call to process_signals in
> (print_name_with_quoting): ... but that call was mistakenly guarded
> by a condition that might be false for many or even all files being
> processed.  Call process_signals unconditionally.
> (print_dir): Also call process_signals in the readdir loop.

Aren't there actually three phases of long processing?  You covered the
readdir loop, and printing, but what about time spent in sorting?  And
if sorting is indeed a long enough processing hog, should we be calling
process_signals in the qsort() callback as a reliable frequently reached
point, or is that too dangerous?

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #36 received at 10243-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Eric Blake <eblake <at> redhat.com>
Cc: Pádraig Brady <P <at> draigbrady.com>,
	Arkadiusz Miśkiewicz <arekm <at> maven.pl>,
	10243-done <at> debbugs.gnu.org
Subject: Re: bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and
	no network fs in use)
Date: Thu, 08 Dec 2011 14:09:32 +0100

Eric Blake wrote:

> On 12/08/2011 03:05 AM, Jim Meyering wrote:
>> Starting with commit adc30a83, when using --color, ls inhibited
>> interrupts to avoid corrupting the state of an output terminal.
>> However, for very large directories, that inhibition rendered ls
>> uninterruptible for too long, including a potentially long period
>> even before any output is generated.
>> * src/ls.c: There are two phases of processing that are time-
>> consuming enough that they can cause this sort of trouble:
>> the readdir loop and the printing loop.  The printing side of things
>> was nominally covered by a call to process_signals in
>> (print_name_with_quoting): ... but that call was mistakenly guarded
>> by a condition that might be false for many or even all files being
>> processed.  Call process_signals unconditionally.
>> (print_dir): Also call process_signals in the readdir loop.
>
> Aren't there actually three phases of long processing?  You covered the

Not for today's systems.

> readdir loop, and printing, but what about time spent in sorting?  And
> if sorting is indeed a long enough processing hog, should we be calling
> process_signals in the qsort() callback as a reliable frequently reached
> point, or is that too dangerous?

It's not "long", as in noticeable with a 2-million-entry directory.
Sorting that many entries is quick enough that no change is needed.
Of course you can construct a situation in which sorting will take
too long, i.e., so little RAM that swapping is required, or so many
dir entries that sorting really does take too long, but on systems
where you don't run out of i-nodes first, a directory with that many
entries will cause you far more trouble than merely rendering ls
uninterruptible for a few seconds.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.