Re: svn commit: r1813027 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

On Fri, Nov 3, 2017 at 4:00 AM, Stefan Eissing
 wrote:
> Can we count that as a +1?

Once I've tested. My initial reviews were ensuring compatibility.

Like you, I don't have a repro case yet. I see there is some activity within
httpd test framework that may cover it, just need to test it, if someone
else is further along with their review you may have a +1 faster than
I can get to it.

Re: svn commit: r1813027 - /httpd/httpd/branches/2.4.x/STATUS

[Stefan Eissing]

Can we count that as a +1?

> Am 01.11.2017 um 18:34 schrieb William A Rowe Jr :
> 
> You are right, thanks. With the new _ex entry points the backport looks ABI 
> clean, nicely done Yann.
> 
> On Nov 1, 2017 06:19, "Marion & Christophe JAILLET" 
>  wrote:
> Same analysis here. The 2 declarations are the same.
> 
> CJ
> 
> 
> Le 01/11/2017 à 11:02, Rainer Jung a écrit :
> Hi Bill,
> 
> Am 31.10.2017 um 21:29 schrieb William A Rowe Jr:
> On Mon, Oct 23, 2017 at 10:17 AM,  wrote:
> Author: ylavic
> Date: Mon Oct 23 15:17:02 2017
> New Revision: 1813027
> 
> URL: http://svn.apache.org/viewvc?rev=1813027=rev
> Log:
> Update comment according to patch version (v5).
> 
> Modified:
>  httpd/httpd/branches/2.4.x/STATUS
> 
> @@ -138,7 +137,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>+1: ylavic,
> wrowe: Suspect that this is an MMN Major bump, not minor without some
>additional detection/workaround of legacy 2.4 compiled modules.
> - ylavic: Changed to v4 (+r1812193) which hopefully should address compat
> + ylavic: Changed to v5 (+r1812193) which hopefully should address compat
>issues, Bill?
> 
> That seems to help with the config parser.
> 
> This looks problematic; it is undoubtedly a binary ABI change to a
> public interface.
> 
> -APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler, (request_rec *r,
> -  proxy_worker *worker, proxy_server_conf
> *conf, char *url,
> -  const char *proxyhost, apr_port_t proxyport))
> -APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler, (request_rec *r,
> -  char *url))
> [...]
> +APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler,
> +  (request_rec *r, proxy_worker *worker,
> +   proxy_server_conf *conf, char *url,
> +   const char *proxyhost, apr_port_t proxyport))
> +APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler,
> +  (request_rec *r, char *url))
> 
> 
> This part you cited to me looks like just a reformat (position of line breaks 
> changed), but I couldn't spot a functional change there.
> 
> Regards,
> 
> Rainer
> 
> 

Re: svn commit: r1813027 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

On Wed, Nov 1, 2017 at 6:34 PM, William A Rowe Jr  wrote:
> You are right, thanks. With the new _ex entry points the backport looks ABI
> clean, nicely done Yann.

Thanks Bill, by taking a last look at the proposed patch, I've noticed
that v5 partially reverted r1747069, a change that you made and
backported to 2.4.21 (IIRC), but which happened later in time than
than my SSLProxy changes in trunk.
I missed that when resolving the conflict around this code in v5.

So I've just updated the proposal to v6 with this (only) change:

--- httpd-2.4.x-r1740928_and_co-v5.patch
+++ httpd-2.4.x-r1740928_and_co-v6.patch
@@ -2069,13 +2046,13 @@ Index: modules/ssl/ssl_engine_io.c
  }
  }
 -else if ((sc->proxy_ssl_check_peer_cn == SSL_ENABLED_TRUE) &&
-+else if ((dc->proxy->ssl_check_peer_cn != FALSE) &&
++else if ((dc->proxy->ssl_check_peer_cn == TRUE) &&
  hostname_note) {
  const char *hostname;
  int match = 0;
_

which restores the existing (and expected) behaviour here.


Regards,
Yann.

Re: svn commit: r1813027 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

You are right, thanks. With the new _ex entry points the backport looks ABI
clean, nicely done Yann.

On Nov 1, 2017 06:19, "Marion & Christophe JAILLET" <
christophe.jail...@wanadoo.fr> wrote:

> Same analysis here. The 2 declarations are the same.
>
> CJ
>
>
> Le 01/11/2017 à 11:02, Rainer Jung a écrit :
>
>> Hi Bill,
>>
>> Am 31.10.2017 um 21:29 schrieb William A Rowe Jr:
>>
>>> On Mon, Oct 23, 2017 at 10:17 AM,  wrote:
>>>
 Author: ylavic
 Date: Mon Oct 23 15:17:02 2017
 New Revision: 1813027

 URL: http://svn.apache.org/viewvc?rev=1813027=rev
 Log:
 Update comment according to patch version (v5).

 Modified:
  httpd/httpd/branches/2.4.x/STATUS

 @@ -138,7 +137,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
+1: ylavic,
 wrowe: Suspect that this is an MMN Major bump, not minor
 without some
additional detection/workaround of legacy 2.4 compiled
 modules.
 - ylavic: Changed to v4 (+r1812193) which hopefully should address
 compat
 + ylavic: Changed to v5 (+r1812193) which hopefully should address
 compat
issues, Bill?

>>>
>>> That seems to help with the config parser.
>>>
>>> This looks problematic; it is undoubtedly a binary ABI change to a
>>> public interface.
>>>
>>> -APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler,
>>> (request_rec *r,
>>> -  proxy_worker *worker, proxy_server_conf
>>> *conf, char *url,
>>> -  const char *proxyhost, apr_port_t proxyport))
>>> -APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler,
>>> (request_rec *r,
>>> -  char *url))
>>> [...]
>>> +APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler,
>>> +  (request_rec *r, proxy_worker *worker,
>>> +   proxy_server_conf *conf, char *url,
>>> +   const char *proxyhost, apr_port_t proxyport))
>>> +APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler,
>>> +  (request_rec *r, char *url))
>>>
>>>
>> This part you cited to me looks like just a reformat (position of line
>> breaks changed), but I couldn't spot a functional change there.
>>
>> Regards,
>>
>> Rainer
>>
>>
>

Re: svn commit: r1813027 - /httpd/httpd/branches/2.4.x/STATUS

[Marion & Christophe JAILLET]

Same analysis here. The 2 declarations are the same.

CJ


Le 01/11/2017 à 11:02, Rainer Jung a écrit :

Hi Bill,

Am 31.10.2017 um 21:29 schrieb William A Rowe Jr:

On Mon, Oct 23, 2017 at 10:17 AM,  wrote:

Author: ylavic
Date: Mon Oct 23 15:17:02 2017
New Revision: 1813027

URL: http://svn.apache.org/viewvc?rev=1813027=rev
Log:
Update comment according to patch version (v5).

Modified:
 httpd/httpd/branches/2.4.x/STATUS

@@ -138,7 +137,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   +1: ylavic,
    wrowe: Suspect that this is an MMN Major bump, not minor 
without some
   additional detection/workaround of legacy 2.4 
compiled modules.
- ylavic: Changed to v4 (+r1812193) which hopefully should 
address compat
+ ylavic: Changed to v5 (+r1812193) which hopefully should 
address compat

   issues, Bill?


That seems to help with the config parser.

This looks problematic; it is undoubtedly a binary ABI change to a
public interface.

-APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler, 
(request_rec *r,

-  proxy_worker *worker, proxy_server_conf
*conf, char *url,
-  const char *proxyhost, apr_port_t proxyport))
-APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler, 
(request_rec *r,

-  char *url))
[...]
+APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler,
+  (request_rec *r, proxy_worker *worker,
+   proxy_server_conf *conf, char *url,
+   const char *proxyhost, apr_port_t 
proxyport))

+APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler,
+  (request_rec *r, char *url))



This part you cited to me looks like just a reformat (position of line 
breaks changed), but I couldn't spot a functional change there.


Regards,

Rainer

Re: svn commit: r1813027 - /httpd/httpd/branches/2.4.x/STATUS

[Rainer Jung]

Hi Bill,

Am 31.10.2017 um 21:29 schrieb William A Rowe Jr:

On Mon, Oct 23, 2017 at 10:17 AM,   wrote:

Author: ylavic
Date: Mon Oct 23 15:17:02 2017
New Revision: 1813027

URL: http://svn.apache.org/viewvc?rev=1813027=rev
Log:
Update comment according to patch version (v5).

Modified:
 httpd/httpd/branches/2.4.x/STATUS

@@ -138,7 +137,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   +1: ylavic,
wrowe: Suspect that this is an MMN Major bump, not minor without some
   additional detection/workaround of legacy 2.4 compiled modules.
- ylavic: Changed to v4 (+r1812193) which hopefully should address compat
+ ylavic: Changed to v5 (+r1812193) which hopefully should address compat
   issues, Bill?


That seems to help with the config parser.

This looks problematic; it is undoubtedly a binary ABI change to a
public interface.

-APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler, (request_rec *r,
-  proxy_worker *worker, proxy_server_conf
*conf, char *url,
-  const char *proxyhost, apr_port_t proxyport))
-APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler, (request_rec *r,
-  char *url))
[...]
+APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler,
+  (request_rec *r, proxy_worker *worker,
+   proxy_server_conf *conf, char *url,
+   const char *proxyhost, apr_port_t proxyport))
+APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler,
+  (request_rec *r, char *url))



This part you cited to me looks like just a reformat (position of line 
breaks changed), but I couldn't spot a functional change there.


Regards,

Rainer

Re: svn commit: r1813027 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

On Mon, Oct 23, 2017 at 10:17 AM,   wrote:
> Author: ylavic
> Date: Mon Oct 23 15:17:02 2017
> New Revision: 1813027
>
> URL: http://svn.apache.org/viewvc?rev=1813027=rev
> Log:
> Update comment according to patch version (v5).
>
> Modified:
> httpd/httpd/branches/2.4.x/STATUS
>
> @@ -138,7 +137,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>   +1: ylavic,
>wrowe: Suspect that this is an MMN Major bump, not minor, without some
>   additional detection/workaround of legacy 2.4 compiled modules.
> - ylavic: Changed to v4 (+r1812193) which hopefully should address compat
> + ylavic: Changed to v5 (+r1812193) which hopefully should address compat
>   issues, Bill?

That seems to help with the config parser.

This looks problematic; it is undoubtedly a binary ABI change to a
public interface.

-APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler, (request_rec *r,
-  proxy_worker *worker, proxy_server_conf
*conf, char *url,
-  const char *proxyhost, apr_port_t proxyport))
-APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler, (request_rec *r,
-  char *url))
[...]
+APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, scheme_handler,
+  (request_rec *r, proxy_worker *worker,
+   proxy_server_conf *conf, char *url,
+   const char *proxyhost, apr_port_t proxyport))
+APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler,
+  (request_rec *r, char *url))