httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: cvs commit: httpd-2.0/modules/http config.m4 http_core.c http_protocol.c http_request.c
Date Sat, 27 Jan 2001 16:30:26 GMT

> >   +/**
> >   + * 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
-------------------------------------------------------------------------------



Mime
View raw message