perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Randy Kobes <ra...@theoryx5.uwinnipeg.ca>
Subject Re: adding APR::Status?
Date Tue, 19 Apr 2005 05:51:23 GMT
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: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org


Mime
View raw message