httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <trawi...@bellsouth.net>
Subject Re: cvs commit: apache-2.0/src/lib/apr/dso/os2 Makefile.in dso.c dso.h
Date Tue, 28 Mar 2000 12:38:53 GMT
I'll try to compare what I think is the code required to record
an error string using a couple of alternative implementations.  Please
help me out where I falter or am unfair.

Assumptions: 

1) the diagnostic string exists only to be traced; the APR app doesn't
have to use the extra diagnostic information in order to choose which
path to take
2) ??

A. app gives APR a call-back function for recording error information

Not knowing any better, I'd store a ptr in context_t.

i. ptr in context_t

context_t is available almost everywhere in APR.  

struct context_t {
    struct ap_pool_t *pool;
    datastruct *prog_data;
    int (*apr_abort)(int retcode);
+   void (*apr_log_error)(int loglvl, const char *fmt, ...);
};     

This needs a related function called ap_set_log_error() (analogous to
ap_set_abort).  Very simple...  (Hmmm... ap_set_abort() only
implemented for UNIX today; what about beos, win32, other versions of
misc/start.c?)

ii.  APR app must store ptr to log function in context_t

We could think about whether or not creating a new context should
inherit the value from the parent context (probably) so that initially
Apache only needs to store it once.  In the future, Apache could
easily choose to provide the log_error function only for certain
clients (CEO that can't load some obscure page?) simply by only
setting apr_log_error in context for certain connections (dynamic
reconfig and new trace specifications very helpful here).

iii. selected APR code calls apr_log_error()

Typical APR code.  Assume this is in some os-specific file where we
"know" we have extra diagnostic data.

ap_status_t apr_do_something(struct ap_widget_t *widget, int parm, 
		             struct context_t *cont)
{
    ...
    rc = some_kernel_call(widget->handle, parm);
    if (rc) {
        status = errno;
        if (cont->apr_log_error) {
	    cont->apr_log_error(APR_LOG_ERROR,
				"some_kernel_call() -> %d/%08X",
				status, __errno2());
	}
	return status;
    }

    ...
}

This is extendable to any type of error.  No extra setup or checking
needed by the APR app.  

Once this is in place, it takes 2-3 lines of code to save extra
diagnostic information at almost any point in APR.

iv. the APR app needs some simple trace function for APR to call

B. APR doesn't call a trace exit function, but instead builds a string
   and stores it in a widget for later retrieval

i. ptr to string in each widget; multiply this by number of widgets

struct ap_widget_t {
  ...
+  const char *diag_data; /* I guess this should be hidden behind interface
+  and a get-last-error function is provided */
  ...
};

ii. APR app must check for extra information after a call and trace it

This could end up being a very large amount of clutter.  Note that on
most systems, apr_do_something() has no extra diagnostic information
anyway.

stat = apr_do_something(&mywidget, SOME_CONST, cont);
if (stat != APR_SUCCESS)
{
  ap_log_error(..., stat, ..., "apr_do_something(): extra info: %s",
               my_widget.get_last_error_str() ? 
	       my_widget.get_last_error_str() : "(none)");
}

iii. selected APR code saves the information

ap_status_t apr_do_something(struct ap_widget_t *widget, int parm, 
		             struct context_t *cont)
{
    ...
    rc = some_kernel_call(widget->handle, parm);
    if (rc) {
        status = errno;
	widget->diag_data = ap_psprintf(cont, 
	                     "some_kernel_call() -> %d/%08X",
                             status, __errno2());
	return status;
    }

    ...
}

Good points of A:

. Once it has been made to work for one APR function, it takes very
  little effort to record diagnostic information in any other APR
  function that has context_t.  If something bad is happening*, within
  a minute or so the relevant APR function can be taught to do the
  right thing, with no changes to data structures or anything else
  that might have widespread effects.  There is no widget-by-widget
  implementation, where adding such information to functions which
  operate on widget X for the first time is more work.

. The APR app is not cluttered up with checks to see if diagnostic
  data is available and to possibly record it.  With B, the APR app
  must have such logic if diagnostic data is available on *any*
  platform.  When APR does all the work (A), the only check that is
  done is to see whether or not a trace function has been stored, and
  that check is only done when we know we have diagnostic data to
  record.

. Very little pathlength is spent when this tracing should not be
  performed.  With B, we carve up storage and fill it in even if the
  APR app doesn't care/has no logic to look for the diagnostic data
  for that particular call.  Multiple failures on the same ap_widget_t
  will use more and more of the pool.

Good points of B:

. Several folks like it.
. (fill this in)

*A or B:

I would stress the "*If something bad is happening*" part quite a
bit.  For example, I wouldn't make a pass over all APR code to do
something with the secondary error indicator if building for
OS/390.  There might be a handful of places where such information
is truly invaluable and timesaving and those will become obvious over
time; in general we don't need to clutter up any code any more than we
have to.

(I hope nobody sent me e-mail in the last couple of days.  I lost an
interesting file yesterday after some unknown power supply glitch:
~/RMAIL :( )
-- 
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