On Mon, 18 Apr 2005, Stas Bekman wrote:
> Randy Kobes wrote:
> > 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.
That's true that some of them aren't composites, but of the
56 APR_E* constants that APR::Const provides, I counted 27
composites (sometimes only on certain platforms). The tests
have encountered only IS_EAGAIN, but I thought conceivably
the others may also be needed in some circumstances, and
since there were so many, I thought it just as easy to do
them all at once (also, at some time in the future apr may
change some non-composite APR_STATUS_IS_* to a composite).
I most likely don't appreciate the implications of your
comment about exposing the API that may trigger a particular
error - if we supply an APR::Const::EFOOBAR, then is there
any added maintenance burden in also supplying an
APR_STATUS_IS_EFOOBAR(s) macro? I know some of the constants
(defines) in apr_errno.h that arise in the composites (eg,
SOCEWOULDBLOCK in APR_STATUS_IS_EAGAIN) aren't supplied by
APR::Const, but that should be taken care of at the C level.
On the other hand, simplicity is a virtue - if we wanted to
start just with IS_EAGAIN, then that's fine; it's certainly
easy to add others as the need arises.
> 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.
Good point - I'll change that.
> > 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.
Good point - thanks.
> > 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?
That'd be better - when I tried that before, I forgot to
put the @constants list in a BEGIN block, which doesn't
work. So I'll change that.
> The rest looks good.
Thanks.
--
best regards,
randy
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
