Thanks for the review. Comments below.
> Am 08.10.2015 um 13:26 schrieb Yann Ylavic <ylavic....@gmail.com>:
>
> On Thu, Oct 8, 2015 at 1:05 PM, Stefan Eissing
> <stefan.eiss...@greenbytes.de> wrote:
>> Yann, care to apply your eyeballs to this one? Old one is reverted. Thanks!
>>
>> Index: modules/http2/h2_util.c
>> ===================================================================
>> --- modules/http2/h2_util.c (revision 1707479)
>> +++ modules/http2/h2_util.c (working copy)
>> @@ -539,14 +539,36 @@
>> apr_size_t *plen, int *peos)
>> {
>> apr_status_t status;
>> + apr_off_t blen = 0;
>> +
>> /* test read to determine available length */
>> - apr_off_t blen = 0;
>> - status = apr_brigade_length(bb, 0, &blen);
>> - if (blen < (apr_off_t)*plen) {
>> - *plen = blen;
>> + status = apr_brigade_length(bb, 1, &blen);
>> + if (status != APR_SUCCESS) {
>> + return status;
>> }
>> - *peos = h2_util_has_eos(bb, *plen);
>> - return status;
>> + else if (blen == 0) {
>> + /* empty brigade, does it have an EOS bucket somwhere? */
>> + *plen = 0;
>> + *peos = h2_util_has_eos(bb, 0);
>> + }
>> + else if (blen > 0) {
>> + /* data in the brigade, limit the length returned. Check for EOS
>> + * bucket only if we indicate data. This is required since plen == 0
>> + * means "the whole brigade" for h2_util_hash_eos()
>> + */
>> + if (blen < (apr_off_t)*plen) {
>> + *plen = blen;
>> + }
>> + *peos = (*plen > 0)? h2_util_has_eos(bb, *plen) : 0;
>
> Maybe last_not_included() could "eat" leading metadata buckets when
> called with maxlen == 0, that is remove the leading "if (maxlen > 0)"
> in there.
> Hence we could call h2_util_has_eos(bb, *plen) unconditionally here
> and still still detect EOS.
>
> Can't tell about the other uses of ast_not_included() though...
Often, mod_http2 needs to transfer a max amount of data. Sometimes
it "moves" buckets from one brigade to another. Sometimes, it just
needs to detect how much data *can* be moved.
last_not_included is used to determine the bucket in a brigade that
will not be moved with the length limit given.
Since the brigade is already being inspected, it is useful to find
out if a read would already encounter an EOS. This prevents the transfer
from being invoked again. Since these transfer often require mutex locks, less
is better.
For the length limit, just data bucket lengths are counted. Meta buckets
count as zero length. So, with a brigade
(1)data[1024] -> (2)data[730] -> (3)EOS -> SENTINEL
last_not_included(100) would be bucket 2.
last_not_included(1024) would be bucket 2.
last_not_included(1025) would be bucket 3.
last_not_included(2048) would be bucket SENTINEL.
//Stefan
>> + }
>> + else if (blen < 0) {
>> + /* famous SHOULD NOT HAPPEN, sinc we told apr_brigade_length to
>> readall
>> + */
>> + *plen = 0;
>> + *peos = h2_util_has_eos(bb, 0);
>> + return APR_EINVAL;
>> + }
>
> This seems not needed, maybe change (blen == 0) to (blen <= 0) above
> to protect from stray photons?
>
>
>> + return APR_SUCCESS;
>> }
