error code overlap -- AP_FILTER_ERROR vs. SUSPENDED

[Eric Covener <co...@gmail.com>]

There are a couple of places, in trunk only, where the macro SUSPENDED
is checked against a handlers return code.

   1     45  include/util_filter.h <<GLOBAL>>
             #define AP_FILTER_ERROR -3

   1    460  include/httpd.h <<GLOBAL>>
             #define SUSPENDED -3

Note that AP_FILTER_ERROR can be returned by a handler that calls
ap_discard_request_body(), in the same contexts that checking against
SUSPENDED is intersting

I was looking to add an additional clause to the IGNORE_HTTP_RANGE
checking in ap_invoke_handler, but found in trunk it wasn't necessary
because of the overlap:

Index: server/config.c
===================================================================
--- server/config.c     (revision 713453)
+++ server/config.c     (working copy)
@@ -382,6 +382,7 @@
             "handler \"%s\" not found for: %s", r->handler, r->filename);
     }
     if ((result != OK) && (result != DONE) && (result != DECLINED) &&
(result != SUSPENDED)
+        && (result != AP_FILTER_ERROR)  /* no-op on trunk due to SUSPENDED */
         && !ap_is_HTTP_VALID_RESPONSE(result)) {
         /* If a module is deliberately returning something else
          * (request_rec in non-HTTP or proprietary extension?)

(ap_die ignores AP_FILTER_ERROR, but not when it's been transformed
from -3 to 500 by the code above -- LimitRequestBody with a custom
ErrorDocument is busted on 2.2.x due to this)

-- 
Eric Covener
covener@gmail.com

Re: error code overlap -- AP_FILTER_ERROR vs. SUSPENDED

[Paul Querna <ch...@force-elite.com>]

Eric Covener wrote:
> There are a couple of places, in trunk only, where the macro SUSPENDED
> is checked against a handlers return code.
> 
>    1     45  include/util_filter.h <<GLOBAL>>
>              #define AP_FILTER_ERROR -3
> 
>    1    460  include/httpd.h <<GLOBAL>>
>              #define SUSPENDED -3


Uhm, Util filter also defines:
/** Returned by the bottom-most filter if no data was written.
  *  @see ap_pass_brigade(). */
#define AP_NOBODY_WROTE         -1
/** Returned by the bottom-most filter if no data was read.
  *  @see ap_get_brigade(). */
#define AP_NOBODY_READ          -2
/** Returned when?? @bug find out when! */
#define AP_FILTER_ERROR         -3


To be honest, I don't think these are used as they should be, and this 
is also a pain since DECLINED is -1, so there was already an overlap.

If anything, change the ones in util_filter.h to different numbers, but 
better yet, remove them all, and have _one_ place for return values from 
  a handler and the filters you pass to directly to be defined.

-Paul


> Note that AP_FILTER_ERROR can be returned by a handler that calls
> ap_discard_request_body(), in the same contexts that checking against
> SUSPENDED is intersting
> 
> I was looking to add an additional clause to the IGNORE_HTTP_RANGE
> checking in ap_invoke_handler, but found in trunk it wasn't necessary
> because of the overlap:
> 
> Index: server/config.c
> ===================================================================
> --- server/config.c     (revision 713453)
> +++ server/config.c     (working copy)
> @@ -382,6 +382,7 @@
>              "handler \"%s\" not found for: %s", r->handler, r->filename);
>      }
>      if ((result != OK) && (result != DONE) && (result != DECLINED) &&
> (result != SUSPENDED)
> +        && (result != AP_FILTER_ERROR)  /* no-op on trunk due to SUSPENDED */
>          && !ap_is_HTTP_VALID_RESPONSE(result)) {
>          /* If a module is deliberately returning something else
>           * (request_rec in non-HTTP or proprietary extension?)
> 
> (ap_die ignores AP_FILTER_ERROR, but not when it's been transformed
> from -3 to 500 by the code above -- LimitRequestBody with a custom
> ErrorDocument is busted on 2.2.x due to this)
>