httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@ibm.net>
Subject Re: logging APR errors
Date Mon, 17 Jul 2000 19:20:47 GMT
> From: rbb@covalent.net
> Date: Mon, 17 Jul 2000 11:35:29 -0700 (PDT)
> 
> 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.  

No way.  You just have to be sure that when creating a new pool (e.g.,
to handle a new request) that if you aren't happy with the inherited user
data that you call ap_set_error() again to update the value.

> > 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.  

Nobody said anything about logging errors that only affect Apache.
There are syscalls which FOR ANY APP either aren't known to fail
anywhere or more importantly don't need extra info for debugging.  

>                                                 What determines when an
> error case gets the call to the registered function?

If somebody thinks a certain call deserves extra log data, in goes
their code :)

I don't think we need to go hog wild and add a bunch of code that may
never execute or which we aren't willing to test ourselves.  I don't
think the extra info (sometimes hardly any) is warranted in all APR
functions. 

> > 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.

I seriously doubt that Apache or any other app is going to want to do
such a thing, but obviously it is a possibility.

> 
> > 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.

I don't see how this is Apache-centric.  It is just common sense to
me.  Consider a call to ap_listen() which fails.  FOR ANY APP, what
would this APR function pass to an error call-back to help debug the
problem?  There is only one syscall there and there is no mystery
about the parameters.  This is not the type of problem that we need to
solve.  It is already solved in fact since the caller of this
functions can log the error, as the caller has all possible info.

> > 
> > 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.

I don't understand any of your response.  There is never call-back
logic except in an error path.  Whether or not we code the call-back
logic in a certain error path (e.g., when shutdown() fails) has no
bearing on any other place we might think about coding the call-back
logic. 

> > 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.

I can see why putting a string in the pool is a problem.  I don't see
why the error call-back is a problem.  This blindness may be due to
assuming that the error call-back's user data will stay the same for
the life of a pool.  Can you please explain the thread-safe issue?

> 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.

It is pretty obvious that your definition of "great deal of code"
differs greatly based on whether or not you like a certain idea :)  My
wild uneducated guess would be that 20 LOC or fewer would be required
for tweaking the user data. 

> 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.

I'll look forward to seeing it.

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


-- 
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
     http://www.geocities.com/SiliconValley/Park/9289/
          Born in Roswell... married an alien...

Mime
View raw message