Re: svn commit: r1856274 - in /apr/apr/branches/1.7.x: ./ configure.in file_io/unix/dir.c
I'm happy to do that, and to preserve your dir.c fix. Commit coming up, crossing fingers for a clean Linux maintainer compilation. On Sun, Mar 31, 2019, 11:44 Yann Ylavic wrote: > On Sun, Mar 31, 2019 at 5:29 PM William A Rowe Jr > wrote: > > > > On Sun, Mar 31, 2019, 09:15 Yann Ylavic wrote: > >> > >> On Sun, Mar 31, 2019 at 3:58 AM William A Rowe Jr > wrote: > >> > > >> > Let me just throw this out there and get your reaction, Yann, now that > >> > I'm back in town... The patch I was initially using to accomplish the > >> > equivalent was as simple as; > >> [] > >> > Seems much less wordy for a feature we agree should just be dropped > >> > altogether fairly soon. Thoughts? > >> > >> I don't know actually, I think it's not really "fair" to say that > >> readdir() is thread-safe, you can't really call it from different > >> threads with the same DIR arg, while you might with readdir_r() > >> (besides all its caveats). I'm talking about the system functions > >> here, not apr_dir_read() which is not thread-safe (on the same > >> apr_dir_t) with either underlying function, as we discussed elsewhere. > >> But who would do that anyway?! I think we should do[x]cument that just > in case. > >> > >> As for your patch vs mine, it depends on whether it exists systems > >> (that we still support) on which readdir() is really not thread-safe > >> even with different passed-in DIRs (e.g. the returned struct dirent is > >> static somewhere in the implementation, as opposed to some room > >> reserved in the struct DIR itself like in "modern" implementations), > >> and these systems provide readdir_r() for the "thread-safe" way. > >> I suppose such system wouldn't emit a warning for readdir_r(), my > >> patch addresses them while yours does not. > >> So the question is, does such system exists and do we want to still > support it? > > > > > > I think our answer is we never did support this. > > > > My thought is that all support this, inasmuch as they support concurrent > dlopen's and readdir's on the same thread. If they are truly not thread > safe (as opposed to parallel or reentrant) on different handles that would > be a very thread-unsafe architecture in the first place, no? A perfect > example for APR has no threads at all. > > > > Since our readdir is threadsafe flag has a well established meaning > (thread safe for APR's purpose alone) and we can preserve that meaning with > no harm between 1.0 and 1.x builds (all overrides preserve the desired > behavior) I'm in favor of the simplest solution. > > > > I'm entirely in favor of dropping the macro from 2.0 and never returning > to dirread_r() after all the explanatory docs and your evaluation of APR. > If user wants to walk a single directory across parallel threads, they > clearly need to serialize the actual directory read and use a serialized > allocator in that odd edge case, much as they must do the same for any sane > file read. > > Make sense, I'm fine with this. > If you want to proceed by reverting my related changes, please go down > to r1789258 since this first commit was an (unsatisfactory) try later > overwritten by r1856189. The comment change from r1789258 might be > preserved though. > Thanks for the simplification! >
Re: svn commit: r1856274 - in /apr/apr/branches/1.7.x: ./ configure.in file_io/unix/dir.c
On Sun, Mar 31, 2019 at 5:29 PM William A Rowe Jr wrote: > > On Sun, Mar 31, 2019, 09:15 Yann Ylavic wrote: >> >> On Sun, Mar 31, 2019 at 3:58 AM William A Rowe Jr >> wrote: >> > >> > Let me just throw this out there and get your reaction, Yann, now that >> > I'm back in town... The patch I was initially using to accomplish the >> > equivalent was as simple as; >> [] >> > Seems much less wordy for a feature we agree should just be dropped >> > altogether fairly soon. Thoughts? >> >> I don't know actually, I think it's not really "fair" to say that >> readdir() is thread-safe, you can't really call it from different >> threads with the same DIR arg, while you might with readdir_r() >> (besides all its caveats). I'm talking about the system functions >> here, not apr_dir_read() which is not thread-safe (on the same >> apr_dir_t) with either underlying function, as we discussed elsewhere. >> But who would do that anyway?! I think we should do[x]cument that just in >> case. >> >> As for your patch vs mine, it depends on whether it exists systems >> (that we still support) on which readdir() is really not thread-safe >> even with different passed-in DIRs (e.g. the returned struct dirent is >> static somewhere in the implementation, as opposed to some room >> reserved in the struct DIR itself like in "modern" implementations), >> and these systems provide readdir_r() for the "thread-safe" way. >> I suppose such system wouldn't emit a warning for readdir_r(), my >> patch addresses them while yours does not. >> So the question is, does such system exists and do we want to still support >> it? > > > I think our answer is we never did support this. > > My thought is that all support this, inasmuch as they support concurrent > dlopen's and readdir's on the same thread. If they are truly not thread safe > (as opposed to parallel or reentrant) on different handles that would be a > very thread-unsafe architecture in the first place, no? A perfect example for > APR has no threads at all. > > Since our readdir is threadsafe flag has a well established meaning (thread > safe for APR's purpose alone) and we can preserve that meaning with no harm > between 1.0 and 1.x builds (all overrides preserve the desired behavior) I'm > in favor of the simplest solution. > > I'm entirely in favor of dropping the macro from 2.0 and never returning to > dirread_r() after all the explanatory docs and your evaluation of APR. If > user wants to walk a single directory across parallel threads, they clearly > need to serialize the actual directory read and use a serialized allocator in > that odd edge case, much as they must do the same for any sane file read. Make sense, I'm fine with this. If you want to proceed by reverting my related changes, please go down to r1789258 since this first commit was an (unsatisfactory) try later overwritten by r1856189. The comment change from r1789258 might be preserved though. Thanks for the simplification!
Re: svn commit: r1856274 - in /apr/apr/branches/1.7.x: ./ configure.in file_io/unix/dir.c
On Sun, Mar 31, 2019 at 3:58 AM William A Rowe Jr wrote: > > Let me just throw this out there and get your reaction, Yann, now that > I'm back in town... The patch I was initially using to accomplish the > equivalent was as simple as; [] > Seems much less wordy for a feature we agree should just be dropped > altogether fairly soon. Thoughts? I don't know actually, I think it's not really "fair" to say that readdir() is thread-safe, you can't really call it from different threads with the same DIR arg, while you might with readdir_r() (besides all its caveats). I'm talking about the system functions here, not apr_dir_read() which is not thread-safe (on the same apr_dir_t) with either underlying function, as we discussed elsewhere. But who would do that anyway?! I think we should do[x]cument that just in case. As for your patch vs mine, it depends on whether it exists systems (that we still support) on which readdir() is really not thread-safe even with different passed-in DIRs (e.g. the returned struct dirent is static somewhere in the implementation, as opposed to some room reserved in the struct DIR itself like in "modern" implementations), and these systems provide readdir_r() for the "thread-safe" way. I suppose such system wouldn't emit a warning for readdir_r(), my patch addresses them while yours does not. So the question is, does such system exists and do we want to still support it?
Re: svn commit: r1856274 - in /apr/apr/branches/1.7.x: ./ configure.in file_io/unix/dir.c
On Tue, Mar 26, 2019 at 3:35 AM wrote:
> Author: ylavic
> Date: Tue Mar 26 08:35:35 2019
> New Revision: 1856274
>
> URL: http://svn.apache.org/viewvc?rev=1856274=rev
> Log:
> Merge r1789258, r1856189, r1856191, r1856192, r1856196 from trunk:
>
>
> apr_dir_read: Since readdir() is now thread safe on most (if not all)
> unixes
> and readdir_r() is defective and deprecated, use the former by default
> unless
> APR_USE_READDIR_R is defined (no use case currently hence not
> autoconfigured).
>
>
> Follow up to r1789258: configure to detect whether readdir() is
> thread-safe.
>
> On platforms where readdir_r() is available but deprecated, readdir() is to
> be used although it's not in libc_r (e.g. Linux has no libc_r).
>
> In this case we can APR_TRY_COMPILE_NO_WARNING readdir_r() and, if it's
> deprecated, define READDIR_IS_THREAD_SAFE.
>
> With this we don't need user-defined APR_USE_READDIR{,64}_R from r1789258.
>
>
> Follow up to r1856189: sys/types.h possibly needed.
>
>
> Follow up to r1856189: use NAME_MAX from limits.h when available.
>
>
> apr_dir: no need to allocate our dir entry if readdir{,64}_r() is not used.
>
> Modified:
> apr/apr/branches/1.7.x/ (props changed)
> apr/apr/branches/1.7.x/configure.in
> apr/apr/branches/1.7.x/file_io/unix/dir.c
>
> URL:
> http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/configure.in?rev=1856274=1856273=1856274=diff
>
> ==
> --- apr/apr/branches/1.7.x/configure.in (original)
> +++ apr/apr/branches/1.7.x/configure.in Tue Mar 26 08:35:35 2019
> @@ -817,8 +817,31 @@ ac_cv_define_GETSERVBYNAME_IS_THREAD_SAF
> if test "$threads" = "1"; then
> echo "APR will use threads"
> AC_CHECK_LIB(c_r, readdir,
> + apr_readdir_is_thread_safe=yes)
> +if test "x$apr_readdir_is_thread_safe" = "x"; then
> +AC_CHECK_HEADERS(dirent.h)
> +AC_CHECK_FUNCS(readdir_r)
> +APR_IFALLYES(header:dirent.h func:readdir_r,
> + apr_has_readdir_r="1", apr_has_readdir_r="0")
> +if test "$apr_has_readdir_r" = "1"; then
> +dnl readdir_r() may exist but be deprecated, meaning
> +dnl the readdir() itself is thread-safe
> +APR_TRY_COMPILE_NO_WARNING([
> +#include
> +#include
> +],
> +[
> +DIR *dir = opendir("/tmp");
> +struct dirent entry, *result;
> +return readdir_r(dir, , ) != 0;
> +], apr_readdir_is_thread_safe=no,
> apr_readdir_is_thread_safe=yes)
> +fi
> +fi
> +if test "$apr_readdir_is_thread_safe" = "yes"; then
> +AC_MSG_NOTICE([APR will use thread-safe readdir()])
> AC_DEFINE(READDIR_IS_THREAD_SAFE, 1,
> - [Define if readdir is thread safe]))
> + [Define if readdir is thread safe])
> +fi
> if test "x$apr_gethostbyname_is_thread_safe" = "x"; then
> AC_CHECK_LIB(c_r, gethostbyname,
> apr_gethostbyname_is_thread_safe=yes)
> fi
>
Let me just throw this out there and get your reaction, Yann, now that
I'm back in town... The patch I was initially using to accomplish the
equivalent was as simple as;
--- configure.in (revision 1839621)
+++ configure.in (working copy)
@@ -835,15 +835,13 @@
fi
fi
-ac_cv_define_READDIR_IS_THREAD_SAFE=no
+ac_cv_define_READDIR_IS_THREAD_SAFE=yes
ac_cv_define_GETHOSTBYNAME_IS_THREAD_SAFE=no
ac_cv_define_GETHOSTBYADDR_IS_THREAD_SAFE=no
ac_cv_define_GETSERVBYNAME_IS_THREAD_SAFE=no
if test "$threads" = "1"; then
AC_MSG_NOTICE([APR will use threads])
-AC_CHECK_LIB(c_r, readdir,
-AC_DEFINE(READDIR_IS_THREAD_SAFE, 1,
- [Define if readdir is thread safe]))
+AC_DEFINE(READDIR_IS_THREAD_SAFE, 1, [Modern readdir is thread safe])
if test "x$apr_gethostbyname_is_thread_safe" = "x"; then
AC_CHECK_LIB(c_r, gethostbyname,
apr_gethostbyname_is_thread_safe=yes)
fi
Seems much less wordy for a feature we agree should just be dropped
altogether fairly soon. Thoughts?
