httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
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 09:04:40 GMT
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/

Mime
View raw message