Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: domain of minfrin@sharp.fm designates
 72.32.122.47 as permitted sender)
Message-ID: <481AEF81.5040007@sharp.fm>
Date: Fri, 02 May 2008 12:40:01 +0200
From: Graham Leggett <minfrin@sharp.fm>
User-Agent: Thunderbird 2.0.0.12 (Macintosh/20080213)
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: svn commit: r647263 - in /httpd/httpd/trunk: ./ docs/manual/mod/
 include/ modules/aaa/ modules/filters/ modules/http/ server/
References: <20080411184216.4F5751A983A@eris.apache.org>
 <481A2B41.1090901@apache.org>
In-Reply-To: <481A2B41.1090901@apache.org>
Content-Type: multipart/signed; protocol="application/x-pkcs7-signature";
 micalg=sha1; boundary="------------ms030304060105000804050306"

This is a cryptographically signed message in MIME format.

--------------ms030304060105000804050306
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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;
> 
> 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
> to apr_brigade_partition.

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;
>> +                }
> 
> Why is this needed? Should this job be performed by the 
> ap_keep_body_filter that should
> be in our input filter chain if we want to keep the body?
> 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
> this is the case.

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);
>>  
> 
> This creates an error message on each subrequest if mod_request is not 
> loaded, because
> in this case the KEPT_BODY_FILTER is not registered.

You're right, let me look at this.

Regards,
Graham
--

--------------ms030304060105000804050306
Content-Type: application/x-pkcs7-signature; name="smime.p7s"
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="smime.p7s"
Content-Description: S/MIME Cryptographic Signature

MIAGCSqGSIb3DQEHAqCAMIACAQExCzAJBgUrDgMCGgUAMIAGCSqGSIb3DQEHAQAAoIIJNTCC
AvUwggJeoAMCAQICEAxljuBYvPVR3WhH6nkbTPowDQYJKoZIhvcNAQEFBQAwYjELMAkGA1UE
BhMCWkExJTAjBgNVBAoTHFRoYXd0ZSBDb25zdWx0aW5nIChQdHkpIEx0ZC4xLDAqBgNVBAMT
I1RoYXd0ZSBQZXJzb25hbCBGcmVlbWFpbCBJc3N1aW5nIENBMB4XDTA3MTAxNDEyMzQyMVoX
DTA4MTAxMzEyMzQyMVowXTEQMA4GA1UEBBMHTGVnZ2V0dDEPMA0GA1UEKhMGR3JhaGFtMRcw
FQYDVQQDEw5HcmFoYW0gTGVnZ2V0dDEfMB0GCSqGSIb3DQEJARYQbWluZnJpbkBzaGFycC5m
bTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALLUMqDEkhgiT7ePdkErBVE2tND+
6C8ElWEOaCSdbAlvwUXgKFzIR7zaD6c1CS2czmBslMwEb3LJHPWjgPN497wSERghkeAa+Fyw
WqydJr+6WE1G67wHsg67tmGPAxG0Lf6eKpsiyh+u0ojKk4n0mRZ6HQxu6PqZYzJ2vOrT4gYz
uVlz4O8TRhHOXGKqclCTxVOfEQMS3AmKDkdkNKJxgkrXSCDZ3mWs1K7yuZ6f0/30Z6AvseTF
N7CWPD7uuf5TvaVd5luOYiprUTl+u+0+CHjG4uiug54FZAyID5N7cMJy6iBRHPfLk4cU+Ksi
R99WkanPHb9wVQ2F34S+7yGdwqECAwEAAaMtMCswGwYDVR0RBBQwEoEQbWluZnJpbkBzaGFy
cC5mbTAMBgNVHRMBAf8EAjAAMA0GCSqGSIb3DQEBBQUAA4GBAB76lZ2i2DDcVPlrcDWPrGIk
mcZSoWm/7HLRvws4+jbZ++bczzUruhm1t410Z8oj95sU5pQ83SoGS3RXAn/+TX0cpgNtx4Sw
J+Nfhvey2w1TE/NLlN3n7q0m7Bm4j4+zNKXLjFj6B30Ifce8qHw7l69MSVcKoJiyd8EMM4q8
Dm+wMIIC9TCCAl6gAwIBAgIQDGWO4Fi89VHdaEfqeRtM+jANBgkqhkiG9w0BAQUFADBiMQsw
CQYDVQQGEwJaQTElMCMGA1UEChMcVGhhd3RlIENvbnN1bHRpbmcgKFB0eSkgTHRkLjEsMCoG
A1UEAxMjVGhhd3RlIFBlcnNvbmFsIEZyZWVtYWlsIElzc3VpbmcgQ0EwHhcNMDcxMDE0MTIz
NDIxWhcNMDgxMDEzMTIzNDIxWjBdMRAwDgYDVQQEEwdMZWdnZXR0MQ8wDQYDVQQqEwZHcmFo
YW0xFzAVBgNVBAMTDkdyYWhhbSBMZWdnZXR0MR8wHQYJKoZIhvcNAQkBFhBtaW5mcmluQHNo
YXJwLmZtMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAstQyoMSSGCJPt492QSsF
UTa00P7oLwSVYQ5oJJ1sCW/BReAoXMhHvNoPpzUJLZzOYGyUzARvcskc9aOA83j3vBIRGCGR
4Br4XLBarJ0mv7pYTUbrvAeyDru2YY8DEbQt/p4qmyLKH67SiMqTifSZFnodDG7o+pljMna8
6tPiBjO5WXPg7xNGEc5cYqpyUJPFU58RAxLcCYoOR2Q0onGCStdIINneZazUrvK5np/T/fRn
oC+x5MU3sJY8Pu65/lO9pV3mW45iKmtROX677T4IeMbi6K6DngVkDIgPk3twwnLqIFEc98uT
hxT4qyJH31aRqc8dv3BVDYXfhL7vIZ3CoQIDAQABoy0wKzAbBgNVHREEFDASgRBtaW5mcmlu
QHNoYXJwLmZtMAwGA1UdEwEB/wQCMAAwDQYJKoZIhvcNAQEFBQADgYEAHvqVnaLYMNxU+Wtw
NY+sYiSZxlKhab/sctG/Czj6Ntn75tzPNSu6GbW3jXRnyiP3mxTmlDzdKgZLdFcCf/5NfRym
A23HhLAn41+G97LbDVMT80uU3efurSbsGbiPj7M0pcuMWPoHfQh9x7yofDuXr0xJVwqgmLJ3
wQwzirwOb7AwggM/MIICqKADAgECAgENMA0GCSqGSIb3DQEBBQUAMIHRMQswCQYDVQQGEwJa
QTEVMBMGA1UECBMMV2VzdGVybiBDYXBlMRIwEAYDVQQHEwlDYXBlIFRvd24xGjAYBgNVBAoT
EVRoYXd0ZSBDb25zdWx0aW5nMSgwJgYDVQQLEx9DZXJ0aWZpY2F0aW9uIFNlcnZpY2VzIERp
dmlzaW9uMSQwIgYDVQQDExtUaGF3dGUgUGVyc29uYWwgRnJlZW1haWwgQ0ExKzApBgkqhkiG
9w0BCQEWHHBlcnNvbmFsLWZyZWVtYWlsQHRoYXd0ZS5jb20wHhcNMDMwNzE3MDAwMDAwWhcN
MTMwNzE2MjM1OTU5WjBiMQswCQYDVQQGEwJaQTElMCMGA1UEChMcVGhhd3RlIENvbnN1bHRp
bmcgKFB0eSkgTHRkLjEsMCoGA1UEAxMjVGhhd3RlIFBlcnNvbmFsIEZyZWVtYWlsIElzc3Vp
bmcgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMSmPFVzVftOucqZWh5owHUEcJ3f
6f+jHuy9zfVb8hp2vX8MOmHyv1HOAdTlUAow1wJjWiyJFXCO3cnwK4Vaqj9xVsuvPAsH5/Ef
kTYkKhPPK9Xzgnc9A74r/rsYPge/QIACZNenprufZdHFKlSFD0gEf6e20TxhBEAeZBlyYLf7
AgMBAAGjgZQwgZEwEgYDVR0TAQH/BAgwBgEB/wIBADBDBgNVHR8EPDA6MDigNqA0hjJodHRw
Oi8vY3JsLnRoYXd0ZS5jb20vVGhhd3RlUGVyc29uYWxGcmVlbWFpbENBLmNybDALBgNVHQ8E
BAMCAQYwKQYDVR0RBCIwIKQeMBwxGjAYBgNVBAMTEVByaXZhdGVMYWJlbDItMTM4MA0GCSqG
SIb3DQEBBQUAA4GBAEiM0VCD6gsuzA2jZqxnD3+vrL7CF6FDlpSdf0whuPg2H6otnzYvwPQc
UCCTcDz9reFhYsPZOhl+hLGZGwDFGguCdJ4lUJRix9sncVcljd2pnDmOjCBPZV+V2vf3h9bG
CE6u9uo05RAaWzVNd+NWIXiC3CEZNd4ksdMdRv9dX2VPMYIDZDCCA2ACAQEwdjBiMQswCQYD
VQQGEwJaQTElMCMGA1UEChMcVGhhd3RlIENvbnN1bHRpbmcgKFB0eSkgTHRkLjEsMCoGA1UE
AxMjVGhhd3RlIFBlcnNvbmFsIEZyZWVtYWlsIElzc3VpbmcgQ0ECEAxljuBYvPVR3WhH6nkb
TPowCQYFKw4DAhoFAKCCAcMwGAYJKoZIhvcNAQkDMQsGCSqGSIb3DQEHATAcBgkqhkiG9w0B
CQUxDxcNMDgwNTAyMTA0MDAxWjAjBgkqhkiG9w0BCQQxFgQU9oJGy1NXhAlsxe+H66xcLKvG
qnwwUgYJKoZIhvcNAQkPMUUwQzAKBggqhkiG9w0DBzAOBggqhkiG9w0DAgICAIAwDQYIKoZI
hvcNAwICAUAwBwYFKw4DAgcwDQYIKoZIhvcNAwICASgwgYUGCSsGAQQBgjcQBDF4MHYwYjEL
MAkGA1UEBhMCWkExJTAjBgNVBAoTHFRoYXd0ZSBDb25zdWx0aW5nIChQdHkpIEx0ZC4xLDAq
BgNVBAMTI1RoYXd0ZSBQZXJzb25hbCBGcmVlbWFpbCBJc3N1aW5nIENBAhAMZY7gWLz1Ud1o
R+p5G0z6MIGHBgsqhkiG9w0BCRACCzF4oHYwYjELMAkGA1UEBhMCWkExJTAjBgNVBAoTHFRo
YXd0ZSBDb25zdWx0aW5nIChQdHkpIEx0ZC4xLDAqBgNVBAMTI1RoYXd0ZSBQZXJzb25hbCBG
cmVlbWFpbCBJc3N1aW5nIENBAhAMZY7gWLz1Ud1oR+p5G0z6MA0GCSqGSIb3DQEBAQUABIIB
ADVSQkuf7kBK8NivkQn1uduQPyahGkjTmVkpBK9Q88Yhs+eCzAYo89fiacw/VZL8HhtcZzzQ
Yf6ENo0xggAW9DVoLxiGfHRBq6mgEpIXHuLulNTlkPkLVgGOB8x7PdNbo/l9Ti7swgsP9tSR
hoUrJlup7O95ZweA8F/gACOp5Kl/nYxJYrXshjU5IwnICP6U2tq6gzkS1mk6csch/wzc0xRb
L5cxjiP7tnF9XgoMxdRAaP1itZGByLuPBojnf+e/MrM5a0qB5k5jrlnSREGZyghVUmhaYE6o
HSPJgfailcAWdM4dFcSxTrxIyufZKp7oVizcITfrZ0HIGL+J/JYgvhcAAAAAAAA=
--------------ms030304060105000804050306--