Skip to content

Fix issue 60: confusing error messages in rename/createLink/createSymbolicLink #122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hs-viktor merged 7 commits into from
Feb 24, 2021
Merged

Fix issue 60: confusing error messages in rename/createLink/createSymbolicLink #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
484566b
Clarify error message for createLink, createSymbolicLink and rename
oherrala Jun 4, 2016
652608e
Disambiguate createLink/createSymbolicLink errors
luispedro Nov 11, 2018
1a120a0
Add a test for createLink
luispedro Feb 22, 2021
bb7bb1c
Improve tests by using newer APIs
luispedro Feb 22, 2021
d92e482
Refactor by introducing throwErrnoTwoPathsIfMinus1_
luispedro Feb 22, 2021
3dae5ac
Use the more polymorphic <> to concatenate strings
luispedro Feb 23, 2021
a37b638
Improve errors for 2-argument ByteString functions
luispedro Feb 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion System/Posix/ByteString/FilePath.hsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ module System.Posix.ByteString.FilePath (
throwErrnoPathIf_,
throwErrnoPathIfNull,
throwErrnoPathIfMinus1,
throwErrnoPathIfMinus1_
throwErrnoPathIfMinus1_,
throwErrnoTwoPathsIfMinus1_
) where

import Foreign hiding ( void )
Expand All @@ -41,6 +42,9 @@ import Control.Monad
import Data.ByteString
import Data.ByteString.Char8 as BC
import Prelude hiding (FilePath)
#if !MIN_VERSION_base(4, 11, 0)
import Data.Monoid ((<>))
#endif

-- | A literal POSIX file path
type RawFilePath = ByteString
Expand Down Expand Up @@ -121,3 +125,9 @@ throwErrnoPathIfMinus1 = throwErrnoPathIf (== -1)
--
throwErrnoPathIfMinus1_ :: (Eq a, Num a) => String -> RawFilePath -> IO a -> IO ()
throwErrnoPathIfMinus1_ = throwErrnoPathIf_ (== -1)

-- | as 'throwErrnoTwoPathsIfMinus1_', but exceptions include two paths when appropriate.
--
throwErrnoTwoPathsIfMinus1_ :: (Eq a, Num a) => String -> RawFilePath -> RawFilePath -> IO a -> IO ()
throwErrnoTwoPathsIfMinus1_ loc path1 path2 =
throwErrnoIfMinus1_ (loc <> " '" <> BC.unpack path1 <> "' to '" <> BC.unpack path2 <> "'")
27 changes: 19 additions & 8 deletions System/Posix/Files.hsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,19 @@ import System.Posix.Files.Common
import System.Posix.Error
import System.Posix.Internals

#if !MIN_VERSION_base(4, 11, 0)
import Data.Monoid ((<>))
#endif

import Data.Time.Clock.POSIX (POSIXTime)

-- throwErrnoTwoPathsIfMinus1_
--
-- | For operations that require two paths (e.g., renaming a file)
throwErrnoTwoPathsIfMinus1_ :: (Eq a, Num a) => String -> FilePath -> FilePath -> IO a -> IO ()
throwErrnoTwoPathsIfMinus1_ loc path1 path2 =
throwErrnoIfMinus1_ (loc <> " '" <> path1 <> "' to '" <> path2 <> "'")

-- -----------------------------------------------------------------------------
-- chmod()

Expand Down Expand Up @@ -228,7 +239,7 @@ createLink :: FilePath -> FilePath -> IO ()
createLink name1 name2 =
withFilePath name1 $ \s1 ->
withFilePath name2 $ \s2 ->
throwErrnoPathIfMinus1_ "createLink" name1 (c_link s1 s2)
throwErrnoTwoPathsIfMinus1_ "createLink" name1 name2 (c_link s1 s2)

-- | @removeLink path@ removes the link named @path@.
--
Expand All @@ -241,18 +252,18 @@ removeLink name =
-- -----------------------------------------------------------------------------
-- Symbolic Links

-- | @createSymbolicLink file1 file2@ creates a symbolic link named @file2@
-- which points to the file @file1@.
-- | @createSymbolicLink name1 name2@ creates a symbolic link named @name2@
-- which points to the file @name1@.
--
-- Symbolic links are interpreted at run-time as if the contents of the link
-- had been substituted into the path being followed to find a file or directory.
--
-- Note: calls @symlink@.
createSymbolicLink :: FilePath -> FilePath -> IO ()
createSymbolicLink file1 file2 =
withFilePath file1 $ \s1 ->
withFilePath file2 $ \s2 ->
throwErrnoPathIfMinus1_ "createSymbolicLink" file2 (c_symlink s1 s2)
createSymbolicLink name1 name2 =
withFilePath name1 $ \s1 ->
withFilePath name2 $ \s2 ->
throwErrnoTwoPathsIfMinus1_ "createSymbolicLink" name1 name2 (c_symlink s1 s2)
Copy link
Member

@nh2 nh2 Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luispedro The error message created by this is wrong, see new bug #353.

Copy link
Contributor

@vdukhovni vdukhovni Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this should be:

  throwErrnoPathIfMinus1_ ("createSymbolicLink to " <> name1) name2 (c_symlink s1 s2)

Because we should make a reasonable effort to record the file location (source of the symlink) where the error occurred, that can be as or more useful than the descriptive string.

Example results for comparison:

λ> do { tryIOError (createSymbolicLink "/foo" "/bar") ; throwErrnoPathIfMinus1_ "createSymlink to /foo" "/bar" (pure (-1)) }
*** Exception: /bar: createSymlink to /foo: permission denied (Permission denied)

λ> do { tryIOError (createSymbolicLink "/foo" "/bar") ; throwErrnoIfMinus1_ "createSymlink /bar to /foo" (pure (-1)) }
*** Exception: createSymlink /bar to /foo: permission denied (Permission denied)

This is different from the rename case where either of the paths could be the source of the problem, but one could make a case for the source path as a sensible default, and consider also recording that.

Copy link
Member

@nh2 nh2 Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdukhovni I don't fully understand what you're saying: Isn't this simply slightly rewording the same sentence?

(I have nothing against the wording, it's good as well, but "we should make a reasonable effort to record the file location (source of the symlink)" is true either way, isn't it?)

Copy link
Contributor

@vdukhovni vdukhovni Aug 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdukhovni I don't fully understand what you're saying: Isn't this simply slightly rewording the same sentence?

(I have nothing against the wording, it's good as well, but "we should make a reasonable effort to record the file location (source of the symlink)" is true either way, isn't it?)

My issue isn't with how the resulting error is rendered by its Show method, but rather with its structure as an IOException.
See below:

λ> :set -XLambdaCase 
λ> import GHC.IO.Exception 
λ> import Control.Exception
λ> import  GHC.Internal.Foreign.C.Error 
λ> import System.Posix.Files
λ> import System.IO.Error 
λ> handler :: IOException -> Maybe IOException; handler = Just
λ> eperm :: IO (Either IOException ()); eperm = try $ createSymbolicLink "/foo" "/bar"
λ> raisePath = throwErrnoPathIfMinus1_ "createSymlink to /foo" "/bar" $ pure (-1)
λ>
λ> tryJust handler (createSymbolicLink "/foo" "/bar") >>= \ case { Left e -> pure $ ioe_filename e; Right _ -> pure (Just "no error") }
Nothing
λ> tryJust handler (eperm >> raisePath) >>= \ case { Left e -> pure $ ioe_filename e; Right _ -> pure (Just "no error") }
Just "/bar"

The difference is in the ice_filename element of the IOException:

λ> :info IOException
type IOException :: *
data IOException
  = IOError {ioe_handle :: Maybe GHC.Internal.IO.Handle.Types.Handle,
             ioe_type :: IOErrorType,
             ioe_location :: String,
             ioe_description :: String,
             ioe_errno :: Maybe GHC.Internal.Foreign.C.Types.CInt,
             ioe_filename :: Maybe FilePath}
        -- Defined in ‘GHC.Internal.IO.Exception’

By using throwErrnoPathIfMinus1_ we do a better job of populating the IOException, giving a non-vacuous ioe_filename field. We can examine all the fields and observe the difference in context, showing just ice_filename not populated (also with the current code the two names swapped per your correct bug report).

λ> tryJust handler (eperm >> raisePath) >>= \ case { Left e -> pure . Just $ (,,,,,) <$> ioe_handle <*> ioe_type <*> ioe_location <*> ioe_description <*> ioe_errno <*> ioe_filename $ e; _ -> pure Nothing }
Just (Nothing,permission denied,"createSymlink to /foo","Permission denied",Just 13,Just "/bar")
λ> 
λ> tryJust handler (createSymbolicLink "/foo" "/bar") >>= \ case { Left e -> pure . Just $ (,,,,,) <$> ioe_handle <*> ioe_type <*> ioe_location <*> ioe_description <*> ioe_errno <*> ioe_filename $ e; _ -> pure Nothing }
Just (Nothing,permission denied,"createSymbolicLink '/foo' to '/bar'","Permission denied",Just 13,Nothing)

Copy link
Member

@nh2 nh2 Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdukhovni Thanks for the example!

I agree, we definitely should populate ioe_filename as you say.


foreign import ccall unsafe "symlink"
c_symlink :: CString -> CString -> IO CInt
Expand Down Expand Up @@ -290,7 +301,7 @@ rename :: FilePath -> FilePath -> IO ()
rename name1 name2 =
withFilePath name1 $ \s1 ->
withFilePath name2 $ \s2 ->
throwErrnoPathIfMinus1_ "rename" name1 (c_rename s1 s2)
throwErrnoTwoPathsIfMinus1_ "rename" name1 name2 (c_rename s1 s2)

foreign import ccall unsafe "rename"
c_rename :: CString -> CString -> IO CInt
Expand Down
12 changes: 6 additions & 6 deletions System/Posix/Files/ByteString.hsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ createLink :: RawFilePath -> RawFilePath -> IO ()
createLink name1 name2 =
withFilePath name1 $ \s1 ->
withFilePath name2 $ \s2 ->
throwErrnoPathIfMinus1_ "createLink" name1 (c_link s1 s2)
throwErrnoTwoPathsIfMinus1_ "createLink" name1 name2 (c_link s1 s2)

-- | @removeLink path@ removes the link named @path@.
--
Expand All @@ -255,10 +255,10 @@ removeLink name =
--
-- Note: calls @symlink@.
createSymbolicLink :: RawFilePath -> RawFilePath -> IO ()
createSymbolicLink file1 file2 =
withFilePath file1 $ \s1 ->
withFilePath file2 $ \s2 ->
throwErrnoPathIfMinus1_ "createSymbolicLink" file2 (c_symlink s1 s2)
createSymbolicLink name1 name2 =
withFilePath name1 $ \s1 ->
withFilePath name2 $ \s2 ->
throwErrnoTwoPathsIfMinus1_ "createSymbolicLink" name1 name2 (c_symlink s1 s2)

foreign import ccall unsafe "symlink"
c_symlink :: CString -> CString -> IO CInt
Expand Down Expand Up @@ -296,7 +296,7 @@ rename :: RawFilePath -> RawFilePath -> IO ()
rename name1 name2 =
withFilePath name1 $ \s1 ->
withFilePath name2 $ \s2 ->
throwErrnoPathIfMinus1_ "rename" name1 (c_rename s1 s2)
throwErrnoTwoPathsIfMinus1_ "rename" name1 name2 (c_rename s1 s2)

foreign import ccall unsafe "rename"
c_rename :: CString -> CString -> IO CInt
Expand Down
34 changes: 27 additions & 7 deletions tests/FileStatus.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,21 @@ import System.Posix.Directory
import System.Posix.IO
import Control.Exception as E
import Control.Monad
import Test.Tasty.HUnit

main = do
cleanup
fs <- testRegular
ds <- testDir
testSymlink fs ds
testLink
cleanup

regular = "regular"
dir = "dir"
link_regular = "link-regular"
link_dir = "link-dir"
regular = "regular"
dir = "dir"
slink_regular = "link-regular-symlink"
hlink_regular = "link-regular-hardlink"
link_dir = "link-dir"

testRegular = do
_ <- createFile regular ownerReadMode
Expand All @@ -42,9 +45,9 @@ testDir = do
return ds

testSymlink fs ds = do
createSymbolicLink regular link_regular
createSymbolicLink regular slink_regular
createSymbolicLink dir link_dir
(fs', ls) <- getStatus link_regular
(fs', ls) <- getStatus slink_regular
(ds', lds) <- getStatus link_dir

let expected = (False,False,False,False,False,True,False)
Expand All @@ -63,10 +66,27 @@ testSymlink fs ds = do
when (statusElements ds /= statusElements ds') $
fail "status for a directory does not match when it's accessed via a symlink"


testLink = do
createLink regular hlink_regular
(fs, _) <- getStatus regular -- we need to retrieve it again as creating the link causes it to change!
(fs', ls) <- getStatus hlink_regular
snd (statusElements ls) @?= (
False, -- isBlockDevice
False, -- isCharacterDevice
False, -- isNamedPipe
True, -- isRegularFile
False, -- isDirectory
False, -- isSymbolicLink
False) -- isSocket
linkCount fs' == 2 @? "Newly created hard link was expected to have a link count of 2"
statusElements fs @?= statusElements fs' -- status for a file should match when accessed via a link


cleanup = do
ignoreIOExceptions $ removeDirectory dir
mapM_ (ignoreIOExceptions . removeLink)
[regular, link_regular, link_dir]
[regular, hlink_regular, slink_regular, link_dir]

ignoreIOExceptions io = io `E.catch`
((\_ -> return ()) :: IOException -> IO ())
Expand Down
33 changes: 26 additions & 7 deletions tests/FileStatusByteString.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,21 @@ module FileStatusByteString (main) where
import System.Posix.ByteString
import Control.Exception as E
import Control.Monad
import Test.Tasty.HUnit

main = do
cleanup
fs <- testRegular
ds <- testDir
testSymlink fs ds
testLink
cleanup

regular = "regular2"
dir = "dir2"
link_regular = "link-regular2"
link_dir = "link-dir2"
regular = "regular2"
dir = "dir2"
hlink_regular = "hlink-regular2"
slink_regular = "slink-regular2"
link_dir = "link-dir2"

testRegular = do
_ <- createFile regular ownerReadMode
Expand All @@ -41,9 +44,9 @@ testDir = do
return ds

testSymlink fs ds = do
createSymbolicLink regular link_regular
createSymbolicLink regular slink_regular
createSymbolicLink dir link_dir
(fs', ls) <- getStatus link_regular
(fs', ls) <- getStatus slink_regular
(ds', lds) <- getStatus link_dir

let expected = (False,False,False,False,False,True,False)
Expand All @@ -62,10 +65,26 @@ testSymlink fs ds = do
when (statusElements ds /= statusElements ds') $
fail "status for a directory does not match when it's accessed via a symlink"

testLink = do
createLink regular hlink_regular
(fs, _) <- getStatus regular -- we need to retrieve it again as creating the link causes it to change!
(fs', ls) <- getStatus hlink_regular
snd (statusElements ls) @?= (
False, -- isBlockDevice
False, -- isCharacterDevice
False, -- isNamedPipe
True, -- isRegularFile
False, -- isDirectory
False, -- isSymbolicLink
False) -- isSocket
linkCount fs' == 2 @? "Newly created hard link was expected to have a link count of 2"
statusElements fs @?= statusElements fs' -- status for a file should match when accessed via a link


cleanup = do
ignoreIOExceptions $ removeDirectory dir
mapM_ (ignoreIOExceptions . removeLink)
[regular, link_regular, link_dir]
[regular, hlink_regular, slink_regular, link_dir]

ignoreIOExceptions io = io `E.catch`
((\_ -> return ()) :: IOException -> IO ())
Expand Down