Re: svn commit: r647263 - in /httpd/httpd/trunk: ./ docs/manual/mod/ include/ modules/aaa/ modules/filters/ modules/http/ server/

[Graham Leggett]

Ruediger Pluem wrote:

+    /* all is well, set aside the buckets */
+    for (bucket = APR_BRIGADE_FIRST(b);
+         bucket != APR_BRIGADE_SENTINEL(b);
+         bucket = APR_BUCKET_NEXT(bucket))
+    {
+        apr_bucket_copy(bucket, &e);

What about transient buckets? Don't we need to set them aside?

I don't follow - does the apr_bucket_copy not do that for us already?

+    ctx->remaining -= readbytes;
+    ctx->offset += readbytes;
+    return APR_SUCCESS;

to apr_brigade_partition.

Why using ctx->offset at all and not just taking all data from the kept_body brigade until readbytes, copy it over to b and remove it afterwards from the kept_body brigade. This would save one call

In theory, that would mean you could only read the kept_body once. The kept body could be delivered to multiple requests embedded within mod_include for example, and would be needed to be read more than once.

+                    c = low ^ hi;

Shouldn't this be c = low + hi ?

In theory either should work, which is faster?

+            /* If we have been asked to, keep the data up until the
+             * configured limit. If the limit is exceeded, we return an
+             * HTTP_REQUEST_ENTITY_TOO_LARGE response so the caller is
+             * clear the server couldn't handle their request.
+             */
+            if (kept_body) {
+                if (len <= left) {
+                    apr_bucket_copy(bucket, &e);
+                    APR_BRIGADE_INSERT_TAIL(kept_body, e);
+                    left -= len;
+                }
+                else {
+                    apr_brigade_destroy(bb);
+                    apr_brigade_destroy(kept_body);
+                    return HTTP_REQUEST_ENTITY_TOO_LARGE;
+                }

be in our input filter chain if we want to keep the body?

this is the case.

Why is this needed? Should this job be performed by the ap_keep_body_filter that should Of course this depends when we call ap_parse_request_form. If we call it during the authn/z phase the filter chain hasn't been setup. So maybe we should ensure that

I think the reason it is there was from when the kept body was being captured by ap_discard_request_body, which wouldn't be run if this code kicked in. However we do call it in the authn/z phase, so if the keep body filter isn't set up yet then it does still need to be here.

Why not using the insert_filter hook?

Good question, let me look.

@@ -1648,8 +1649,8 @@
      * Add the KEPT_BODY filter, which will insert any body marked to be
      * kept for the use of a subrequest, into the subrequest.
      */
-    ap_add_input_filter_handle(ap_kept_body_input_filter_handle,
-                               NULL, rnew, rnew->connection);
+    ap_add_input_filter(KEPT_BODY_FILTER,
+                        NULL, rnew, rnew->connection);

in this case the KEPT_BODY_FILTER is not registered.

This creates an error message on each subrequest if mod_request is not loaded, because

You're right, let me look at this.

Regards,
Graham
--

smime.p7s
Description: S/MIME Cryptographic Signature