sup talk - Feb 2008 - [PATCH] shell commands are now run in a child process

When shelling out, the external command is now run in a child process.
Before the external command is run, all (I hope) of sup''s open files
are
closed first.

This should fix the ferret "Permission denied" errors on Windows.
---
 lib/sup/buffer.rb |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/lib/sup/buffer.rb b/lib/sup/buffer.rb
index 4374fa8..7768011 100644
--- a/lib/sup/buffer.rb
+++ b/lib/sup/buffer.rb
@@ -696,11 +696,36 @@ EOS
     draw_screen :refresh => true
   end
 
+  ## There is no good way that I can find the get the maximum fd allowed.
+  ## On the POSIX systems I have played with, 500 seems to be the maximum. On
Windows,
+  ## 2048 is the documented maximum (for the C Runtime Library).
+  MAX_FD = 2048
+
   def shell_out command
     @shelled = true
     Ncurses.sync do
       Ncurses.endwin
-      system command
+
+      child_pid = fork
+      if child_pid == nil
+        ## child process
+
+        start_fd = 1 + [STDIN.fileno, STDOUT.fileno, STDERR.fileno].max #
don''t close these
+	start_fd.upto(MAX_FD) do |fd|
+          begin
+            IO.for_fd(fd).close
+          rescue Errno::EBADF
+            ## fd is not open: ignore and move on
+          end
+        end
+
+        exec(command)
+        ## never gets here
+      else
+        ## sup process
+        Process.waitpid(child_pid, Process::WUNTRACED) # catch an already dead
child
+      end
+
       Ncurses.refresh
       Ncurses.curs_set 0
     end
-- 
1.5.3.8

Reformatted excerpts from Christopher Warrington''s message of
2008-02-03:
> When shelling out, the external command is now run in a child process.
> Before the external command is run, all (I hope) of sup''s open
files
> are closed first.
This seems crazy. Do mbox sources even continue to function properly
once this has happened? It seems like they''d crash with their file
handles closed from under them.

If the issue is that Windows has bad behavior when Sup polls while
you''re editing a message, another option might be to turn off polling
when Sup is shelled out.

Something like (although this doesn''t prevent you from starting an
editor while a background poll is in progress):

diff --git a/lib/sup/buffer.rb b/lib/sup/buffer.rb
index d40a626..96f676b 100644
--- a/lib/sup/buffer.rb
+++ b/lib/sup/buffer.rb
@@ -137,6 +137,7 @@ class BufferManager
   include Singleton
 
   attr_reader :focus_buf
+  bool_reader :shelled
 
   ## we have to define the key used to continue in-buffer search here, because
   ## it has special semantics that BufferManager deals with---current searches
diff --git a/lib/sup/poll.rb b/lib/sup/poll.rb
index 2dd9150..a67e801 100644
--- a/lib/sup/poll.rb
+++ b/lib/sup/poll.rb
@@ -45,7 +45,8 @@ EOS
   end
 
   def poll
-    return if @polling
+    return if @polling || BufferManager.shelled?
+
     @polling = true
     HookManager.run "before-poll"
 


-- 
William <wmorgan-sup at masanjin.net>

Excerpts from William Morgan''s message of Tue Feb 05 11:34:12 -0600
2008:
>> When shelling out, the external command is now run in a child 
>> process. Before the external command is run, all (I hope) of
sup''s
>> open files are closed first.
> This seems crazy. Do mbox sources even continue to function properly 
> once this has happened? It seems like they''d crash with their file
> handles closed from under them.
No, it''s not elegant at all. mbox sources do still work, though. I only
close the files in the child process. They remain open in sup''s main 
process.

Setting the file descriptors to FD_CLOEXEC would be a cleaner solution, 
but I don''t think that this works on Cygwin (or is accessible via
Ruby).
> If the issue is that Windows has bad behavior when Sup polls while
> you''re editing a message, another option might be to turn off
polling
> when Sup is shelled out.
That''s a thought. If I get some time, I may be able to dig into this.

-- 
Christopher Warrington <chrisw at rice.edu>

Reformatted excerpts from Christopher Warrington''s message of
2008-02-05:
> No, it''s not elegant at all. mbox sources do still work, though. I
> only close the files in the child process. They remain open in
sup''s
> main process.
Oh, I see. Sorry, I misread the patch.

It would be good to understand this a bit more. Why are the Ferret
errors caused by open filehandles? And Windows supports fork now? I
thought that didn''t work.

-- 
William <wmorgan-sup at masanjin.net>

Excerpts from William Morgan''s message of Tue Feb 05 14:12:23 -0600
2008:
> It would be good to understand this a bit more. Why are the Ferret
> errors caused by open filehandles?
It is an odd interplay between Cygwin and Windows.

Sometimes, when ferret updates the index, it needs to delete (or
rename, I''m not sure) one of its files. So, ferret deletes the file
and then checks that it has been deleted (in fs_exists() from
ferret-.../ext/fs_store.c:70). To do the delete, unlink() is
called. Cygwin maps this to a series of Windows kernel calls that 1)
open the file, 2) set the delete-on-close flag, and 3) close the file.

(See unline_nt() in
http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/cygwin/syscalls.cc?rev=1.469&content-type=text/x-cvsweb-markup&cvsroot=src)

If only sup is running, the file is closed and deleted. The file no
longer exists, so the open() in fs_exists fails with ENOENT. This is
to be expected.

If the editor is also running, things happen a little differently. The
editor has inherited all of sup''s file handles. When the unlink() is
called this time, the same three kernel calls are made: 1) open, 2)
set delete-on-close flag, 3) close file. However, there are still an
open handle for the file (the editor has it), so the file enters the
DELETE_PENDING state. This causes the open() call in fs_exists to fail
with EACCES, triggering the exception.

I have a Process Monitor log of what''s going on and the exception log
if you think that that would be helpful.
> And Windows supports fork now? I thought that didn''t work.
No. But Cygwin does. This still causes the file handle duplication, so
I close all the open files except STDIN, STDOUT, and STDERR. That way,
the ferret file never enters the DELETE_PENDING state.

I don''t think that this would be an issue if I were running sup
natively on Windows. The editor would NOT inherit sup''s file
handlers. However, I have not been able to get ncurses (or the Ruby
gem, I can''t remember) to compile for Windows. So, I''m stuck
in Cygwin
bizzaro-land.

-- 
Christopher Warrington <chrisw at rice.edu>

Reformatted excerpts from Christopher Warrington''s message of
2008-02-08:
> It is an odd interplay between Cygwin and Windows.
[snip lots]

Thanks for that very detailed explanation.
> So, I''m stuck in Cygwin bizzaro-land.
I think most Windows Sup users are in the same situtation, so I would
like to make this work.

The only thing that makes me uncomfortable about the patch now is the
2048 thing. But if that is, in fact, the documented maximum number of
fds ever opened by a Windows process, then heck, let''s go for it.

I''m going to move this code into a separate module that''s only
called
when run under Cygwin, then commit.

-- 
William <wmorgan-sup at masanjin.net>