Skip to content

Remove readDirStreamWithPtr; replace readdir_r() with readdir() #349

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove readDirStreamWithPtr; replace readdir_r() with readdir() #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 14 additions & 42 deletions System/Posix/Directory/Common.hsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ module System.Posix.Directory.Common (
getRealDirType,
unsafeOpenDirStreamFd,
readDirStreamWith,
readDirStreamWithPtr,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is exported from System.Posix.Directory.Internals, but the module says "no PVP guarantees", so it's fine. Although maybe we can shim it for time being with readDirStreamWithPtr _ = readDirStreamWith?..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I reintroduced it, it would be with a deprecation warning stating it's implemented like that. Happy to do that—let me know.


rewindDirStream,
closeDirStream,
Expand Down Expand Up @@ -289,46 +288,23 @@ foreign import capi unsafe "dirent.h fdopendir"
-- if an entry was read and @Nothing@ if the end of the directory stream was
-- reached.
--
-- __NOTE:__ The lifetime of the pointer wrapped in the `DirEnt` is limited to
-- invocation of the callback and it will be freed automatically after. Do not
-- pass it to the outside world!
-- __NOTE:__ Accessing the `DirEnt` is not guaranteed to be valid after any
-- subsequent operations on the same `DirStream`. To be safe, do not pass
-- references to the `DirEnt` to the outside world.
--
-- @since 2.8.6.0
readDirStreamWith :: (DirEnt -> IO a) -> DirStream -> IO (Maybe a)
readDirStreamWith f dstream = alloca
(\ptr_dEnt -> readDirStreamWithPtr ptr_dEnt f dstream)

-- | A version of 'readDirStreamWith' that takes a pre-allocated pointer in
-- addition to the other arguments. This pointer is used to store the pointer
-- to the next directory entry, if there is any. This function is intended for
-- use cases where you need to read a lot of directory entries and want to
-- reuse the pointer for each of them. Using for example 'readDirStream' or
-- 'readDirStreamWith' in this scenario would allocate a new pointer for each
-- call of these functions.
--
-- __NOTE__: You are responsible for releasing the pointer after you are done.
-- __NOTE:__ Multiple threads reading from the same `DirStream` is not safe.
--
-- @since 2.8.6.0
readDirStreamWithPtr :: Ptr DirEnt -> (DirEnt -> IO a) -> DirStream -> IO (Maybe a)
readDirStreamWithPtr ptr_dEnt f dstream@(DirStream dirp) = do
readDirStreamWith :: (DirEnt -> IO a) -> DirStream -> IO (Maybe a)
readDirStreamWith f (DirStream dirp) = do
resetErrno
r <- c_readdir dirp (castPtr ptr_dEnt)
if (r == 0)
then do dEnt@(DirEnt dEntPtr) <- peek ptr_dEnt
if (dEntPtr == nullPtr)
cDirentPtr <- c_readdir dirp
if (cDirentPtr /= nullPtr)
then Just <$> f (DirEnt cDirentPtr)
else do (Errno eo) <- getErrno
if (eo == 0)
then return Nothing
else do
res <- f dEnt
c_freeDirEnt dEntPtr
return (Just res)
else do errno <- getErrno
if (errno == eINTR)
then readDirStreamWithPtr ptr_dEnt f dstream
else do
let (Errno eo) = errno
if (eo == 0)
then return Nothing
else throwErrno "readDirStream"
else throwErrno "readDirStream"

-- | @since 2.8.6.0
dirEntName :: DirEnt -> IO CString
Expand All @@ -345,12 +321,8 @@ foreign import ccall unsafe "__hscore_d_type"
d_type :: Ptr CDirent -> IO CChar

-- traversing directories
foreign import ccall unsafe "__hscore_readdir"
c_readdir :: Ptr CDir -> Ptr (Ptr CDirent) -> IO CInt

foreign import ccall unsafe "__hscore_free_dirent"
c_freeDirEnt :: Ptr CDirent -> IO ()

foreign import ccall unsafe "readdir"
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this works on mac? We've had horrible bugs before when not using capi.

Copy link
Member

Choose a reason for hiding this comment

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

Also see:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure this works on mac?

It passed CI so I assume so. 32-bit ARM failed though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing that to capi fails locally with

/tmp/ghc233912_0/ghc_160.c:7:102: error:
     error: unknown type name ‘DIR’
        7 | struct dirent* ghczuwrapperZC0ZCunixzm2zi8zi7zi0zminplaceZCSystemziPosixziDirectoryziCommonZCreaddir(DIR* a1) {return readdir(a1);}
          |

Busy with work so not looking into it right now.

c_readdir :: Ptr CDir -> IO (Ptr CDirent)

-- | @rewindDirStream dp@ calls @rewinddir@ to reposition
-- the directory stream @dp@ at the beginning of the directory.
Expand Down
1 change: 0 additions & 1 deletion System/Posix/Directory/Internals.hsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ module System.Posix.Directory.Internals (
isWhiteoutType,
getRealDirType,
readDirStreamWith,
readDirStreamWithPtr,
) where

import System.Posix.Directory.Common
74 changes: 0 additions & 74 deletions cbits/HsUnix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,73 +32,6 @@ int __hsunix_push_module(int fd, const char *module)
clock_t __hsunix_clocks_per_second (void) {return CLOCKS_PER_SEC;}
#endif

/*
* GNU glibc 2.23 and later deprecate `readdir_r` in favour of plain old
* `readdir` which in some upcoming POSIX standard is going to required to be
* re-entrant.
* Eventually we want to drop `readdir_r` all together, but want to be
* compatible with older unixen which may not have a re-entrant `readdir`.
* Solution is to make systems with *known* re-entrant `readdir` use that and use
* `readdir_r` wherever we have it and don't *know* that `readdir` is
* re-entrant.
*/

#if defined (__GLIBC__) && ((__GLIBC__ > 2) || (__GLIBC__ == 2) && (__GLIBC_MINOR__ >= 23))
#define USE_READDIR_R 0
#else
#define USE_READDIR_R 1
#endif

/*
* read an entry from the directory stream; opt for the
* re-entrant friendly way of doing this, if available.
*/
int __hscore_readdir( DIR *dirPtr, struct dirent **pDirEnt )
{
#if HAVE_READDIR_R && USE_READDIR_R
struct dirent* p;
int res;
static unsigned int nm_max = (unsigned int)-1;

if (pDirEnt == NULL) {
return -1;
}
if (nm_max == (unsigned int)-1) {
#ifdef NAME_MAX
nm_max = NAME_MAX + 1;
#else
nm_max = pathconf(".", _PC_NAME_MAX);
if (nm_max == -1) { nm_max = 255; }
nm_max++;
#endif
}
p = (struct dirent*)malloc(sizeof(struct dirent) + nm_max);
if (p == NULL) return -1;
res = readdir_r(dirPtr, p, pDirEnt);
if (res != 0) {
*pDirEnt = NULL;
free(p);
}
else if (*pDirEnt == NULL) {
// end of stream
free(p);
}
return res;
#else

if (pDirEnt == NULL) {
return -1;
}

*pDirEnt = readdir(dirPtr);
if (*pDirEnt == NULL) {
return -1;
} else {
return 0;
}
#endif
}

char *__hscore_d_name( struct dirent* d )
{
return (d->d_name);
Expand All @@ -112,10 +45,3 @@ char __hscore_d_type( struct dirent* d )
return CONST_DT_UNKNOWN;
#endif
}

void __hscore_free_dirent(struct dirent *dEnt)
{
#if HAVE_READDIR_R && USE_READDIR_R
free(dEnt);
#endif
}