You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by jw...@apache.org on 2001/04/25 19:07:43 UTC
cvs commit: httpd-2.0/modules/filters mod_include.h
jwoolley 01/04/25 10:07:42
Modified: modules/filters mod_include.h
Log:
Fix a reference to "ctx" in mod_include's CREATE_ERROR_BUCKET macro
to "cntx", which is the actual name of the argument to that macro. It
accidentally worked before because all of the callers of the macro happen
to be passing in a variable named "ctx". If one of them were to ever try
to pass in a context named something else, bad things would happen.
Revision Changes Path
1.18 +1 -1 httpd-2.0/modules/filters/mod_include.h
Index: mod_include.h
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.h,v
retrieving revision 1.17
retrieving revision 1.18
diff -u -d -u -r1.17 -r1.18
--- mod_include.h 2001/04/10 16:57:27 1.17
+++ mod_include.h 2001/04/25 17:07:40 1.18
@@ -175,7 +175,7 @@
{ \
apr_size_t e_wrt; \
t_buck = apr_bucket_heap_create(cntx->error_str, \
- ctx->error_length, 1, &e_wrt); \
+ cntx->error_length, 1, &e_wrt); \
APR_BUCKET_INSERT_BEFORE(h_ptr, t_buck); \
\
if (ins_head == NULL) { \
Re: cvs commit: httpd-2.0/modules/filters mod_include.h
Posted by Cliff Woolley <cl...@yahoo.com>.
On Thu, 26 Apr 2001, Greg Stein wrote:
> If the data is in a pool, then a pool bucket should be used (not an immortal
> bucket). Sure, the data has (effectively) an immortal lifetime as far as the
> request goes, but that path leads to nightmare.
>
> Immortal buckets should only be for static/const data. Use pool buckets for
> pool data.
Exactly. That's why I changed my mind.
When I originally suggested the use of an immortal bucket, it was when I
thought that the data in the bucket was one of either a static string (the
default) or a string allocated from the cmd->pool at server config time.
In either case, the data was invariant during the course of normal request
processing, meaning that the buckets code could just treat it as immortal.
But then I realized that the error message was configurable within the
document itself, meaning that the error message used is actually allocated
from the connection pool, at which point it would have to be a pool
bucket. But it doesn't save us much to make it a pool bucket (one malloc
and one memcpy, to be precise), and it adds the registration of a pool
cleanup. It would only be a real gain if the include_ctx_t also contained
a pointer to the error message's bucket, and for each error it encountered
it did an apr_bucket_copy(), which incurs a single malloc. But since it's
the error case anyway, I punted and said leave it the way it is (which is
to copy it to the heap right away).
At any rate, it must be either heap or pool. The difference is basically
just a matter of timing.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/filters mod_include.h
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Apr 25, 2001 at 08:10:27PM -0400, Cliff Woolley wrote:
>...
> Anyway, I've changed my mind. I didn't realize that the error message was
> configurable within the document itself (nice :-). The fact that it is
> means that the error_str gets copied into the include_filter_ctx structure
> (which is allocated from f->c->pool) when the context is created and
> overwritten if the document specifies its own error message. But it's one
> spot in memory that's the one used in all cases. That means that we could
> using a pool bucket (not an immortal bucket), but that would only gain us
> anything if we create a *single* pool bucket and used apr_bucket_copy().
> Not worth the complexity, I think. So I guess it's fine the way it is.
If the data is in a pool, then a pool bucket should be used (not an immortal
bucket). Sure, the data has (effectively) an immortal lifetime as far as the
request goes, but that path leads to nightmare.
Immortal buckets should only be for static/const data. Use pool buckets for
pool data.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/filters mod_include.h
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 25 Apr 2001 rbb@covalent.net wrote:
> > Yes, I agree that you can't use a transient bucket (because a transient
> > bucket means data on the stack that must be passed down right away or
> > setaside (ie, copied into a heap bucket). But you *can* use an immortal
> > bucket. Any disagreement with that statement, Ryan?
>
> I haven't reviewed patch itself, but the concepts are correct.
Patches? We don' need no steeking patches! (Sorry, couldn't resist.)
Anyway, I've changed my mind. I didn't realize that the error message was
configurable within the document itself (nice :-). The fact that it is
means that the error_str gets copied into the include_filter_ctx structure
(which is allocated from f->c->pool) when the context is created and
overwritten if the document specifies its own error message. But it's one
spot in memory that's the one used in all cases. That means that we could
using a pool bucket (not an immortal bucket), but that would only gain us
anything if we create a *single* pool bucket and used apr_bucket_copy().
Not worth the complexity, I think. So I guess it's fine the way it is.
Besides, it's the error path. =-)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/filters mod_include.h
Posted by rb...@covalent.net.
> >> parse_string(r, tag_val, parsed_string, sizeof(parsed_string), 0);
> >> if (!find_file(r, "flastmod", tag, parsed_string, &finfo)) {
> >> char *t_val;
> >>
> >> t_val = ap_ht_time(r->pool, finfo.mtime, ctx->time_str, 0);
> >> t_len = strlen(t_val);
> >>
> >> tmp_buck = ap_bucket_create_transient(t_val, t_len);
> >> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> >> if (*inserted_head == NULL) {
> >> *inserted_head = tmp_buck;
> >> }
> >>
> [snip]
> > CREATE_ERROR_BUCKET is brokecn because of the transient problem.
>
> Yes, I agree that you can't use a transient bucket (because a transient
> bucket means data on the stack that must be passed down right away or
> setaside (ie, copied into a heap bucket). But you *can* use an immortal
> bucket. Any disagreement with that statement, Ryan?
I haven't reviewed patch itself, but the concepts are correct.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0/modules/filters mod_include.h
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 25 Apr 2001, Paul J. Reder wrote:
> I believe that in an original version I used an immortal bucket here and
> had to change it for some reason, but I can't remember why. I might be
> thinking of one of the other places in the code where buckets are created
> though. Whether it was this code or some other code I seem to recall that
> the reason for the change had to do with what the code around it did, like
> maybe something to do with mixing immortal buckets with other types. If I
> recall correctly the change was recommended by Ryan.
I did a bit of digging and found it. In a message from Ryan dated
2000-11-21 1:14:58, he said this:
>> parse_string(r, tag_val, parsed_string, sizeof(parsed_string), 0);
>> if (!find_file(r, "flastmod", tag, parsed_string, &finfo)) {
>> char *t_val;
>>
>> t_val = ap_ht_time(r->pool, finfo.mtime, ctx->time_str, 0);
>> t_len = strlen(t_val);
>>
>> tmp_buck = ap_bucket_create_transient(t_val, t_len);
>> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
>> if (*inserted_head == NULL) {
>> *inserted_head = tmp_buck;
>> }
>>
[snip]
> CREATE_ERROR_BUCKET is brokecn because of the transient problem.
Yes, I agree that you can't use a transient bucket (because a transient
bucket means data on the stack that must be passed down right away or
setaside (ie, copied into a heap bucket). But you *can* use an immortal
bucket. Any disagreement with that statement, Ryan?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/filters mod_include.h
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
Cliff Woolley wrote:
> > t_buck = apr_bucket_heap_create(cntx->error_str, \
> > - ctx->error_length, 1, &e_wrt); \
> > + cntx->error_length, 1, &e_wrt); \
> > APR_BUCKET_INSERT_BEFORE(h_ptr, t_buck); \
> > \
> > if (ins_head == NULL) { \
>
> PS: This is a nit, since it's a matter of performance in the error path,
> but why on earth does the CREATE_ERROR_BUCKET macro create a copy-to-heap
> heap bucket for the error message?? The message is either a const string
> if the default is used or essentially const (allocated from cmd->pool) if
> a custom one is used. Sounds like a good place to use an immortal bucket
> if you ask me.
I believe that in an original version I used an immortal bucket here and
had to change it for some reason, but I can't remember why. I might be
thinking of one of the other places in the code where buckets are created
though. Whether it was this code or some other code I seem to recall that
the reason for the change had to do with what the code around it did, like
maybe something to do with mixing immortal buckets with other types. If I
recall correctly the change was recommended by Ryan.
Sorry I can't recall the history with more clarity.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: cvs commit: httpd-2.0/modules/filters mod_include.h
Posted by Cliff Woolley <cl...@yahoo.com>.
On 25 Apr 2001 jwoolley@apache.org wrote:
> Log:
> Fix a reference to "ctx" in mod_include's CREATE_ERROR_BUCKET macro
> to "cntx", which is the actual name of the argument to that macro. It
> accidentally worked before because all of the callers of the macro happen
> to be passing in a variable named "ctx". If one of them were to ever try
> to pass in a context named something else, bad things would happen.
>
> Revision Changes Path
> 1.18 +1 -1 httpd-2.0/modules/filters/mod_include.h
>
> Index: mod_include.h
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.h,v
> retrieving revision 1.17
> retrieving revision 1.18
> diff -u -d -u -r1.17 -r1.18
> --- mod_include.h 2001/04/10 16:57:27 1.17
> +++ mod_include.h 2001/04/25 17:07:40 1.18
> @@ -175,7 +175,7 @@
> { \
> apr_size_t e_wrt; \
> t_buck = apr_bucket_heap_create(cntx->error_str, \
> - ctx->error_length, 1, &e_wrt); \
> + cntx->error_length, 1, &e_wrt); \
> APR_BUCKET_INSERT_BEFORE(h_ptr, t_buck); \
> \
> if (ins_head == NULL) { \
PS: This is a nit, since it's a matter of performance in the error path,
but why on earth does the CREATE_ERROR_BUCKET macro create a copy-to-heap
heap bucket for the error message?? The message is either a const string
if the default is used or essentially const (allocated from cmd->pool) if
a custom one is used. Sounds like a good place to use an immortal bucket
if you ask me.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA