[darcs-devel] Better error reporting for exec_ and friends
Magnus Jonsson
magnus at smartelectronix.com
Fri Oct 6 21:23:41 PDT 2006
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,
>
>
-------------- next part --------------
New patches:
[Added rigorous error checking in exec
Magnus Jonsson <magnus at smartelectronix.com>**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
era+darcs at iki.fi**20060924111522]
[add some changelog entries
Tommy Pettersson <ptp at lysator.liu.se>**20060930120140]
[remove duplicate file names in fix_filepaths (fixes issue273)
Tommy Pettersson <ptp at lysator.liu.se>**20060929145335]
[add test for replace command with duplicated file name
Tommy Pettersson <ptp at lysator.liu.se>**20060929144008]
[remove some tabs from darcs source
Tommy Pettersson <ptp at lysator.liu.se>**20060929211203]
[--matches now accepts logical 'and' 'or' '!' in addition to '&&' '||' 'not'.
Pekka Pessi <ppessi at gmail.com>**20060915140406]
[Canonize Era Eriksson.
Eric Kow <eric.kow at loria.fr>**20060928223224]
[Move bug reporting code to its own module.
Eric Kow <eric.kow at loria.fr>**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 <droundy at darcs.net>**20060917214136]
[Reword paragraph about Procmail's umask handling
era+darcs at iki.fi**20060924114546
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
era+darcs at iki.fi**20060924111313]
["Granting access to a repository": remove odd orphaned? sentence
era+darcs at iki.fi**20060924111142]
[era's trivial typo fixes
era+darcs at iki.fi**20060924110945
* 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 <ptp at lysator.liu.se>**20060916010645]
[redirect errors to null where exec output is used but failure is not fatal
Tommy Pettersson <ptp at lysator.liu.se>**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 <ptp at lysator.liu.se>**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 <ptp at lysator.liu.se>**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 <ptp at lysator.liu.se>**20060915222923]
[reindent some long lines
Tommy Pettersson <ptp at lysator.liu.se>**20060915222654]
[update calls to exec and exec_fancy to new interface
Tommy Pettersson <ptp at lysator.liu.se>**20060915222226]
[fix typo
Tommy Pettersson <ptp at lysator.liu.se>**20060915164446]
[rewrite Exec.lhs, new exec interface with Redirects
Tommy Pettersson <ptp at lysator.liu.se>**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 <eric.kow at gmail.com>**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 <dagit at codersbase.com>**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 <kirr at landau.phys.spbu.ru>**20060912080004]
[Be explicit about timezone handling (issue220); assume local by default.
Eric Kow <eric.kow at gmail.com>**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 <eric.kow at gmail.com>**20060610193049
]
[move test for tabs from makefile to haskell_policy test
Tommy Pettersson <ptp at lysator.liu.se>**20060730122348]
[add test for haskell policy
Tommy Pettersson <ptp at lysator.liu.se>**20060730121404]
[ratify some uses of readFile and hGetContents
Tommy Pettersson <ptp at lysator.liu.se>**20060730121158]
[Remove direct dependency to mapi32.dll; Improve MAPI compatibility.
Esa Ilari Vuokko <ei at vuokko.info>**20051130000915]
[Canonize Kirill Smelkov and Anders Hockersten.
Eric Kow <eric.kow at gmail.com>**20060910052541]
[Correct 'one one' in web page.
Eric Kow <eric.kow at loria.fr>**20060908191241]
[Fix merge conflicts.
Juliusz Chroboczek <jch at pps.jussieu.fr>**20060906191317]
[fix bug in pristine handling when dealing with multiple patches.
David Roundy <droundy at darcs.net>**20060731111404]
[fix ordering of operations to call pull_first_middles properly.
David Roundy <droundy at darcs.net>**20060730111409]
[make amend-record.pl test a bit pickier.
David Roundy <droundy at darcs.net>**20060730103854]
[simplify code a tad in get.
David Roundy <droundy at darcs.net>**20060726121737]
[fix bug in refactoring of get.
David Roundy <droundy at darcs.net>**20060726121655]
[refactor Population.
David Roundy <droundy at darcs.net>**20060716034837]
[add TODO for refactoring get_markedup_file.
David Roundy <droundy at darcs.net>**20060716034339]
[partial refactoring in annotate.
David Roundy <droundy at darcs.net>**20060716034319]
[don't use DarcsRepo in list_authors.
David Roundy <droundy at darcs.net>**20060716033450]
[I've now eliminated need to export DarcsRepo.write_patch.
David Roundy <droundy at darcs.net>**20060716033109]
[partially refactor Optimize.
David Roundy <droundy at darcs.net>**20060716032934]
[partial refactoring of Get.
David Roundy <droundy at darcs.net>**20060716031605]
[refactor amend-record.
David Roundy <droundy at darcs.net>**20060716021003]
[add TODO to refactor unrevert handling.
David Roundy <droundy at darcs.net>**20060716020247]
[refactor Unrecord, adding tentativelyRemovePatches.
David Roundy <droundy at darcs.net>**20060716015150]
[refactor tag.
David Roundy <droundy at darcs.net>**20060716011853]
[refactor Repository to allow truly atomic updates.
David Roundy <droundy at darcs.net>**20060716011245]
[Do not redirect to or from /dev/null when calling ssh.
Eric Kow <eric.kow at loria.fr>**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 <eric.kow at gmail.com>**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 <jch at pps.jussieu.fr>**20060831231933]
[overhaul the darcs.net front page.
Mark Stosberg <mark at summersault.com>**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 <eric.kow at gmail.com>**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 <eric.kow at gmail.com>**20060812121943]
[Rename AtExit module to Global.
Eric Kow <eric.kow at gmail.com>**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 <kapheine at divineinvasion.net>**20060810034035]
[Make the pull 'permission test' work when run as root
Jon Olsson <jon at vexed.se>**20060831193834]
[TAG darcs-unstable-20060831
Juliusz Chroboczek <jch at pps.jussieu.fr>**20060831191554]
Patch bundle hash:
efb428c341b8afcb6457ef3763d15c18b96975e1
More information about the darcs-devel
mailing list