Re: backport proposals

[Daniel Ruggeri]

Which reminds me... How about that cool proxy protocol patch? Anyone want to 
give it a whirl? ;-)
-- 
Daniel Ruggeri


 Original Message 
From: Jim Jagielski 
Sent: April 27, 2017 9:55:39 AM CDT
To: dev@httpd.apache.org
Subject: Re: backport proposals

Yeah... sometimes it is easier to create a "combined" patchfile
which shows the collected changes, for ease of reviewing, but, as
you say, it can get out of sync as other patches are backported,
esp if the proposed backport is in STATUS for a semi-extended
period of time.

> On Apr 27, 2017, at 10:47 AM, Eric Covener  wrote:
> 
> On Thu, Apr 27, 2017 at 10:15 AM, Stefan Eissing
>  wrote:
>> Take this as an observation about proposals in general, nothing wrong with 
>> this one in particular:
>> 
>>  *) mod_proxy_hcheck: Honor checks in Vhosts w/o hanging
>> trunk patch: http://svn.apache.org/r1784203
>>  http://svn.apache.org/r1784205
>>  http://svn.apache.org/r1784227
>>  http://svn.apache.org/r1784228
>>  http://svn.apache.org/r1784275
>>  http://svn.apache.org/r1785871
>>  http://svn.apache.org/r1786009
>>  http://svn.apache.org/r1789387
>> 2.4.x patch: trunk works *after r1779573 above* (modulo CHANGES)
>>  ie: 
>> http://home.apache.org/~ylavic/patches/httpd2.4-hcheck-after-r1779573.patch
>>   FULL hcheck patch: 
>> http://home.apache.org/~jim/patches/httpd2.4-hcheck.patch
>>  http://svn.apache.org/r1789387
>>   (includes all hcheck related patches, including showstopper)
>> +1: jim, ylavic
>> 
>> So, how to check this? I tried the FULL hcheck patch. It does not work, ~50% 
>> of hunks fail.
>> 
>> Ok, revert. Then I just tried this:
>>svn merge -c 
>> 1784203,1784205,1784227,1784228,1784275,1785871,1786009,1789387 
>> ^/httpd/httpd/trunk .
> 
> Looks much easier to me.  Another option is what kotkov recently used
> 
> Not 100% clear to me in this case that the collected patch was really
> just meant to be the same revs + merge conflicts due to the wording.

Re: svn commit: r1792169 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h modules/generators/mod_status.c modules/proxy/mod_proxy.c server/config.c server/util.c

[Eric Covener]

On Fri, Apr 21, 2017 at 4:44 AM,   wrote:
> +/* A request that has passed through .htaccess has no business
> + * landing up here.
> + */
> +if (ap_request_tainted(r, AP_TAINT_HTACCESS)) {
> +return DECLINED;
> +}
> +

If AllowOverride is enabled for the document root an d an htaccess is
present,  this renders /server-status unreachable, regardless of
what's in the htaccess. If we're going to block this by default, we
might as well just stop configuring it with SetHandler and then the
taint checking is not needed.

We also have in another thread the issue with RewriteRule ... [P] in
htaccess being blocked.  We need some kind of way to express a policy
that will be unique to different handlers.

-- 
Eric Covener
cove...@gmail.com

Re: backport proposals

[Jacob Champion]

On 04/27/2017 07:15 AM, Stefan Eissing wrote:

So, the proposal could have been written as:

*) mod_proxy_hcheck: Honor checks in Vhosts w/o hanging svn merge -c
1784203,1784205,1784227,1784228,1784275,1785871,1786009,1789387
^/httpd/httpd/trunk . +1: jim, ylavic

Wouldn't that be easier?


+1, and I plan to write similar proposals like this when I start 
backporting feature branches. They'll end up looking like


svn merge -r :HEAD ^/httpd/httpd/branches/

--Jacob

Re: backport proposals

[Jim Jagielski]

Yeah... sometimes it is easier to create a "combined" patchfile
which shows the collected changes, for ease of reviewing, but, as
you say, it can get out of sync as other patches are backported,
esp if the proposed backport is in STATUS for a semi-extended
period of time.

> On Apr 27, 2017, at 10:47 AM, Eric Covener  wrote:
> 
> On Thu, Apr 27, 2017 at 10:15 AM, Stefan Eissing
>  wrote:
>> Take this as an observation about proposals in general, nothing wrong with 
>> this one in particular:
>> 
>>  *) mod_proxy_hcheck: Honor checks in Vhosts w/o hanging
>> trunk patch: http://svn.apache.org/r1784203
>>  http://svn.apache.org/r1784205
>>  http://svn.apache.org/r1784227
>>  http://svn.apache.org/r1784228
>>  http://svn.apache.org/r1784275
>>  http://svn.apache.org/r1785871
>>  http://svn.apache.org/r1786009
>>  http://svn.apache.org/r1789387
>> 2.4.x patch: trunk works *after r1779573 above* (modulo CHANGES)
>>  ie: 
>> http://home.apache.org/~ylavic/patches/httpd2.4-hcheck-after-r1779573.patch
>>   FULL hcheck patch: 
>> http://home.apache.org/~jim/patches/httpd2.4-hcheck.patch
>>  http://svn.apache.org/r1789387
>>   (includes all hcheck related patches, including showstopper)
>> +1: jim, ylavic
>> 
>> So, how to check this? I tried the FULL hcheck patch. It does not work, ~50% 
>> of hunks fail.
>> 
>> Ok, revert. Then I just tried this:
>>svn merge -c 
>> 1784203,1784205,1784227,1784228,1784275,1785871,1786009,1789387 
>> ^/httpd/httpd/trunk .
> 
> Looks much easier to me.  Another option is what kotkov recently used
> 
> Not 100% clear to me in this case that the collected patch was really
> just meant to be the same revs + merge conflicts due to the wording.

Re: backport proposals

[Yann Ylavic]

On Thu, Apr 27, 2017 at 4:15 PM, Stefan Eissing
 wrote:
> Take this as an observation about proposals in general, nothing wrong with 
> this one in particular:
>
>   *) mod_proxy_hcheck: Honor checks in Vhosts w/o hanging
>  trunk patch: http://svn.apache.org/r1784203
>   http://svn.apache.org/r1784205
>   http://svn.apache.org/r1784227
>   http://svn.apache.org/r1784228
>   http://svn.apache.org/r1784275
>   http://svn.apache.org/r1785871
>   http://svn.apache.org/r1786009
>   http://svn.apache.org/r1789387
>  2.4.x patch: trunk works *after r1779573 above* (modulo CHANGES)
>   ie: 
> http://home.apache.org/~ylavic/patches/httpd2.4-hcheck-after-r1779573.patch
>FULL hcheck patch: 
> http://home.apache.org/~jim/patches/httpd2.4-hcheck.patch
>   http://svn.apache.org/r1789387
>(includes all hcheck related patches, including showstopper)
>  +1: jim, ylavic
>
> So, how to check this? I tried the FULL hcheck patch. It does not work, ~50% 
> of hunks fail.
>
> Ok, revert. Then I just tried this:
> svn merge -c 
> 1784203,1784205,1784227,1784228,1784275,1785871,1786009,1789387 
> ^/httpd/httpd/trunk .
>
> and all is well! So, the proposal could have been written as:
>
>   *) mod_proxy_hcheck: Honor checks in Vhosts w/o hanging
> svn merge -c 
> 1784203,1784205,1784227,1784228,1784275,1785871,1786009,1789387 
> ^/httpd/httpd/trunk .
>  +1: jim, ylavic
>
> Wouldn't that be easier? I mean, sometimes trunk and backport may differ a 
> lot. But most commonly, only CHANGES and message-tags need to be ignored. I 
> myself would prefer just to copy a one liner.

Agreed, I tried several merges too, that's why I added the patch I
tested in the proposal (httpd2.4-hcheck-after-r1779573.patch), which
corresponds to yours (as indicated in the trailer: "Merged
/httpd/httpd/trunk:r1784203,1784205,1784227-1784228,1784275,1785871,1786009,1789387").

Looks like it didn't help either :/

Re: backport proposals

[Eric Covener]

On Thu, Apr 27, 2017 at 10:15 AM, Stefan Eissing
 wrote:
> Take this as an observation about proposals in general, nothing wrong with 
> this one in particular:
>
>   *) mod_proxy_hcheck: Honor checks in Vhosts w/o hanging
>  trunk patch: http://svn.apache.org/r1784203
>   http://svn.apache.org/r1784205
>   http://svn.apache.org/r1784227
>   http://svn.apache.org/r1784228
>   http://svn.apache.org/r1784275
>   http://svn.apache.org/r1785871
>   http://svn.apache.org/r1786009
>   http://svn.apache.org/r1789387
>  2.4.x patch: trunk works *after r1779573 above* (modulo CHANGES)
>   ie: 
> http://home.apache.org/~ylavic/patches/httpd2.4-hcheck-after-r1779573.patch
>FULL hcheck patch: 
> http://home.apache.org/~jim/patches/httpd2.4-hcheck.patch
>   http://svn.apache.org/r1789387
>(includes all hcheck related patches, including showstopper)
>  +1: jim, ylavic
>
> So, how to check this? I tried the FULL hcheck patch. It does not work, ~50% 
> of hunks fail.
>
> Ok, revert. Then I just tried this:
> svn merge -c 
> 1784203,1784205,1784227,1784228,1784275,1785871,1786009,1789387 
> ^/httpd/httpd/trunk .

Looks much easier to me.  Another option is what kotkov recently used

Not 100% clear to me in this case that the collected patch was really
just meant to be the same revs + merge conflicts due to the wording.

backport proposals

[Stefan Eissing]

Take this as an observation about proposals in general, nothing wrong with this 
one in particular:

  *) mod_proxy_hcheck: Honor checks in Vhosts w/o hanging
 trunk patch: http://svn.apache.org/r1784203
  http://svn.apache.org/r1784205
  http://svn.apache.org/r1784227
  http://svn.apache.org/r1784228
  http://svn.apache.org/r1784275
  http://svn.apache.org/r1785871
  http://svn.apache.org/r1786009
  http://svn.apache.org/r1789387
 2.4.x patch: trunk works *after r1779573 above* (modulo CHANGES)
  ie: 
http://home.apache.org/~ylavic/patches/httpd2.4-hcheck-after-r1779573.patch
   FULL hcheck patch: 
http://home.apache.org/~jim/patches/httpd2.4-hcheck.patch
  http://svn.apache.org/r1789387
   (includes all hcheck related patches, including showstopper)
 +1: jim, ylavic

So, how to check this? I tried the FULL hcheck patch. It does not work, ~50% of 
hunks fail.

Ok, revert. Then I just tried this:
svn merge -c 
1784203,1784205,1784227,1784228,1784275,1785871,1786009,1789387 
^/httpd/httpd/trunk .

and all is well! So, the proposal could have been written as:

  *) mod_proxy_hcheck: Honor checks in Vhosts w/o hanging
svn merge -c 
1784203,1784205,1784227,1784228,1784275,1785871,1786009,1789387 
^/httpd/httpd/trunk .
 +1: jim, ylavic

Wouldn't that be easier? I mean, sometimes trunk and backport may differ a lot. 
But most commonly, only CHANGES and message-tags need to be ignored. I myself 
would prefer just to copy a one liner.

just saying (while the active hcheck runs in my proxy tests flawlessly),

-Stefan

Re: HTTP Server Hackathon/BOFs in Miami?

[jean-frederic clere]

On 04/18/2017 08:38 PM, William A Rowe Jr wrote:
> Evaluating whether I will attend ApacheCon, the most specific reason
> would be hackathon time. Or productive BoF sessions.
> 
> Who all is planning to spend some time hacking at ACNA '17? Ideas for
> projects or BoF topics?

I will be there if I have time I need help/directives to "dump"
mod_cluster to httpd, but I will quite busy with my talks and the TomcatCon.

Cheers

Jean-Frederic

Re: svn commit: r1707087 - /httpd/httpd/trunk/modules/debugging/mod_bucketeer.c

[Yann Ylavic]

On Thu, Apr 27, 2017 at 2:45 PM, Plüm, Rüdiger, Vodafone Group
 wrote:
> Shouldn't we call apr_brigade_cleanup in any case after ap_pass_brigade?

We should yes, I first did this since we don't want possible r->pool's
buckets staying in bb.
I wanted to cleaning up tmp_bb too when rv != APR_SUCCESS && !eos,
actually that were both upcoming questions if the patch works and
looks reasonable otherwise ;)


Regards,
Yann.

AW: svn commit: r1707087 - /httpd/httpd/trunk/modules/debugging/mod_bucketeer.c

[Plüm , Rüdiger , Vodafone Group]

Shouldn't we call apr_brigade_cleanup in any case after ap_pass_brigade?

Regards

Rüdiger

> -Ursprüngliche Nachricht-
> Von: Yann Ylavic [mailto:ylavic@gmail.com]
> Gesendet: Donnerstag, 27. April 2017 11:47
> An: httpd-dev 
> Betreff: Re: svn commit: r1707087 -
> /httpd/httpd/trunk/modules/debugging/mod_bucketeer.c
> 
> On Wed, Apr 26, 2017 at 11:26 AM, Stefan Eissing
>  wrote:
> >
> >> Am 26.04.2017 um 11:14 schrieb Plüm, Rüdiger, Vodafone Group
> :
> >>
> >>
> >>
> >>> -Ursprüngliche Nachricht-
> >>> Von: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> >>> Gesendet: Mittwoch, 26. April 2017 10:55
> >>> An: dev@httpd.apache.org
> >>> Betreff: Re: svn commit: r1707087 -
> >>> /httpd/httpd/trunk/modules/debugging/mod_bucketeer.c
> >>>
> >>> Eh, not really into mod_bucketeer and its use cases, but some
> >>> observations after a quick scan:
> >>>
> >>> line 99:
> >>>rv = ap_pass_brigade(f->next, ctx->bb);
> >>>apr_brigade_cleanup(ctx->bb);
> >>>
> >>> line 146:
> >>>rv = ap_pass_brigade(f->next, ctx->bb);
> >>>if (rv) {
> >>>return rv;
> >>>}
> >>>apr_brigade_cleanup(ctx->bb);
> >>>
> >>> such things only work if an EOS always comes *before* an EOR
> >>> bucket (case 1) or of no DATA bucket of any kind follows an EOR.
> >>
> >> Correct, but with the BUCKETEER filter being a resource filter that
> >> should not happen.
> >
> > Because the implementations of everything else in the server do not
> > do it? Should we then add a filter that checks exactly that?
> 
> I don't think that EOR before EOS can happen in HTTP/1, because
> ap_finalize_request_protocol() is always called before
> ap_process_request_after_handler().
> 
> EOS is the signal for request filters to get out of the way, so this
> is probably what ap_request_core_filter() should do too, and the
> purpose of the attached patch.
> This patch could use EOR instead, but I find EOS more "semantically"
> correct for a request filter (we don't need to set aside anything
> after it), while EOR would be crash safe only...
> 
> Jacob, does it work better?
> 
> 
> Regards,
> Yann.

Re: svn commit: r1707087 - /httpd/httpd/trunk/modules/debugging/mod_bucketeer.c

[Yann Ylavic]

On Wed, Apr 26, 2017 at 11:26 AM, Stefan Eissing
 wrote:
>
>> Am 26.04.2017 um 11:14 schrieb Plüm, Rüdiger, Vodafone Group 
>> :
>>
>>
>>
>>> -Ursprüngliche Nachricht-
>>> Von: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
>>> Gesendet: Mittwoch, 26. April 2017 10:55
>>> An: dev@httpd.apache.org
>>> Betreff: Re: svn commit: r1707087 -
>>> /httpd/httpd/trunk/modules/debugging/mod_bucketeer.c
>>>
>>> Eh, not really into mod_bucketeer and its use cases, but some
>>> observations after a quick scan:
>>>
>>> line 99:
>>>rv = ap_pass_brigade(f->next, ctx->bb);
>>>apr_brigade_cleanup(ctx->bb);
>>>
>>> line 146:
>>>rv = ap_pass_brigade(f->next, ctx->bb);
>>>if (rv) {
>>>return rv;
>>>}
>>>apr_brigade_cleanup(ctx->bb);
>>>
>>> such things only work if an EOS always comes *before* an EOR
>>> bucket (case 1) or of no DATA bucket of any kind follows an EOR.
>>
>> Correct, but with the BUCKETEER filter being a resource filter that
>> should not happen.
>
> Because the implementations of everything else in the server do not
> do it? Should we then add a filter that checks exactly that?

I don't think that EOR before EOS can happen in HTTP/1, because
ap_finalize_request_protocol() is always called before
ap_process_request_after_handler().

EOS is the signal for request filters to get out of the way, so this
is probably what ap_request_core_filter() should do too, and the
purpose of the attached patch.
This patch could use EOR instead, but I find EOS more "semantically"
correct for a request filter (we don't need to set aside anything
after it), while EOR would be crash safe only...

Jacob, does it work better?


Regards,
Yann.
Index: server/request.c
===
--- server/request.c	(revision 1791747)
+++ server/request.c	(working copy)
@@ -2057,12 +2057,27 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
 tmp_bb = f->ctx = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
 }
 
+/* Use the given bb downstream (purposedly scoped), hence move the
+ * buckets into tmp_bb to walk through them locally.
+ */
+APR_BRIGADE_CONCAT(tmp_bb, bb);
+
 /* Reinstate any buffered content */
-ap_filter_reinstate_brigade(f, bb, _upto);
+ap_filter_reinstate_brigade(f, tmp_bb, _upto);
 
-while (!APR_BRIGADE_EMPTY(bb)) {
-apr_bucket *bucket = APR_BRIGADE_FIRST(bb);
+while (!APR_BRIGADE_EMPTY(tmp_bb)) {
+apr_bucket *bucket = APR_BRIGADE_FIRST(tmp_bb);
+int eos = APR_BUCKET_IS_EOS(bucket);
 
+if (eos) {
+/* pass everything down the chain, this request is over (and will
+ * possibly be destroyed before we leave, should the EOR be aside),
+ * not this filter's business anymore.
+ */
+APR_BRIGADE_CONCAT(bb, tmp_bb);
+ap_remove_output_filter(f);
+}
+else {
 /* if the core has set aside data, back off and try later */
 if (!flush_upto) {
 if (ap_filter_should_yield(f)) {
@@ -2088,20 +2103,19 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
 }
 }
 }
-
 /* pass each bucket down the chain */
 APR_BUCKET_REMOVE(bucket);
-APR_BRIGADE_INSERT_TAIL(tmp_bb, bucket);
+APR_BRIGADE_INSERT_TAIL(bb, bucket);
+}
 
-status = ap_pass_brigade(f->next, tmp_bb);
-if (!APR_STATUS_IS_EOF(status) && (status != APR_SUCCESS)) {
+status = ap_pass_brigade(f->next, bb);
+if (status != APR_SUCCESS || eos) {
 return status;
 }
-
+apr_brigade_cleanup(bb);
 }
 
-ap_filter_setaside_brigade(f, bb);
-return status;
+return ap_filter_setaside_brigade(f, tmp_bb);
 }
 
 extern APR_OPTIONAL_FN_TYPE(authz_some_auth_required) *ap__authz_ap_some_auth_required;