In the discussion of a failure on Win32 of t/error/runtime at http://marc.theaimsgroup.com/?t=111217856900001&r=1&w=2, Stas suggested adding an APR::Status, containing things like APR_STATUS_IS_EAGAIN(s) from apr_errno.h. I've attached a patch against the current mp2 svn implementing such a thing, which includes all the APR_STATUS_IS_* macros available for the 'error' codes of APR::Const in Apache2::ConstantsTable. The patch includes some tests, as well as a modification to t/response/TestError/runtime.pm using APR::Status; with this, the t/error/runtime test passes on Win32.
Thanks Randy for working on this one :)
Do we really want to expose all those macros? I bet some of those will never be used since we don't expose the APIs which can possibly trigger those error conditions. Exposing APIs comes with maintenance burden. So I'd suggest exposing only those we know are needed. i.e. IS_EAGAIN, and may be some others that we should decide on. Especially notice that most macros are not needed, as they are not composites, e.g.:
#define APR_STATUS_IS_EOF(s) ((s) == APR_EOF)
which can already be easily done with what we have.
If it was me deciding I'd just start by adding IS_EAGAIN and add others as we discover the need. If you can see something else that will certainly be useful in the future, please suggest which.
Now to the patch:
> Index: xs/maps/apr_functions.map > +MODULE=APR::Status PREFIX=mpxs_APR__STATUS_ [...] > + mpxs_APR__STATUS_is_eagain
I think those should be upcase: IS_EAGAIN, since it's a macro after all, matching error constants. eagain doesn't look as familiar as EAGAIN.
> Index: xs/APR/Status/APR__Status.h
> +#include "apr_errno.h" [...]
> +static MP_INLINE int mpxs_APR__STATUS_is_eagain(pTHX_ apr_status_t rc)
> +{
> + return APR_STATUS_IS_EAGAIN(rc) ? TRUE : FALSE;
> +}That should be a #define macro, not a func. Why adding an overhead of func call.
> Index: t/lib/TestAPRlib/status.pm > =================================================================== > --- t/lib/TestAPRlib/status.pm (revision 0) > +++ t/lib/TestAPRlib/status.pm (revision 0) > @@ -0,0 +1,113 @@ > +package TestAPRlib::status; > + > +# Testing APR::Status > + > +use strict; > +use warnings FATAL => 'all'; > + > +use Apache::Test; > +use Apache::TestUtil; > + > +our @constants = qw(ENOSTAT ENOPOOL EBADDATE EINVALSOCK > + ENOPROC ENOTIME ENODIR ENOLOCK ENOPOLL > + ENOSOCKET ENOTHREAD ENOTHDKEY EGENERAL > + ENOSHMAVAIL EBADIP EBADMASK EDSOOPEN > + EABSOLUTE ERELATIVE EINCOMPLETE EABOVEROOT > + EBADPATH EPATHWILD ESYMNOTFOUND EPROC_UNKNOWN > + EOF EINIT ENOTIMPL EMISMATCH EBUSY > + EACCES EEXIST ENAMETOOLONG ENOENT > + ENOTDIR ENOSPC ENOMEM EMFILE ENFILE > + EBADF EINVAL ESPIPE EAGAIN EINTR ENOTSOCK > + ECONNREFUSED EINPROGRESS ECONNABORTED > + ECONNRESET ETIMEDOUT EHOSTUNREACH > + ENETUNREACH EFTYPE EPIPE EXDEV ENOTEMPTY > + ); > + > +use APR::Const -compile => qw(ENOSTAT ENOPOOL EBADDATE EINVALSOCK
why not using @constants here, and repeat all the constants again?
The rest looks good.
-- __________________________________________________________________ Stas Bekman JAm_pH ------> Just Another mod_perl Hacker http://stason.org/ mod_perl Guide ---> http://perl.apache.org mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com http://modperlbook.org http://apache.org http://ticketmaster.com
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
