You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Greg Stein <gs...@lyra.org> on 2001/01/27 10:04:40 UTC
Re: cvs commit: httpd-2.0/modules/http config.m4 http_core.c http_protocol.c http_request.c
Not bad...
On Sat, Jan 27, 2001 at 07:13:39AM -0000, rbb@apache.org wrote:
>...
> +/**
> + * A bucket referring to an HTTP error
> + * This bucket can be passed down the filter stack to indicate that an
> + * HTTP error occurred while running a filter. In order for this bucket
> + * to be used successfully, it MUST be sent as the first bucket in the
> + * first brigade to be sent from a given filter.
> + */
> +struct ap_bucket_error {
> + /** The start of the data actually allocated. This should never be
> + * modified, it is only used to free the bucket.
> + */
> + char *start;
> +};
Shouldn't this have an "int" for the HTTP status, and an (optional) "const
char *" for any message? And will the message be the body, or the status
line?
Adding the fields will prevent the getword/atoi stuff when you process the
bucket.
>...
> --- config.m4 2000/12/05 03:51:41 1.1
> +++ config.m4 2001/01/27 07:13:39 1.2
> @@ -2,7 +2,7 @@
>
> APACHE_MODPATH_INIT(http)
>
> -http_objects="http_core.lo http_protocol.lo http_request.lo"
> +http_objects="http_core.lo http_protocol.lo http_request.lo error_bucket.lo"
You didn't "cvs add" error_bucket.c ... :-(
>...
> --- http_protocol.c 2001/01/26 18:48:57 1.282
> +++ http_protocol.c 2001/01/27 07:13:39 1.283
> @@ -2466,6 +2466,14 @@
> return OK;
> }
>
> + if (APR_BRIGADE_FIRST(b)->type == &ap_bucket_type_error) {
> + const char *str;
> + apr_size_t length;
> + apr_bucket_read(APR_BRIGADE_FIRST(b), &str, &length, APR_NONBLOCK_READ);
> + ap_die(atoi(ap_getword_white(r->pool, &str)), r);
> + return AP_FILTER_ERROR;
> + }
We need to have a scheme that doesn't depend on the error bucket being
first. It is entirely reasonable for a filter to simply prepend something to
the brigade and pass it along. If we continue to force this to be the head,
then *every* filter is going to start with this prologue:
if (APR_BRIGADE_FIRST(b)->type == &ap_bucket_type_error)
return ap_pass_brigade(f->next, b);
That is going to be icky in the long run.
It will be simpler if the catching filter can go ahead and process buckets,
run into an error bucket, and then go "woah! oops! got an error!"
Also, you check the type of the bucket, then fall back to the bucket read
code and do the atoi/getword stuff. You *know* it is an error bucket. Deref
the thing and yank out the values you need. No need for apr_bucket_read.
>...
> --- http_request.c 2001/01/24 02:14:23 1.77
> +++ http_request.c 2001/01/27 07:13:39 1.78
> @@ -1351,7 +1351,7 @@
> */
> ap_run_insert_filter(r);
>
> - if ((access_status = ap_invoke_handler(r)) != 0) {
> + if ((access_status = ap_invoke_handler(r)) != 0 && access_status != -3) {
> ap_die(access_status, r);
> return;
> }
Since ap_die() was called in the filter code, we want to skip that above.
However, we still need to do the return; otherwise,
ap_finalize_request_protocol() gets called twice.
I'd recommend moving the access_status check into ap_die() itself (to
prevent *any* potential problem with somebody calling it).
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/http config.m4 http_core.c
http_protocol.c http_request.c
Posted by rb...@covalent.net.
> > > Adding the fields will prevent the getword/atoi stuff when you process the
> > > bucket.
> >
> > I thought of this, but remember that all of our buckets need to return
> > const char * from the read function. Since we have to return a const char
> > *, I decided to just store things in that format. If you have another way
> > to do this, I'd love to see it.
>
> True. But this guy *is* a "meta" bucket -- it has no content. So it probably
> ought to be like EOS or FLUSH -- b->length==0 and return "" from the read.
> Does that sound right?
Yeah. Good call there. Feel free to make that change, because I am
getting away from the computer until about 5:00 tonight.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0/modules/http config.m4 http_core.c http_protocol.c http_request.c
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Jan 27, 2001 at 08:30:26AM -0800, rbb@covalent.net wrote:
>...
> > Shouldn't this have an "int" for the HTTP status, and an (optional) "const
> > char *" for any message? And will the message be the body, or the status
> > line?
> >
> > Adding the fields will prevent the getword/atoi stuff when you process the
> > bucket.
>
> I thought of this, but remember that all of our buckets need to return
> const char * from the read function. Since we have to return a const char
> *, I decided to just store things in that format. If you have another way
> to do this, I'd love to see it.
True. But this guy *is* a "meta" bucket -- it has no content. So it probably
ought to be like EOS or FLUSH -- b->length==0 and return "" from the read.
Does that sound right?
>...
> Good call. patch coming.
Great!
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/http config.m4 http_core.c
http_protocol.c http_request.c
Posted by rb...@covalent.net.
> > +/**
> > + * A bucket referring to an HTTP error
> > + * This bucket can be passed down the filter stack to indicate that an
> > + * HTTP error occurred while running a filter. In order for this bucket
> > + * to be used successfully, it MUST be sent as the first bucket in the
> > + * first brigade to be sent from a given filter.
> > + */
> > +struct ap_bucket_error {
> > + /** The start of the data actually allocated. This should never be
> > + * modified, it is only used to free the bucket.
> > + */
> > + char *start;
> > +};
>
> Shouldn't this have an "int" for the HTTP status, and an (optional) "const
> char *" for any message? And will the message be the body, or the status
> line?
>
> Adding the fields will prevent the getword/atoi stuff when you process the
> bucket.
I thought of this, but remember that all of our buckets need to return
const char * from the read function. Since we have to return a const char
*, I decided to just store things in that format. If you have another way
to do this, I'd love to see it.
The message right now is not used. I think it can be used as an error log
message though. The error page is done by ap_die.
> You didn't "cvs add" error_bucket.c ... :-(
Yep, my bad. Done now. Sorry.
> > + if (APR_BRIGADE_FIRST(b)->type == &ap_bucket_type_error) {
> > + const char *str;
> > + apr_size_t length;
> > + apr_bucket_read(APR_BRIGADE_FIRST(b), &str, &length, APR_NONBLOCK_READ);
> > + ap_die(atoi(ap_getword_white(r->pool, &str)), r);
> > + return AP_FILTER_ERROR;
> > + }
>
> We need to have a scheme that doesn't depend on the error bucket being
> first. It is entirely reasonable for a filter to simply prepend something to
> the brigade and pass it along. If we continue to force this to be the head,
> then *every* filter is going to start with this prologue:
This would be easy to change. I'll do it today.
> It will be simpler if the catching filter can go ahead and process buckets,
> run into an error bucket, and then go "woah! oops! got an error!"
>
> Also, you check the type of the bucket, then fall back to the bucket read
> code and do the atoi/getword stuff. You *know* it is an error bucket. Deref
> the thing and yank out the values you need. No need for apr_bucket_read.
Alright. I was trying to avoid that, but okay. Ignore my comments above
then.
>
> > --- http_request.c 2001/01/24 02:14:23 1.77
> > +++ http_request.c 2001/01/27 07:13:39 1.78
> > @@ -1351,7 +1351,7 @@
> > */
> > ap_run_insert_filter(r);
> >
> > - if ((access_status = ap_invoke_handler(r)) != 0) {
> > + if ((access_status = ap_invoke_handler(r)) != 0 && access_status != -3) {
> > ap_die(access_status, r);
> > return;
> > }
>
> Since ap_die() was called in the filter code, we want to skip that above.
> However, we still need to do the return; otherwise,
> ap_finalize_request_protocol() gets called twice.
>
> I'd recommend moving the access_status check into ap_die() itself (to
> prevent *any* potential problem with somebody calling it).
Good call. patch coming.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------