You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Eissing <st...@greenbytes.de> on 2015/10/08 12:06:04 UTC
stop the press!
I need a tiny patch for mod_http2 that solves an empty response problem with the latest nghttp2 libraries:
Jim, can we get that still in? Commited in trunk as r1707468.
//Stefan
Index: modules/http2/h2_util.c
===================================================================
--- modules/http2/h2_util.c (revision 1707467)
+++ modules/http2/h2_util.c (revision 1707468)
@@ -484,20 +484,39 @@
int h2_util_has_eos(apr_bucket_brigade *bb, apr_size_t len)
{
- apr_bucket *b, *end;
+ apr_bucket *b;
- apr_status_t status = last_not_included(bb, len, 0, 0, &end);
- if (status != APR_SUCCESS) {
- return status;
+ if (len == 0) {
+ /* special case: this is only true, if there are only meta
+ * and an eos bucket in the brigade head.
+ */
+ for (b = APR_BRIGADE_FIRST(bb);
+ b != APR_BRIGADE_SENTINEL(bb);
+ b = APR_BUCKET_NEXT(b))
+ {
+ if (!APR_BUCKET_IS_METADATA(b) && b->length != 0) {
+ break;
+ }
+ else if (APR_BUCKET_IS_EOS(b)) {
+ return 1;
+ }
+ }
}
-
- for (b = APR_BRIGADE_FIRST(bb);
- b != APR_BRIGADE_SENTINEL(bb) && b != end;
- b = APR_BUCKET_NEXT(b))
- {
- if (APR_BUCKET_IS_EOS(b)) {
- return 1;
+ else {
+ apr_bucket *end;
+ apr_status_t status = last_not_included(bb, len, 0, 0, &end);
+ if (status != APR_SUCCESS) {
+ return status;
}
+
+ for (b = APR_BRIGADE_FIRST(bb);
+ b != APR_BRIGADE_SENTINEL(bb) && b != end;
+ b = APR_BUCKET_NEXT(b))
+ {
+ if (APR_BUCKET_IS_EOS(b)) {
+ return 1;
+ }
+ }
}
return 0;
}
Index: modules/http2/h2_version.h
===================================================================
--- modules/http2/h2_version.h (revision 1707467)
+++ modules/http2/h2_version.h (revision 1707468)
@@ -20,7 +20,7 @@
* @macro
* Version number of the h2 module as c string
*/
-#define MOD_HTTP2_VERSION "0.9.9"
+#define MOD_HTTP2_VERSION "1.0.0"
/**
* @macro
@@ -28,7 +28,7 @@
* release. This is a 24 bit number with 8 bits for major number, 8 bits
* for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
*/
-#define MOD_HTTP2_VERSION_NUM 0x000909
+#define MOD_HTTP2_VERSION_NUM 0x010000
#endif /* mod_h2_h2_version_h */
Re: stop the press!
Posted by Yann Ylavic <yl...@gmail.com>.
OK, fair enough.
On Thu, Oct 8, 2015 at 12:16 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
> Valid question. The failure can only happen when a bucket of indeterminate length fails the read.
>
> I think the prototype of h2_util_has_eos() needs to change to return arp_status_t and have an int* for the flag. But that change I do not want to do right now for the 2.4.17...
>
> //Stefan
>
>> Am 08.10.2015 um 12:11 schrieb Yann Ylavic <yl...@gmail.com>:
>>
>> Hi Stefan,
>>
>> On Thu, Oct 8, 2015 at 12:06 PM, Stefan Eissing
>> <st...@greenbytes.de> wrote:
>>>
>>> Index: modules/http2/h2_util.c
>>> ===================================================================
>>> --- modules/http2/h2_util.c (revision 1707467)
>>> +++ modules/http2/h2_util.c (revision 1707468)
>>> @@ -484,20 +484,39 @@
>>>
>>> int h2_util_has_eos(apr_bucket_brigade *bb, apr_size_t len)
>>> {
>>> - apr_bucket *b, *end;
>>> + apr_bucket *b;
>>>
>>> - apr_status_t status = last_not_included(bb, len, 0, 0, &end);
>>> - if (status != APR_SUCCESS) {
>>> - return status;
>>> + if (len == 0) {
>>> + /* special case: this is only true, if there are only meta
>>> + * and an eos bucket in the brigade head.
>>> + */
>>> + for (b = APR_BRIGADE_FIRST(bb);
>>> + b != APR_BRIGADE_SENTINEL(bb);
>>> + b = APR_BUCKET_NEXT(b))
>>> + {
>>> + if (!APR_BUCKET_IS_METADATA(b) && b->length != 0) {
>>> + break;
>>> + }
>>> + else if (APR_BUCKET_IS_EOS(b)) {
>>> + return 1;
>>> + }
>>> + }
>>> }
>>> -
>>> - for (b = APR_BRIGADE_FIRST(bb);
>>> - b != APR_BRIGADE_SENTINEL(bb) && b != end;
>>> - b = APR_BUCKET_NEXT(b))
>>> - {
>>> - if (APR_BUCKET_IS_EOS(b)) {
>>> - return 1;
>>> + else {
>>> + apr_bucket *end;
>>> + apr_status_t status = last_not_included(bb, len, 0, 0, &end);
>>> + if (status != APR_SUCCESS) {
>>> + return status;
>>
>> Do we want to assume EOS when last_not_included() fails?
>>
>>> }
>>> +
>>> + for (b = APR_BRIGADE_FIRST(bb);
>>> + b != APR_BRIGADE_SENTINEL(bb) && b != end;
>>> + b = APR_BUCKET_NEXT(b))
>>> + {
>>> + if (APR_BUCKET_IS_EOS(b)) {
>>> + return 1;
>>> + }
>>> + }
>>> }
>>> return 0;
>>> }
>>
>> Regards,
>> Yann.
>
Re: stop the press!
Posted by Stefan Eissing <st...@greenbytes.de>.
Valid question. The failure can only happen when a bucket of indeterminate length fails the read.
I think the prototype of h2_util_has_eos() needs to change to return arp_status_t and have an int* for the flag. But that change I do not want to do right now for the 2.4.17...
//Stefan
> Am 08.10.2015 um 12:11 schrieb Yann Ylavic <yl...@gmail.com>:
>
> Hi Stefan,
>
> On Thu, Oct 8, 2015 at 12:06 PM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>> Index: modules/http2/h2_util.c
>> ===================================================================
>> --- modules/http2/h2_util.c (revision 1707467)
>> +++ modules/http2/h2_util.c (revision 1707468)
>> @@ -484,20 +484,39 @@
>>
>> int h2_util_has_eos(apr_bucket_brigade *bb, apr_size_t len)
>> {
>> - apr_bucket *b, *end;
>> + apr_bucket *b;
>>
>> - apr_status_t status = last_not_included(bb, len, 0, 0, &end);
>> - if (status != APR_SUCCESS) {
>> - return status;
>> + if (len == 0) {
>> + /* special case: this is only true, if there are only meta
>> + * and an eos bucket in the brigade head.
>> + */
>> + for (b = APR_BRIGADE_FIRST(bb);
>> + b != APR_BRIGADE_SENTINEL(bb);
>> + b = APR_BUCKET_NEXT(b))
>> + {
>> + if (!APR_BUCKET_IS_METADATA(b) && b->length != 0) {
>> + break;
>> + }
>> + else if (APR_BUCKET_IS_EOS(b)) {
>> + return 1;
>> + }
>> + }
>> }
>> -
>> - for (b = APR_BRIGADE_FIRST(bb);
>> - b != APR_BRIGADE_SENTINEL(bb) && b != end;
>> - b = APR_BUCKET_NEXT(b))
>> - {
>> - if (APR_BUCKET_IS_EOS(b)) {
>> - return 1;
>> + else {
>> + apr_bucket *end;
>> + apr_status_t status = last_not_included(bb, len, 0, 0, &end);
>> + if (status != APR_SUCCESS) {
>> + return status;
>
> Do we want to assume EOS when last_not_included() fails?
>
>> }
>> +
>> + for (b = APR_BRIGADE_FIRST(bb);
>> + b != APR_BRIGADE_SENTINEL(bb) && b != end;
>> + b = APR_BUCKET_NEXT(b))
>> + {
>> + if (APR_BUCKET_IS_EOS(b)) {
>> + return 1;
>> + }
>> + }
>> }
>> return 0;
>> }
>
> Regards,
> Yann.
Re: stop the press!
Posted by Yann Ylavic <yl...@gmail.com>.
Hi Stefan,
On Thu, Oct 8, 2015 at 12:06 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> Index: modules/http2/h2_util.c
> ===================================================================
> --- modules/http2/h2_util.c (revision 1707467)
> +++ modules/http2/h2_util.c (revision 1707468)
> @@ -484,20 +484,39 @@
>
> int h2_util_has_eos(apr_bucket_brigade *bb, apr_size_t len)
> {
> - apr_bucket *b, *end;
> + apr_bucket *b;
>
> - apr_status_t status = last_not_included(bb, len, 0, 0, &end);
> - if (status != APR_SUCCESS) {
> - return status;
> + if (len == 0) {
> + /* special case: this is only true, if there are only meta
> + * and an eos bucket in the brigade head.
> + */
> + for (b = APR_BRIGADE_FIRST(bb);
> + b != APR_BRIGADE_SENTINEL(bb);
> + b = APR_BUCKET_NEXT(b))
> + {
> + if (!APR_BUCKET_IS_METADATA(b) && b->length != 0) {
> + break;
> + }
> + else if (APR_BUCKET_IS_EOS(b)) {
> + return 1;
> + }
> + }
> }
> -
> - for (b = APR_BRIGADE_FIRST(bb);
> - b != APR_BRIGADE_SENTINEL(bb) && b != end;
> - b = APR_BUCKET_NEXT(b))
> - {
> - if (APR_BUCKET_IS_EOS(b)) {
> - return 1;
> + else {
> + apr_bucket *end;
> + apr_status_t status = last_not_included(bb, len, 0, 0, &end);
> + if (status != APR_SUCCESS) {
> + return status;
Do we want to assume EOS when last_not_included() fails?
> }
> +
> + for (b = APR_BRIGADE_FIRST(bb);
> + b != APR_BRIGADE_SENTINEL(bb) && b != end;
> + b = APR_BUCKET_NEXT(b))
> + {
> + if (APR_BUCKET_IS_EOS(b)) {
> + return 1;
> + }
> + }
> }
> return 0;
> }
Regards,
Yann.
Re: stop the press!
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 8, 2015 at 1:26 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Maybe last_not_included() could "eat" leading metadata buckets when
> called with maxlen == 0, that is remove the leading "if (maxlen > 0)"
> in there.
Possibly the original code with this only change would work too...
Re: stop the press!
Posted by Stefan Eissing <st...@greenbytes.de>.
Yes, that's what I realized after your first review.
I think the currently applied patch works fine and any improvement is to much for 2.4.17. Time to revisit when Graham rips it all apart... ;-)
//Stefan
> Am 08.10.2015 um 14:36 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Thu, Oct 8, 2015 at 2:12 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Oct 8, 2015 at 1:56 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> So we can change that later if necessary
>>
>> Something like this maybe:
>
> Hm, no, this changes "the whole brigade" with *plen == 0 semantic in
> h2_util_has_eos(), which other callers may not expect.
Re: stop the press!
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 8, 2015 at 2:12 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Oct 8, 2015 at 1:56 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> So we can change that later if necessary
>
> Something like this maybe:
Hm, no, this changes "the whole brigade" with *plen == 0 semantic in
h2_util_has_eos(), which other callers may not expect.
Re: stop the press!
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 8, 2015 at 1:56 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> So we can change that later if necessary
Something like this maybe:
Index: modules/http2/h2_util.c
===================================================================
--- modules/http2/h2_util.c (revision 1707505)
+++ modules/http2/h2_util.c (working copy)
@@ -215,7 +215,7 @@ static const int FILE_MOVE = 1;
static apr_status_t last_not_included(apr_bucket_brigade *bb,
apr_size_t maxlen,
- int same_alloc,
+ int same_alloc, int meta_only,
int *pfile_buckets_allowed,
apr_bucket **pend)
{
@@ -223,7 +223,7 @@ static apr_status_t last_not_included(apr_bucket_b
apr_status_t status = APR_SUCCESS;
int files_allowed = pfile_buckets_allowed? *pfile_buckets_allowed : 0;
- if (maxlen > 0) {
+ if (maxlen > 0 || meta_only) {
/* Find the bucket, up to which we reach maxlen/mem bytes */
for (b = APR_BRIGADE_FIRST(bb);
(b != APR_BRIGADE_SENTINEL(bb));
@@ -291,7 +291,7 @@ apr_status_t h2_util_move(apr_bucket_brigade *to,
if (!APR_BRIGADE_EMPTY(from)) {
apr_bucket *b, *end;
- status = last_not_included(from, maxlen, same_alloc,
+ status = last_not_included(from, maxlen, same_alloc, 0,
pfile_handles_allowed, &end);
if (status != APR_SUCCESS) {
return status;
@@ -419,7 +419,7 @@ apr_status_t h2_util_copy(apr_bucket_brigade *to,
if (!APR_BRIGADE_EMPTY(from)) {
apr_bucket *b, *end, *cpy;
- status = last_not_included(from, maxlen, 0, 0, &end);
+ status = last_not_included(from, maxlen, 0, 0, 0, &end);
if (status != APR_SUCCESS) {
return status;
}
@@ -486,7 +486,7 @@ int h2_util_has_eos(apr_bucket_brigade *bb, apr_si
{
apr_bucket *b, *end;
- apr_status_t status = last_not_included(bb, len, 0, 0, &end);
+ apr_status_t status = last_not_included(bb, len, 0, 1, 0, &end);
if (status != APR_SUCCESS) {
return status;
}
@@ -546,28 +546,11 @@ apr_status_t h2_util_bb_avail(apr_bucket_brigade *
if (status != APR_SUCCESS) {
return status;
}
- else if (blen == 0) {
- /* empty brigade, does it have an EOS bucket somwhere? */
- *plen = 0;
- *peos = h2_util_has_eos(bb, 0);
+
+ if (blen < (apr_off_t)*plen) {
+ *plen = blen;
}
- 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;
- }
- 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;
- }
+ *peos = h2_util_has_eos(bb, *plen);
return APR_SUCCESS;
}
Re: stop the press!
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 8, 2015 at 1:38 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>>
>> 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.
OK, the change I proposed was only for simplification, your patch
already handles that anyway.
So we can change that later if necessary, the code looks good to me now.
Thanks Stefan.
Re: stop the press!
Posted by Stefan Eissing <st...@greenbytes.de>.
Thanks for the review. Comments below.
> Am 08.10.2015 um 13:26 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Thu, Oct 8, 2015 at 1:05 PM, Stefan Eissing
> <st...@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;
>> }
Re: stop the press!
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 8, 2015 at 1:05 PM, Stefan Eissing
<st...@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...
> + }
> + 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;
> }
Re: stop the press!
Posted by Stefan Eissing <st...@greenbytes.de>.
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;
+ }
+ 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;
+ }
+ return APR_SUCCESS;
}
apr_status_t h2_util_bb_readx(apr_bucket_brigade *bb,
Index: modules/http2/h2_version.h
===================================================================
--- modules/http2/h2_version.h (revision 1707479)
+++ modules/http2/h2_version.h (working copy)
@@ -20,7 +20,7 @@
* @macro
* Version number of the h2 module as c string
*/
-#define MOD_HTTP2_VERSION "0.9.9"
+#define MOD_HTTP2_VERSION "1.0.0"
/**
* @macro
@@ -28,7 +28,7 @@
* release. This is a 24 bit number with 8 bits for major number, 8 bits
* for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
*/
-#define MOD_HTTP2_VERSION_NUM 0x000909
+#define MOD_HTTP2_VERSION_NUM 0x010000
> Am 08.10.2015 um 12:30 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Thu, Oct 8, 2015 at 12:06 PM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>> I need a tiny patch for mod_http2 that solves an empty response problem with the latest nghttp2 libraries:
>>
>> Jim, can we get that still in? Commited in trunk as r1707468.
>
> I quickly ask (since I did not check all the h2_util_has_eos(bb, 0)
> callers paths and you may know that already), what happens to the EOS
> bucket it is in the brigade along with other non-empty buckets and
> hence the callers consider we are not EOS?
> Does it get stripped somehow or the next run will find it?
Re: stop the press!
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 8, 2015 at 12:06 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
> I need a tiny patch for mod_http2 that solves an empty response problem with the latest nghttp2 libraries:
>
> Jim, can we get that still in? Commited in trunk as r1707468.
I quickly ask (since I did not check all the h2_util_has_eos(bb, 0)
callers paths and you may know that already), what happens to the EOS
bucket it is in the brigade along with other non-empty buckets and
hence the callers consider we are not EOS?
Does it get stripped somehow or the next run will find it?
Re: stop the press!
Posted by Stefan Eissing <st...@greenbytes.de>.
Ah, good and useful hint! Currently working on a better patch and backport that myself then. Thanks!
> Am 08.10.2015 um 12:46 schrieb Jim Jagielski <ji...@jaguNET.com>:
>
> The mod_http2 stuff for 2.4.x is CTR, meaning that one does not
> need to propose a backport in STATUS for it to be included; rather,
> one simply patches 2.4 as required, while ensuring that one doesn't
> break anything :)
>
>> On Oct 8, 2015, at 6:06 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>
>> I need a tiny patch for mod_http2 that solves an empty response problem with the latest nghttp2 libraries:
>>
>> Jim, can we get that still in? Commited in trunk as r1707468.
>>
>> //Stefan
>>
>> Index: modules/http2/h2_util.c
>> ===================================================================
>> --- modules/http2/h2_util.c (revision 1707467)
>> +++ modules/http2/h2_util.c (revision 1707468)
>> @@ -484,20 +484,39 @@
>>
>> int h2_util_has_eos(apr_bucket_brigade *bb, apr_size_t len)
>> {
>> - apr_bucket *b, *end;
>> + apr_bucket *b;
>>
>> - apr_status_t status = last_not_included(bb, len, 0, 0, &end);
>> - if (status != APR_SUCCESS) {
>> - return status;
>> + if (len == 0) {
>> + /* special case: this is only true, if there are only meta
>> + * and an eos bucket in the brigade head.
>> + */
>> + for (b = APR_BRIGADE_FIRST(bb);
>> + b != APR_BRIGADE_SENTINEL(bb);
>> + b = APR_BUCKET_NEXT(b))
>> + {
>> + if (!APR_BUCKET_IS_METADATA(b) && b->length != 0) {
>> + break;
>> + }
>> + else if (APR_BUCKET_IS_EOS(b)) {
>> + return 1;
>> + }
>> + }
>> }
>> -
>> - for (b = APR_BRIGADE_FIRST(bb);
>> - b != APR_BRIGADE_SENTINEL(bb) && b != end;
>> - b = APR_BUCKET_NEXT(b))
>> - {
>> - if (APR_BUCKET_IS_EOS(b)) {
>> - return 1;
>> + else {
>> + apr_bucket *end;
>> + apr_status_t status = last_not_included(bb, len, 0, 0, &end);
>> + if (status != APR_SUCCESS) {
>> + return status;
>> }
>> +
>> + for (b = APR_BRIGADE_FIRST(bb);
>> + b != APR_BRIGADE_SENTINEL(bb) && b != end;
>> + b = APR_BUCKET_NEXT(b))
>> + {
>> + if (APR_BUCKET_IS_EOS(b)) {
>> + return 1;
>> + }
>> + }
>> }
>> return 0;
>> }
>> Index: modules/http2/h2_version.h
>> ===================================================================
>> --- modules/http2/h2_version.h (revision 1707467)
>> +++ modules/http2/h2_version.h (revision 1707468)
>> @@ -20,7 +20,7 @@
>> * @macro
>> * Version number of the h2 module as c string
>> */
>> -#define MOD_HTTP2_VERSION "0.9.9"
>> +#define MOD_HTTP2_VERSION "1.0.0"
>>
>> /**
>> * @macro
>> @@ -28,7 +28,7 @@
>> * release. This is a 24 bit number with 8 bits for major number, 8 bits
>> * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
>> */
>> -#define MOD_HTTP2_VERSION_NUM 0x000909
>> +#define MOD_HTTP2_VERSION_NUM 0x010000
>>
>>
>> #endif /* mod_h2_h2_version_h */
>>
>
Re: stop the press!
Posted by Jim Jagielski <ji...@jaguNET.com>.
The mod_http2 stuff for 2.4.x is CTR, meaning that one does not
need to propose a backport in STATUS for it to be included; rather,
one simply patches 2.4 as required, while ensuring that one doesn't
break anything :)
> On Oct 8, 2015, at 6:06 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>
> I need a tiny patch for mod_http2 that solves an empty response problem with the latest nghttp2 libraries:
>
> Jim, can we get that still in? Commited in trunk as r1707468.
>
> //Stefan
>
> Index: modules/http2/h2_util.c
> ===================================================================
> --- modules/http2/h2_util.c (revision 1707467)
> +++ modules/http2/h2_util.c (revision 1707468)
> @@ -484,20 +484,39 @@
>
> int h2_util_has_eos(apr_bucket_brigade *bb, apr_size_t len)
> {
> - apr_bucket *b, *end;
> + apr_bucket *b;
>
> - apr_status_t status = last_not_included(bb, len, 0, 0, &end);
> - if (status != APR_SUCCESS) {
> - return status;
> + if (len == 0) {
> + /* special case: this is only true, if there are only meta
> + * and an eos bucket in the brigade head.
> + */
> + for (b = APR_BRIGADE_FIRST(bb);
> + b != APR_BRIGADE_SENTINEL(bb);
> + b = APR_BUCKET_NEXT(b))
> + {
> + if (!APR_BUCKET_IS_METADATA(b) && b->length != 0) {
> + break;
> + }
> + else if (APR_BUCKET_IS_EOS(b)) {
> + return 1;
> + }
> + }
> }
> -
> - for (b = APR_BRIGADE_FIRST(bb);
> - b != APR_BRIGADE_SENTINEL(bb) && b != end;
> - b = APR_BUCKET_NEXT(b))
> - {
> - if (APR_BUCKET_IS_EOS(b)) {
> - return 1;
> + else {
> + apr_bucket *end;
> + apr_status_t status = last_not_included(bb, len, 0, 0, &end);
> + if (status != APR_SUCCESS) {
> + return status;
> }
> +
> + for (b = APR_BRIGADE_FIRST(bb);
> + b != APR_BRIGADE_SENTINEL(bb) && b != end;
> + b = APR_BUCKET_NEXT(b))
> + {
> + if (APR_BUCKET_IS_EOS(b)) {
> + return 1;
> + }
> + }
> }
> return 0;
> }
> Index: modules/http2/h2_version.h
> ===================================================================
> --- modules/http2/h2_version.h (revision 1707467)
> +++ modules/http2/h2_version.h (revision 1707468)
> @@ -20,7 +20,7 @@
> * @macro
> * Version number of the h2 module as c string
> */
> -#define MOD_HTTP2_VERSION "0.9.9"
> +#define MOD_HTTP2_VERSION "1.0.0"
>
> /**
> * @macro
> @@ -28,7 +28,7 @@
> * release. This is a 24 bit number with 8 bits for major number, 8 bits
> * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
> */
> -#define MOD_HTTP2_VERSION_NUM 0x000909
> +#define MOD_HTTP2_VERSION_NUM 0x010000
>
>
> #endif /* mod_h2_h2_version_h */
>