httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject what are the issues? (was: Re: Patch review: ...)
Date Tue, 27 Jun 2000 10:39:45 GMT
On Mon, Jun 26, 2000 at 09:39:43PM -0700, rbb@covalent.net wrote:
>...
> We are definately making more progress.

Agreed.

> I am beginning to think though
> that we aren't making much more progress.  I need to step back from this
> for a while.  I am -1 for putting ANY patch in at all right now.

I can wait, but I don't have a clear understanding on your issues with my
patch. I also don't want to see us try to solve ALL the filtering problems
in one fell swoop. We should take a step at a time.

> I am
> also beginning to think that we may need to re-implement some pool
> functions in order to do this at all properly.

*blink*  Eh? I don't see this, but I'm sure you will demonstrate :-)

I'll also say that I believe my patches empirically show that pool changes
aren't needed to move us that first step.

>...
> I am -1 for using Greg's patch for all the reasons I dislike my patch.

I have three review points from you so far, each are soluable. I would like
to hear from you whether these don't seem solved, or whether you see other
problems:

1) need to delay issuing headers (the r->headers_sent thing in your patch)

   SOLUTION: Simple addition, solved in my scheme the same way you did. I
             deferred simply because I knew it was disjoint and did not have
             to be solved in this first step.

   RATIONALE: When a content-generator calls ap_rwrite(), they have already
              given up hope on modifying the headers. We will not be
              changing the status quo.

	      What it *does* mean, however, is that the filters themselves
	      cannot change the headers during their content processing
	      (they certainly can at other points in the request
	      processing). I consider this an additional feature, for a
	      later step.

2) Should not use char* callbacks.

   SOLUTION: I disagree this is a problem. It is a stylistic issue, but has
             no technical problems that are not present with buckets or
             ioblock -based callbacks.

   RATIONALE: If a filter callback is passed a bucket/ioblock and it needs a
	      char* to examine the actual content, then it must call a
	      utility function right away to get that char*:
	      
	      my_callback(bucket):
	          str = give_me_string(bucket)
		  process_string(str)

              The simple callback form alters this to:
	      
	      str = give_me_string(bucket)
	      my_callback(str):
	          process_string(str)


              By doing the "give me a string" in the framework, we have more
	      opportunity to optimize the string's construction and
	      handling. For example, create and destroy a pool to hold the
	      string while the callback is invoked. In the "build the string
	      inside" approach, the callback would look like:
	      
	      my_callback(bucket):
	          pool = create_pool(bucket->r->pool)
		  str = give_me_string(bucket)
		  process_string_str)
		  destroy_pool(pool)

              or:
	      
	      my_callback(bucket):
		  str = give_me_string(bucket, &pool)
		  process_string_str)
		  destroy_pool(pool)

              For the simple callback, the pool handling occurs *outside*
	      the callback. It remains just as simple as above. The point
	      here is that we have optimization choices that will not affect
	      the simple_cb style of filter.

3) ordering issues

   SOLUTION: Per a previous email, I added a condition to the scan loop in
	     ap_add_filter(). I had guessed that would eventually be added,
	     but wanted to defer it until it was found to be really
	     necessary.

   RATIONALE: not needed :-) ... this has been addressed explicitly.


Other problems?

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message