Re: svn commit: r1897458 - in /httpd/httpd/trunk: changes-entries/ab-ssl-sense-fix.txt support/ab.c

[Graham Leggett]

On 27 Jan 2022, at 09:53, Ruediger Pluem  wrote:

>> Modified: httpd/httpd/trunk/support/ab.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/support/ab.c?rev=1897458=1897457=1897458=diff
>>  
>> 
>> ==
>> --- httpd/httpd/trunk/support/ab.c (original)
>> +++ httpd/httpd/trunk/support/ab.c Tue Jan 25 15:54:22 2022
> 
>> @@ -810,9 +811,6 @@ static void ssl_proceed_handshake(struct
>> 
>> static void write_request(struct connection * c)
>> {
>> -if (started >= requests) {
>> -return;
>> -}
> 
> Why is this no longer needed?

It’s in the wrong place, this has been moved one level up. 

>> do {
>> apr_time_t tnow;
> 
>> @@ -1461,7 +1465,6 @@ static void start_connect(struct connect
>> }
>> 
>> /* connected first time */
>> -set_conn_state(c, STATE_CONNECTED);
> 
> Why don't we set the state to connected any longer?
> 
>> #ifdef USE_SSL
>> if (c->ssl) {
>> ssl_proceed_handshake(c);

…because directly after being set, ssl_proceed_handshake() or read_connection() 
sets the state to something else.

Part of the confusion is that these states represent how the code needs to 
react after the poll. It seems in a number of places they were being set 
arbitrarily where it didn’t make sense.

>> @@ -1786,7 +1799,7 @@ read_more:
>> c->read = c->bread = 0;
>> /* zero connect time with keep-alive */
>> c->start = c->connect = lasttime = apr_time_now();
>> -set_conn_state(c, STATE_CONNECTED);
> 
> Why don't we set the state to connected any longer?
> 
>> +
>> write_request(c);

Again, directly after being set, write_request() sets it to something else.

>> }
>> }
> 
>> @@ -2048,7 +2077,7 @@ static void test(void)
>> continue;
>> }
>> else {
>> -set_conn_state(c, STATE_CONNECTED);
> 
> Why don't we set the state to connected any longer?
> 
>> +
>> #ifdef USE_SSL
>> if (c->ssl)
>> ssl_proceed_handshake(c);

Same reason as above.

Regards,
Graham
—

Re: svn commit: r1897458 - in /httpd/httpd/trunk: changes-entries/ab-ssl-sense-fix.txt support/ab.c

[Ruediger Pluem]

On 1/25/22 4:54 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Tue Jan 25 15:54:22 2022
> New Revision: 1897458
> 
> URL: http://svn.apache.org/viewvc?rev=1897458=rev
> Log:
> ab: Respond appropriately to SSL_ERROR_WANT_READ and SSL_ERROR_WANT_WRITE.
> Previously the correct event was polled for, but the response to the poll
> would call write instead of read, and read instead of write. PR 55952
> 
> Added:
> httpd/httpd/trunk/changes-entries/ab-ssl-sense-fix.txt
> Modified:
> httpd/httpd/trunk/support/ab.c
> 

> Modified: httpd/httpd/trunk/support/ab.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/support/ab.c?rev=1897458=1897457=1897458=diff
> ==
> --- httpd/httpd/trunk/support/ab.c (original)
> +++ httpd/httpd/trunk/support/ab.c Tue Jan 25 15:54:22 2022

> @@ -810,9 +811,6 @@ static void ssl_proceed_handshake(struct
>  
>  static void write_request(struct connection * c)
>  {
> -if (started >= requests) {
> -return;
> -}

Why is this no longer needed?

>  
>  do {
>  apr_time_t tnow;

> @@ -1461,7 +1465,6 @@ static void start_connect(struct connect
>  }
>  
>  /* connected first time */
> -set_conn_state(c, STATE_CONNECTED);

Why don't we set the state to connected any longer?

>  #ifdef USE_SSL
>  if (c->ssl) {
>  ssl_proceed_handshake(c);

> @@ -1786,7 +1799,7 @@ read_more:
>  c->read = c->bread = 0;
>  /* zero connect time with keep-alive */
>  c->start = c->connect = lasttime = apr_time_now();
> -set_conn_state(c, STATE_CONNECTED);

Why don't we set the state to connected any longer?

> +
>  write_request(c);
>  }
>  }

> @@ -2048,7 +2077,7 @@ static void test(void)
>  continue;
>  }
>  else {
> -set_conn_state(c, STATE_CONNECTED);

Why don't we set the state to connected any longer?

> +
>  #ifdef USE_SSL
>  if (c->ssl)
>  ssl_proceed_handshake(c);


Regards

Rüdiger