[gnutls-devel] [PATCH] Broke apart _gnutls_recv_int() to the packet and non-packet cases.

[Jaak Ristioja]

Postponed, rejected or forgotten?

On 15.02.2016 12:14, Jaak Ristioja wrote:
> Only gnutls_record_recv_packet() called _gnutls_recv_int() with
> (packet != NULL). I refactored this logic directly downstream into
> gnutls_record_recv_packet(). The _gnutls_recv_int() function now only
> handles non-packet specific logic. The _gnutls_recv_int2() function
> was created to deduplicate common code which would otherwise have
> ended up in both functions.
> 
> The rationale behind this change is to optimize what were previously
> calls of _gnutls_recv_int(). First of all _gnutls_recv_int() now has
> only 6 parameters, which according to the x86_64 System V Application
> Binary Interface should now fit into CPU registers and no longer use
> the stack. Secondly this change avoids a number of branching checks
> for both packet and non-packet cases.
> ---
>  lib/ext/heartbeat.c |   2 +-
>  lib/handshake.c     |   2 +-
>  lib/record.c        | 103 ++++++++++++++++++++++++++++++----------------------
>  lib/record.h        |   1 -
>  4 files changed, 62 insertions(+), 46 deletions(-)
> 
> diff --git a/lib/ext/heartbeat.c b/lib/ext/heartbeat.c
> index cc13508..77d990b 100644
> --- a/lib/ext/heartbeat.c
> +++ b/lib/ext/heartbeat.c
> @@ -229,7 +229,7 @@ gnutls_heartbeat_ping(gnutls_session_t session, size_t data_size,
>  
>  	case SHB_RECV:
>  		ret =
> -		    _gnutls_recv_int(session, GNUTLS_HEARTBEAT, NULL,
> +		    _gnutls_recv_int(session, GNUTLS_HEARTBEAT,
>  				     NULL, 0, NULL,
>  				     session->internals.
>  				     hb_actual_retrans_timeout_ms);
> diff --git a/lib/handshake.c b/lib/handshake.c
> index 3d7a153..82f605b 100644
> --- a/lib/handshake.c
> +++ b/lib/handshake.c
> @@ -3104,7 +3104,7 @@ static int recv_handshake_final(gnutls_session_t session, int init)
>  
>  		ret =
>  		    _gnutls_recv_int(session, GNUTLS_CHANGE_CIPHER_SPEC,
> -				     NULL, ccs, ccs_len, NULL, tleft);
> +				     ccs, ccs_len, NULL, tleft);
>  		if (ret <= 0) {
>  			ERR("recv ChangeCipherSpec", ret);
>  			gnutls_assert();
> diff --git a/lib/record.c b/lib/record.c
> index a2f8637..713dd94 100644
> --- a/lib/record.c
> +++ b/lib/record.c
> @@ -301,7 +301,7 @@ int gnutls_bye(gnutls_session_t session, gnutls_close_request_t how)
>  			do {
>  				ret =
>  				    _gnutls_recv_int(session, GNUTLS_ALERT,
> -						     NULL, NULL, 0, NULL,
> +						     NULL, 0, NULL,
>  						     session->internals.
>  						     record_timeout_ms);
>  			}
> @@ -1356,23 +1356,13 @@ _gnutls_recv_in_buffers(gnutls_session_t session, content_type_t type,
>  		return ret;
>  }
>  
> -/* This function behaves exactly like read(). The only difference is
> - * that it accepts the gnutls_session_t and the content_type_t of data to
> - * receive (if called by the user the Content is Userdata only)
> - * It is intended to receive data, under the current session.
> - */
> -ssize_t
> -_gnutls_recv_int(gnutls_session_t session, content_type_t type,
> -		 gnutls_packet_t *packet,
> -		 uint8_t * data, size_t data_size, void *seq,
> -		 unsigned int ms)
> +/* Returns a value greater than zero (>= 0) if buffers should be checked
> + * for data. */
> +static ssize_t
> +_gnutls_recv_int2(gnutls_session_t session)
>  {
>  	int ret;
>  
> -	if (packet == NULL && (type != GNUTLS_ALERT && type != GNUTLS_HEARTBEAT)
> -	    && (data_size == 0 || data == NULL))
> -		return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
> -
>  	if (session->internals.read_eof != 0) {
>  		/* if we have already read an EOF
>  		 */
> @@ -1390,37 +1380,48 @@ _gnutls_recv_int(gnutls_session_t session, content_type_t type,
>  			return gnutls_assert_val(ret);
>  
>  		session->internals.recv_state = RECV_STATE_0;
> +		/* Fall through: */
>  	case RECV_STATE_0:
>  
>  		_dtls_async_timer_check(session);
> +		return 1;
> +	default:
> +		return gnutls_assert_val(GNUTLS_E_INTERNAL_ERROR);
> +	}
> +}
>  
> -		if (packet == NULL) {
> -			/* If we have enough data in the cache do not bother receiving
> -			 * a new packet. (in order to flush the cache)
> -			 */
> -			ret = check_buffers(session, type, data, data_size, seq);
> -			if (ret != 0)
> -				return ret;
> +/* This function behaves exactly like read(). The only difference is
> + * that it accepts the gnutls_session_t and the content_type_t of data to
> + * receive (if called by the user the Content is Userdata only)
> + * It is intended to receive data, under the current session.
> + */
> +ssize_t
> +_gnutls_recv_int(gnutls_session_t session, content_type_t type,
> +		 uint8_t * data, size_t data_size, void *seq,
> +		 unsigned int ms)
> +{
> +	int ret;
>  
> -			ret = _gnutls_recv_in_buffers(session, type, -1, ms);
> -			if (ret < 0 && ret != GNUTLS_E_SESSION_EOF)
> -				return gnutls_assert_val(ret);
> +	if ((type != GNUTLS_ALERT && type != GNUTLS_HEARTBEAT)
> +	    && (data_size == 0 || data == NULL))
> +		return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
>  
> -			return check_buffers(session, type, data, data_size, seq);
> -		} else {
> -			ret = check_packet_buffers(session, type, packet);
> -			if (ret != 0)
> -				return ret;
> +	ret = _gnutls_recv_int2(session);
> +	if (ret <= 0)
> +		return ret;
>  
> -			ret = _gnutls_recv_in_buffers(session, type, -1, ms);
> -			if (ret < 0 && ret != GNUTLS_E_SESSION_EOF)
> -				return gnutls_assert_val(ret);
> +	/* If we have enough data in the cache do not bother receiving
> +	 * a new packet. (in order to flush the cache)
> +	 */
> +	ret = check_buffers(session, type, data, data_size, seq);
> +	if (ret != 0)
> +		return ret;
>  
> -			return check_packet_buffers(session, type, packet);
> -		}
> -	default:
> -		return gnutls_assert_val(GNUTLS_E_INTERNAL_ERROR);
> -	}
> +	ret = _gnutls_recv_in_buffers(session, type, -1, ms);
> +	if (ret < 0 && ret != GNUTLS_E_SESSION_EOF)
> +		return gnutls_assert_val(ret);
> +
> +	return check_buffers(session, type, data, data_size, seq);
>  }
>  
>  /**
> @@ -1511,9 +1512,25 @@ ssize_t
>  gnutls_record_recv_packet(gnutls_session_t session, 
>  		   	  gnutls_packet_t *packet)
>  {
> -	return _gnutls_recv_int(session, GNUTLS_APPLICATION_DATA, packet,
> -				NULL, 0, NULL,
> -				session->internals.record_timeout_ms);
> +	int ret;
> +
> +	if (packet == NULL)
> +		return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
> +
> +	ret = _gnutls_recv_int2(session);
> +	if (ret <= 0)
> +		return ret;
> +
> +	ret = check_packet_buffers(session, GNUTLS_APPLICATION_DATA, packet);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = _gnutls_recv_in_buffers(session, GNUTLS_APPLICATION_DATA, -1,
> +	                              session->internals.record_timeout_ms);
> +	if (ret < 0 && ret != GNUTLS_E_SESSION_EOF)
> +		return gnutls_assert_val(ret);
> +
> +	return check_packet_buffers(session, GNUTLS_APPLICATION_DATA, packet);
>  }
>  
>  /**
> @@ -1694,7 +1711,7 @@ int gnutls_record_uncork(gnutls_session_t session, unsigned int flags)
>  ssize_t
>  gnutls_record_recv(gnutls_session_t session, void *data, size_t data_size)
>  {
> -	return _gnutls_recv_int(session, GNUTLS_APPLICATION_DATA, NULL,
> +	return _gnutls_recv_int(session, GNUTLS_APPLICATION_DATA,
>  				data, data_size, NULL,
>  				session->internals.record_timeout_ms);
>  }
> @@ -1723,7 +1740,7 @@ ssize_t
>  gnutls_record_recv_seq(gnutls_session_t session, void *data,
>  		       size_t data_size, unsigned char *seq)
>  {
> -	return _gnutls_recv_int(session, GNUTLS_APPLICATION_DATA, NULL,
> +	return _gnutls_recv_int(session, GNUTLS_APPLICATION_DATA,
>  				data, data_size, seq,
>  				session->internals.record_timeout_ms);
>  }
> diff --git a/lib/record.h b/lib/record.h
> index d029586..f16df47 100644
> --- a/lib/record.h
> +++ b/lib/record.h
> @@ -45,7 +45,6 @@ _gnutls_send_int(gnutls_session_t session, content_type_t type,
>  }
>  
>  ssize_t _gnutls_recv_int(gnutls_session_t session, content_type_t type,
> -			 gnutls_packet_t *packet,
>  			 uint8_t * data,
>  			 size_t sizeofdata, void *seq, unsigned int ms);
>  
>