httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: logging APR errors
Date Mon, 17 Jul 2000 18:35:29 GMT

Very long message shortened to be smaller.  :-)

On Mon, 17 Jul 2000, Jeff Trawick wrote:

> The ap_pool_t structure is a convenient place to register an error
> call-back, both for APR -- available almost everywhere in APR where
> additional error information would be helpful -- and for the
> application -- ap_pool_t is a central point of interacting with APR.
> 
> A function like this would be used to register an error call-back:
> 
> ap_status_t ap_set_error(void (*apr_error)(undetermined-signature),
>                          void *user_data,
>                          ap_pool_t *p);
> 
> parm 1: address of error call-back
> parm 2: user data to be passed by APR to the call-back
> parm 3: the pool

This doesn't work.  Even if we have one callback for all of Apache, we
have to set the user data appropriately.  Because we have to set the user
data, this impacts EVERY call to APR.  You can NEVER be sure that the user
data was set correctly before entering before entering an APR
function.  

> In selected error paths in APR, code like the following would be
> inserted.
> 
>         srv = select(sock->socketdes + 1,
>             for_read ? &fdset : NULL,
>             for_read ? NULL : &fdset,
>             NULL,
>             tvptr);
>         /* TODO - timeout should be smaller on repeats of this loop */
>     } while (srv == -1 && errno == EINTR);
> 
>     if (srv == 0) {
>         return APR_TIMEUP;
>     }
>     else if (srv < 0) {
> +       if (sock->cntxt->apr_error) {
> +           sock->cntxt->apr_error(sock->cntxt->apr_error_user_data, 
> +                                  0 /* flags */, errno,
> +                                  "select()");
> +                        
> +       }
>         return errno;
>     }
>     return APR_SUCCESS;
> 
> I would not be in favor of blindly adding such logic all over the
> place.  Instead, I think it should be added proactively in APR
> functions which invoke multiple syscalls, and added on an as-needed
> basis after other syscalls where our experience shows that it would be
> useful.  Adding it everywhere brings unnecessary code bloat and more
> importantly will probably result in some amount of untested code to be
> committed (some of which will only die in front of a live studio
> audience due to the rarity of failure of certain syscalls).

Right here, you tie APR to Apache.  By just inserting this logic in places
that Apache cares about it, you make APR less useful to other
programs.  That is un-acceptable.  Either all error conditions need to be
treated equally, or we can't do this approach.  What determines when an
error case gets the call to the registered function?

> V. A few comparisions with other approproaches
> 
> A. In what ways does this improve on having APR build a string and
>    making it accessible to the application?
> 
>    i. Some interesting work is required to make sure we record
>    information on all syscall failures if we build a single string
>    (or if we build a list/stack of strings).
> 
>    ii. We build a string even if nobody cares.

This design doesn't solve this problem, in fact, IMNSHO it makes it
worse.  Think of it this way.  What if Apache doesn't want to log on a
given error.  Say for ap_sendfile, there is at least one condition we
don't want to log, but it requires the ability to make the call-back
(otherwise we are Apache centric).  Apache must de-register the callback
before calling ap_sendfile.  And then re-register it after calling
ap_sendfile, or Apache will need to register/de-register before EVERY APR
call.  There is only one way aroung this, which is to only make the
callback in the cases Apache cares about, which ties APR to Apache, which
is a BIG no-no.

> rbb@covalent.net wrote:
> >                          ...              Having every APR program
> > be able to register an error logging function adds yet another if
> > condition to EVERY single APR function.  Many of those functions have
> > multiple error paths.  This means that we get multiple if's for every APR
> > function.  People are already complaining that APR isn't performance
> > aware, and now we want to slow it down more.
> 
> 1. I'm not in favor of adding logging to every error path.

Too bad.  You can't add code in some cases and not others.  That ties you
to Apache.

> 
> 2. This is the error path we're talking about after all.  We can't
>    compare the almost-nil performance hit for adding an if and a
>    function call to an error path with some of the APR performance
>    issues which have been demonstrated recently.

Yes, this is true, iff we only make the callback in some places, we don't
add extra function calls to the non-error cases.  Since we can't just make
the callback in some places, we have to add at least one extra call in ALL
cases.

> Let's see.  The error function solves everything that has a pool and
> needs to log errors (90% of APR?).  Why doesn't this cover the dso
> errors?  Our previously discussed breakage was the inability to get
> the failing dll name on OS/2, which is interesting because it may not
> be the same as the dll being loaded.  It seems that the OS/2
> implementation of ap_dso_load() could call the OS/2 function to get
> that secondary dll name and then pass it in the var args to the error
> call-back function.

You can't use pools for this BTW.  This is the same problem with putting a
string in the pool.  The pools are not in general per thread.  Many
threads can all be using the same pool.

Yes, if you add the printf, this may cover the dso case.

> >APR needs to be able to return a string to Apache, so that Apache can use
> >that string in it's logs.
> 
> The problem with the "return a string" design is that Apache (and any
> other app) needs to write code in a great number of places to do
> something with the string.  The call-back mechanism is much easier to
> implement and avoids a lot of uninteresting and duplicated clutter.

Whether you agree or not, Apache and any other program, has to write a
great deal of code to manage the request_rec and server_rec correctly in
this design.

The design I am leaning most towards right now, is for APR to define a
bunch more error values, and just return them in the correct spots.

Ryan
_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------




Mime
View raw message