* [sup-talk] [PATCH] shell commands are now run in a child process
@ 2008-02-03 8:11 Christopher Warrington
2008-02-05 17:34 ` William Morgan
0 siblings, 1 reply; 9+ messages in thread
From: Christopher Warrington @ 2008-02-03 8:11 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [sup-talk] [PATCH] shell commands are now run in a child process
2008-02-03 8:11 [sup-talk] [PATCH] shell commands are now run in a child process Christopher Warrington
@ 2008-02-05 17:34 ` William Morgan
2008-02-05 19:29 ` Christopher Warrington
0 siblings, 1 reply; 9+ messages in thread
From: William Morgan @ 2008-02-05 17:34 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [sup-talk] [PATCH] shell commands are now run in a child process
2008-02-05 17:34 ` William Morgan
@ 2008-02-05 19:29 ` Christopher Warrington
2008-02-05 20:12 ` William Morgan
0 siblings, 1 reply; 9+ messages in thread
From: Christopher Warrington @ 2008-02-05 19:29 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [sup-talk] [PATCH] shell commands are now run in a child process
2008-02-05 19:29 ` Christopher Warrington
@ 2008-02-05 20:12 ` William Morgan
2008-02-08 9:40 ` Christopher Warrington
0 siblings, 1 reply; 9+ messages in thread
From: William Morgan @ 2008-02-05 20:12 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [sup-talk] [PATCH] shell commands are now run in a child process
2008-02-05 20:12 ` William Morgan
@ 2008-02-08 9:40 ` Christopher Warrington
2008-02-19 17:48 ` William Morgan
0 siblings, 1 reply; 9+ messages in thread
From: Christopher Warrington @ 2008-02-08 9:40 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [sup-talk] [PATCH] shell commands are now run in a child process
2008-02-08 9:40 ` Christopher Warrington
@ 2008-02-19 17:48 ` William Morgan
0 siblings, 0 replies; 9+ messages in thread
From: William Morgan @ 2008-02-19 17:48 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [sup-talk] [PATCH] shell commands are now run in a child process
2008-03-01 4:42 ` Christopher Warrington
@ 2008-03-01 20:37 ` William Morgan
0 siblings, 0 replies; 9+ messages in thread
From: William Morgan @ 2008-03-01 20:37 UTC (permalink / raw)
Reformatted excerpts from Christopher Warrington's message of 2008-02-29:
> I've been getting more of these when playing with offlineimap. I
> submitted a bug upstream:
> http://ferret.davebalmain.com/trac/ticket/343
And it looks like Dave is back maintaining Ferret, so this might
actually get addressed. That's great.
--
William <wmorgan-sup at masanjin.net>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [sup-talk] [PATCH] shell commands are now run in a child process
2008-02-25 17:48 William Morgan
@ 2008-03-01 4:42 ` Christopher Warrington
2008-03-01 20:37 ` William Morgan
0 siblings, 1 reply; 9+ messages in thread
From: Christopher Warrington @ 2008-03-01 4:42 UTC (permalink / raw)
William Morgan @ 2008-2-25 11:48:19 AM
"[sup-talk] [PATCH] shell commands are now run in a child process" <mid:1203961671-sup-8158 at south>
I've been getting more of these when playing with offlineimap. I
submitted a bug upstream:
http://ferret.davebalmain.com/trac/ticket/343
--
Christopher Warrington <chrisw at rice.edu>
Today's Oxymoron: Twelve-ounce pound cake
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 183 bytes
Desc: not available
Url : http://rubyforge.org/pipermail/sup-talk/attachments/20080229/bc3669a5/attachment.bin
^ permalink raw reply [flat|nested] 9+ messages in thread
* [sup-talk] [PATCH] shell commands are now run in a child process
@ 2008-02-25 17:48 William Morgan
2008-03-01 4:42 ` Christopher Warrington
0 siblings, 1 reply; 9+ messages in thread
From: William Morgan @ 2008-02-25 17:48 UTC (permalink / raw)
Reformatted excerpts from Christopher Warrington's message of 2008-02-20:
> I wouldn't right now. I was getting ruby seg. faults with the code I
> submitted and ruby 1.8.6 (ruby bug? I don't know). Changing to code
> that closes open files descriptors until the first invalid file
> descriptor is encountered closes enough that I don't get the crash,
> but also doesn't cause a seg. fault.
Well, this wouldn't be the most cargo-cultish code that's in Sup. (See
previous message about ncurses.)
I'll keep this patch on the back burner, then, until we have a little
more experience. Is anyone else out there using Cygwin?
--
William <wmorgan-sup at masanjin.net>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-03-01 20:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-03 8:11 [sup-talk] [PATCH] shell commands are now run in a child process Christopher Warrington
2008-02-05 17:34 ` William Morgan
2008-02-05 19:29 ` Christopher Warrington
2008-02-05 20:12 ` William Morgan
2008-02-08 9:40 ` Christopher Warrington
2008-02-19 17:48 ` William Morgan
2008-02-25 17:48 William Morgan
2008-03-01 4:42 ` Christopher Warrington
2008-03-01 20:37 ` William Morgan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox