Hi Eric and darcs-devel,
I have written a new patch in light of Tommy Petterson's changes to Exec
and the comments on my previous patch. This time the patch is all Haskell
code. See the attachment.
Best regards,
Magnus Jonsson
On Fri, 29 Sep 2006, Eric Y. Kow wrote:
On Fri, Sep 29, 2006 at 00:39:21 -0400, Magnus Jonsson wrote:
1) It adds some new exit codes for internal usage, paraphrasing the
comments,
-1 for fork() failure
-2 for pipe(),
-3 execvp_no_vtalarm()
Dut does this mean we're counting on the programs themselves never
exiting with these codes?
exec_extern returns process id of the newly launched child process, or one
of those error codes if it fails. There used to be no/little error
checking here.
Oops! I just realised in the shower that those were process ids you were
returning, not exit codes. (Which makes questions 1 and 2 rather
nonsensical; sorry about that).
doesn't carry with it any exit code, because no program was run.
3) I'm a bit nervous about catching ExitFailure at main - it seems like
this is something we would want to bubble up. How do you know this
doesn't misreport ExitFailures thrown by darcs itself?
It is better than the current status I think - right now these failures
pass unseen and if here is a failure, the desired side effects never take
place. This way darcs realizes that there's a problem and bails out. Darcs
itself shouldn't throw any ExecFailure. Only Exec.hs should throw
ExecFailure.
Oh, I see; that is a new exception type _Exec_Failure, hence the
catchDyn, not ExitFailure, which for that matter probably doesn't even
exist (exitFailure). Consider me less nervous.
A new question for you
5) Why use bug? This indicates that darcs itself is broken; is an
ExecFailure an indication that darcs is broken?
All the best,
New patches:
[Added rigorous error checking in exec
Magnus Jonsson <[EMAIL PROTECTED]>**20061006222630
All lowlevel C return values are checked and turned into
exceptions if they are error codes. In darcs main
ExecExceptions are caught and turned into error messages
to help the user.
] {
hunk ./Exec.lhs 18
-{-# OPTIONS -fffi #-}
+{-# OPTIONS -fffi -fglasgow-exts #-}
hunk ./Exec.lhs 21
+ ExecException(..)
hunk ./Exec.lhs 27
+import Data.Typeable ( Typeable )
+import Control.Exception ( throwDyn, handleJust, userErrors )
hunk ./Exec.lhs 35
-import Monad ( liftM )
hunk ./Exec.lhs 65
+ deriving Show
+
+{-
+ ExecException is thrown by exec if any system call fails,
+ for example because the executable we're trying to run
+ doesn't exist.
+-}
+-- ExecException cmd args redirecs errorDesc
+data ExecException = ExecException String [String] Redirects String
+ deriving (Typeable,Show)
hunk ./Exec.lhs 108
- On Unix we fork, use dup2 for redirections (after opening
- relevant files). Then we exec the command in the child, and wait
- for its exit status in the parent.
+ On Unix we create a pipe, fork, use dup2 for redirections (after opening
+ relevant files). Then we exec the command in the child. If the exec fails
+ the child sends back an error string over the pipe. The parent process waits
+ for the child's exit status and also checks if an error was sent on the pipe.
+ If any system call returns an error code, we throw an ExecException.
hunk ./Exec.lhs 115
-exec cmd args redirs = do
- fval <- c_fork
- case fval of
- -1 -> return $ ExitFailure $ 1
- 0 -> -- child
+exec cmd args redirs =
+ handleJust userErrors
+ (throwDyn . ExecException cmd args redirs) $ do
+ (readFd,writeFd) <- pipe
+ fval <- c_fork
+ case fval of
+ -1 -> do err <- errno
+ c_close readFd
+ c_close writeFd
+ failWithErrno "fork" err
+ 0 -> do -- child
+ c_close readFd
+ let sendError::String->IO a -- does not return
+ sendError str = do
+ writeFullString writeFd str
+ c_close writeFd
+ exitWith (ExitFailure 127)
+ bracket (return ())
+ (\()->sendError "uncaught exception") $ \()-> do
+ handleJust userErrors sendError $
hunk ./Exec.lhs 136
- withCString cmd $ \c_cmd ->
- withCStrings (cmd:args) $ \c_args -> do
- -- execvp only returns if there is an error:
- ExitFailure `liftM` execvp_no_vtalarm c_cmd c_args
- pid -> -- parent
- do ecode <- smart_wait pid
- if ecode == 0 then return ExitSuccess
- else return $ ExitFailure ecode
+ execvp_no_vtalarm cmd args
+ -- execvp either doesn't return or throws a userError.
+ -- If it doesn't return, writeFd will be closed
+ -- automatically by the OS.
+ pid -> do -- parent
+ c_close writeFd
+ -- read what the child process sent on the pipe
+ pipestr <- readUntilEof readFd
+ c_close readFd
+ ecode <- smart_wait pid
+ case pipestr of
+ "" -> -- child process succeeded in executing command
+ return $ if ecode == 0 then ExitSuccess
+ else ExitFailure (fromIntegral ecode)
+ _ -> -- child sent an error through the pipe
+ fail pipestr
hunk ./Exec.lhs 161
- redirect std_fd Stdout = c_dup2 1 std_fd >> return ()
- redirect std_fd (File fp) = withCString fp $ \c_fp -> do
- file_fd <- open_like std_fd c_fp
- c_dup2 file_fd std_fd
+ redirect std_fd Stdout = do dup2 1 std_fd
+ return ()
+ redirect std_fd (File fp) = do file_fd <- open_like std_fd fp
+ dup2 file_fd std_fd
hunk ./Exec.lhs 171
+{-
+ Consume a whole stream from an fd and return a string.
+ Throws a userError if any system call fails.
+-}
+
+readUntilEof :: CInt -> IO String
+readUntilEof fd =
+ allocaBytes 1000 $ \ buffer ->
+ let loop :: IO String
+ loop = do
+ readResult <- retryWhileEINTR $ c_read fd buffer 1000
+ case compare readResult 0 of
+ EQ -> return "" -- 0 means end of file
+ LT -> failWithErrno "read from pipe" (fromIntegral readResult)
+ GT -> do str <- peekCStringLen (buffer,fromIntegral readResult)
+ rest <- loop
+ return (str++rest)
+ in loop
+
+{-
+ Writes a string to an fd, making sure that the whole
+ string is sent.
+ NB. It does NOT throw any exception if a system call
+ fails.
+-}
+
+writeFullString :: CInt -> String -> IO ()
+writeFullString fd str =
+ withCStringLen str $ \ (c_str,len) ->
+ let loop::Int->IO ()
+ loop pos =
+ case compare pos len of
+ EQ -> return ()
+ LT -> do
+ result <-
+ retryWhileEINTR $
+ c_write fd (plusPtr c_str pos) (fromIntegral (len-pos))
+ case compare result 0 of
+ LT -> return () -- failed to write for some reason... but
+ -- there is no point in throwing an
error.
+ GT -> loop (pos+fromIntegral result)
+ EQ -> loop pos
+ GT -> impossible
+ in loop 0
+
+
+
+{-
+ Wrappers around lowlevel C functions so that they throw
+ userErrors in case of failure and are retried if they
+ return EINTR.
+-}
+
+execvp_no_vtalarm::String->[String]->IO a
+execvp_no_vtalarm cmd args =
+ do withCString cmd $ \c_cmd ->
+ withCStrings (cmd:args) $ \c_args ->
+ c_execvp_no_vtalarm c_cmd c_args
+ -- execvp only returns if there was an error
+ -- so we throw an exception
+ err <- errno
+ failWithErrno "execvp" err
+
+open_read::String->IO CInt
+open_read fname =
+ withCString fname (failOnMinus1 "open_read" . c_open_read)
+
+open_write::String->IO CInt
+open_write fname =
+ withCString fname (failOnMinus1 "open_write" . c_open_write)
+
+dup2::CInt->CInt->IO CInt
+dup2 oldfd newfd = failOnMinus1 "dup2" $ c_dup2 oldfd newfd
+
+
+pipe::IO (CInt,CInt) -- (readFd, writeFd)
+pipe =
+ allocaArray 2 $ \ fileDescs -> do
+ failOnMinus1 "pipe" $ c_pipe fileDescs
+ [readFd,writeFd] <- peekArray 2 fileDescs
+ return (readFd,writeFd)
+
+errno::IO CInt
+errno = peek c_errno
+
+strerror::CInt->IO String
+strerror err = do
+ cstr <- c_strerror err
+ case () of
+ _ | cstr == nullPtr -> return "unknown error"
+ | otherwise -> peekCString cstr
+
+
+{-
+ Helper used to wrap C routines that return -1 on failure.
+ If there is a failure, it fails with a string derived from errno,
+ unless errno==EINTR in which case the C call will be retried.
+-}
+
+failOnMinus1::String->IO CInt->IO CInt
+failOnMinus1 context job = do
+ result <- job
+ case result of
+ -1 -> do e <- errno
+ case () of
+ _ | Errno e == eOK -> impossible
+ | Errno e == eINTR -> failOnMinus1 context job
+ | otherwise -> failWithErrno context e
+ _ -> return result
+
+
+
+{-
+ Helper used to wrap C routines like read and write that
+ return EINTR when interrupted by signals.
+-}
+
+retryWhileEINTR :: Integral a => IO a -> IO a
+retryWhileEINTR job = do
+ result <- job
+ case () of
+ _ | Errno (fromIntegral result) == eINTR -> retryWhileEINTR job
+ | otherwise -> return result
+
+
+{-
+ Looks up the error string for a particular
+ errno and throws a userError containing it.
+-}
+
+failWithErrno :: String -> CInt -> IO a
+failWithErrno context err = do
+ str <- strerror err
+ fail (context++": "++str)
+
hunk ./Exec.lhs 307
-foreign import ccall unsafe "static unistd.h dup2" c_dup2
- :: CInt -> CInt -> IO CInt
hunk ./Exec.lhs 309
-foreign import ccall unsafe "static compat.h open_read" open_read
+foreign import ccall unsafe "static compat.h open_read" c_open_read
hunk ./Exec.lhs 311
-foreign import ccall unsafe "static compat.h open_write" open_write
+foreign import ccall unsafe "static compat.h open_write" c_open_write
hunk ./Exec.lhs 313
-foreign import ccall unsafe "static unistd.h fork" c_fork
- :: IO Int
hunk ./Exec.lhs 314
- "static compat.h execvp_no_vtalarm" execvp_no_vtalarm
+ "static compat.h execvp_no_vtalarm" c_execvp_no_vtalarm
hunk ./Exec.lhs 316
+foreign import ccall unsafe "static unistd.h fork" c_fork
+ :: IO Int
+foreign import ccall unsafe "static unistd.h dup2" c_dup2
+ :: CInt -> CInt -> IO CInt
+foreign import ccall unsafe "static unistd.h pipe" c_pipe
+ :: Ptr CInt -> IO CInt
+foreign import ccall unsafe "static unistd.h close" c_close
+ :: CInt->IO CInt
+foreign import ccall unsafe "static unistd.h read" c_read
+ :: CInt->Ptr a->CSize->IO CSize
+foreign import ccall unsafe "static unistd.h write" c_write
+ :: CInt->Ptr a->CSize->IO CSize
+foreign import ccall unsafe "static errno.h &errno" c_errno
+ :: Ptr CInt
+foreign import ccall unsafe "static errno.h strerror" c_strerror
+ :: CInt -> IO CString
hunk ./darcs.lhs 34
-import Control.Exception ( Exception( AssertionFailed ), handleJust, )
+import Control.Exception ( Exception( AssertionFailed ), handleJust, catchDyn )
hunk ./darcs.lhs 54
+import Exec ( ExecException(..) )
hunk ./darcs.lhs 661
+execExceptionHandler :: ExecException -> IO a
+execExceptionHandler (ExecException cmd args redirects reason) =
+ do putStrLn $ "Failed to execute external command: " ++ unwords (cmd:args)
++ "\n"
+ ++ "Lowlevel error: " ++ reason ++ "\n"
+ ++ "Redirects: " ++ show redirects ++"\n"
+ exitWith $ ExitFailure 3
+
hunk ./darcs.lhs 669
-main = with_atexit $ withSignalsHandled $ handleJust assertions bug $ do
+main = with_atexit $ withSignalsHandled $
+ flip catchDyn execExceptionHandler $
+ handleJust assertions bug $ do
}
Context:
[In procmail examples, don't use a lock file
[EMAIL PROTECTED]
[add some changelog entries
Tommy Pettersson <[EMAIL PROTECTED]>**20060930120140]
[remove duplicate file names in fix_filepaths (fixes issue273)
Tommy Pettersson <[EMAIL PROTECTED]>**20060929145335]
[add test for replace command with duplicated file name
Tommy Pettersson <[EMAIL PROTECTED]>**20060929144008]
[remove some tabs from darcs source
Tommy Pettersson <[EMAIL PROTECTED]>**20060929211203]
[--matches now accepts logical 'and' 'or' '!' in addition to '&&' '||' 'not'.
Pekka Pessi <[EMAIL PROTECTED]>**20060915140406]
[Canonize Era Eriksson.
Eric Kow <[EMAIL PROTECTED]>**20060928223224]
[Move bug reporting code to its own module.
Eric Kow <[EMAIL PROTECTED]>**20060928222826
Fixes circular dependency caused by David's unrevert cleanup (which moves
edit_file to DarcsUtil, thus causing it to depend on Exec) and Tommy's
exec patches (which add impossible.h to Exec, thus causing it to depend
on DarcsUtil).
]
[clean up unrevert and pending handling.
David Roundy <[EMAIL PROTECTED]>**20060917214136]
[Reword paragraph about Procmail's umask handling
[EMAIL PROTECTED]
The explanation now helpfully hints that similar tricks may be necessary
in other mail programs, too
]
[Wrap .muttrc example so it doesn't bleed into margin in PostScript version
[EMAIL PROTECTED]
["Granting access to a repository": remove odd orphaned? sentence
[EMAIL PROTECTED]
[era's trivial typo fixes
[EMAIL PROTECTED]
* best_practices.tex (subsection{Conflicts}): \emph pro \verb
around emphasized word "only"
* DarcsArguments.lhs (intersection_or_union): uppercase "[DEFAULT]";
(disable_ssh_cm docs): remove duplicate "which"
* Help.lhs: Missing full stop in description of --extended-help
* Mv.lhs (mv_description): Missing apostrophe in "Apple's"
* PatchShow.lhs (showHunk): Replace "that the white space must not"
with "that whitespace must not"
]
[show error messages when starting and stoping the ssh control master
Tommy Pettersson <[EMAIL PROTECTED]>**20060916010645]
[redirect errors to null where exec output is used but failure is not fatal
Tommy Pettersson <[EMAIL PROTECTED]>**20060916010116
Error messages in the output would destroy the result, but if the command
fails some other action is taken, so error messages shall not be displayed
to the user.
]
[redirect errors to stderr where exec output is used
Tommy Pettersson <[EMAIL PROTECTED]>**20060916005651
Error messages would destroy the result if they ended up in the output.
If the external command fails, darcs should (but does not always) fail.
]
[redirect errors to stderr where exec is checked and darcs fails
Tommy Pettersson <[EMAIL PROTECTED]>**20060916004407
In these situations the user will get both the error message from the
failing external command and a message from darcs about what action it
could not perform.
]
[simplify helper function stupidexec in copyRemoteCmd
Tommy Pettersson <[EMAIL PROTECTED]>**20060915222923]
[reindent some long lines
Tommy Pettersson <[EMAIL PROTECTED]>**20060915222654]
[update calls to exec and exec_fancy to new interface
Tommy Pettersson <[EMAIL PROTECTED]>**20060915222226]
[fix typo
Tommy Pettersson <[EMAIL PROTECTED]>**20060915164446]
[rewrite Exec.lhs, new exec interface with Redirects
Tommy Pettersson <[EMAIL PROTECTED]>**20060911102933
Make the code structure a bit simpler and easier to understand.
Only one (fancy) version of exec.
]
[Fix Windows stderr non-redirection.
Eric Kow <[EMAIL PROTECTED]>**20060909055204
(It was consistently redirecting to stdout.)
Also make the exec code more readable/transparent.
]
[whatsnew --look-for-adds doesn't read unadded files (fix for issue79)
Jason Dagit <[EMAIL PROTECTED]>**20060910193803
The default mode for whatsnew --look-for-adds is summary mode. In summary
mode full patches are not needed. This fix changes whatsnew
--look-for-adds to stop computing the full patch for a file when the
file is not managed by darcs.
]
[Correct canonical email for Kirill Smelkov
Kirill Smelkov <[EMAIL PROTECTED]>**20060912080004]
[Be explicit about timezone handling (issue220); assume local by default.
Eric Kow <[EMAIL PROTECTED]>**20060812102034
Except for the local timezone in the user interface, this patch is not
expected to change darcs's behaviour. It merely makes current practice
explicit:
- Assume local timezone when parsing date strings from the user
interface (previous behaviour was assuming UTC).
- Assume UTC timezone when parsing date strings from PatchInfo.
Newer patch date strings do *not* specify the timezone, so it
would be prudent to treat these as UTC.
- Disregard timezone information altogether when reading patch
dates (issue220). Note that this bug was not caused by assuming local
timezone, because legacy patch date strings explicitly tell you what
the timezone to use. The bug was caused by a patch that fixed
issue173 by using timezone information correctly. To preserve
backwards-compatability, we deliberatly replicate the incorrect
behaviour of overriding the timezone with UTC.
(PatchInfo.make_filename)
]
[Account for timezone offset in cleanDate (Fixes issue173).
Eric Kow <[EMAIL PROTECTED]>**20060610193049
]
[move test for tabs from makefile to haskell_policy test
Tommy Pettersson <[EMAIL PROTECTED]>**20060730122348]
[add test for haskell policy
Tommy Pettersson <[EMAIL PROTECTED]>**20060730121404]
[ratify some uses of readFile and hGetContents
Tommy Pettersson <[EMAIL PROTECTED]>**20060730121158]
[Remove direct dependency to mapi32.dll; Improve MAPI compatibility.
Esa Ilari Vuokko <[EMAIL PROTECTED]>**20051130000915]
[Canonize Kirill Smelkov and Anders Hockersten.
Eric Kow <[EMAIL PROTECTED]>**20060910052541]
[Correct 'one one' in web page.
Eric Kow <[EMAIL PROTECTED]>**20060908191241]
[Fix merge conflicts.
Juliusz Chroboczek <[EMAIL PROTECTED]>**20060906191317]
[fix bug in pristine handling when dealing with multiple patches.
David Roundy <[EMAIL PROTECTED]>**20060731111404]
[fix ordering of operations to call pull_first_middles properly.
David Roundy <[EMAIL PROTECTED]>**20060730111409]
[make amend-record.pl test a bit pickier.
David Roundy <[EMAIL PROTECTED]>**20060730103854]
[simplify code a tad in get.
David Roundy <[EMAIL PROTECTED]>**20060726121737]
[fix bug in refactoring of get.
David Roundy <[EMAIL PROTECTED]>**20060726121655]
[refactor Population.
David Roundy <[EMAIL PROTECTED]>**20060716034837]
[add TODO for refactoring get_markedup_file.
David Roundy <[EMAIL PROTECTED]>**20060716034339]
[partial refactoring in annotate.
David Roundy <[EMAIL PROTECTED]>**20060716034319]
[don't use DarcsRepo in list_authors.
David Roundy <[EMAIL PROTECTED]>**20060716033450]
[I've now eliminated need to export DarcsRepo.write_patch.
David Roundy <[EMAIL PROTECTED]>**20060716033109]
[partially refactor Optimize.
David Roundy <[EMAIL PROTECTED]>**20060716032934]
[partial refactoring of Get.
David Roundy <[EMAIL PROTECTED]>**20060716031605]
[refactor amend-record.
David Roundy <[EMAIL PROTECTED]>**20060716021003]
[add TODO to refactor unrevert handling.
David Roundy <[EMAIL PROTECTED]>**20060716020247]
[refactor Unrecord, adding tentativelyRemovePatches.
David Roundy <[EMAIL PROTECTED]>**20060716015150]
[refactor tag.
David Roundy <[EMAIL PROTECTED]>**20060716011853]
[refactor Repository to allow truly atomic updates.
David Roundy <[EMAIL PROTECTED]>**20060716011245]
[Do not redirect to or from /dev/null when calling ssh.
Eric Kow <[EMAIL PROTECTED]>**20060903214831
Redirection of stdin and stdout breaks putty, which uses these to
interact with the user. Quiet mode, and redirecting stderr are good
enough for making ssh silent.
]
[Exec improvements : Windows redirection, and more redirection control.
Eric Kow <[EMAIL PROTECTED]>**20060707054134
- Implement ability to redirect to /dev/null under Windows
(eivuokko on #darcs points out that it is NUL under Windows)
- Add exec_ function, which does the same thing as exec,
but allows redirection on stderr, and also allows us
to NOT redirect stdin/stderr
]
[Ignore .git if _darcs found.
Juliusz Chroboczek <[EMAIL PROTECTED]>**20060831231933]
[overhaul the darcs.net front page.
Mark Stosberg <[EMAIL PROTECTED]>**20060820191415
The themes to this change are:
- Focus on the key benefits of darcs:
Distributed. Interactive. Smart.
- Recognize that the wiki is the central resource,
and remove some information that is duplicated here
and reference the wik instead.
I can post a demo of this HTML for easy comparison if you'd like.
Mark
]
[Reimplement --disable-ssh-cm flag (issue239).
Eric Kow <[EMAIL PROTECTED]>**20060812134856
My patch to "Only launch SSH control master on demand" accidentally
removed the ability to disable use of SSH ControlMaster. Also, the
way it was implemented is not compatible with launching on demand.
This implementation relies on a notion of global variables using
unsafe IORefs.
]
[Compile Global.lhs in place of AtExit.lhs.
Eric Kow <[EMAIL PROTECTED]>**20060812121943]
[Rename AtExit module to Global.
Eric Kow <[EMAIL PROTECTED]>**20060812121925
The goal is to capture some broad "global" notions like exit handlers
and global variables. Note the GPL header thrown in for good measure.
]
[Raise exception if unable to open logfile (issue142).
Zachary P. Landau <[EMAIL PROTECTED]>**20060810034035]
[Make the pull 'permission test' work when run as root
Jon Olsson <[EMAIL PROTECTED]>**20060831193834]
[TAG darcs-unstable-20060831
Juliusz Chroboczek <[EMAIL PROTECTED]>**20060831191554]
Patch bundle hash:
efb428c341b8afcb6457ef3763d15c18b96975e1
_______________________________________________
darcs-devel mailing list
[email protected]
http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
