Re: [RFC PATCH v2 5/8] crypto: Remove qcrypto_tls_session_get_handshake_status

[Daniel P . Berrangé]

On Fri, Feb 07, 2025 at 11:27:55AM -0300, Fabiano Rosas wrote:
> The correct way of calling qcrypto_tls_session_handshake() requires
> calling qcrypto_tls_session_get_handshake_status() right after it so
> there's no reason to have a separate method.
> 
> Refactor qcrypto_tls_session_handshake() to inform the status in its
> own return value and alter the callers accordingly.
> 
> No functional change.
> 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Fabiano Rosas 
> ---
>  crypto/tlssession.c | 64 +++--
>  include/crypto/tlssession.h | 32 ---
>  io/channel-tls.c|  7 ++--
>  tests/unit/test-crypto-tlssession.c | 12 ++
>  4 files changed, 39 insertions(+), 76 deletions(-)
> 
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> @@ -720,14 +710,6 @@ qcrypto_tls_session_check_pending(QCryptoTLSSession 
> *session)
>  int
>  qcrypto_tls_session_handshake(QCryptoTLSSession *sess,
>Error **errp)
> -{
> -error_setg(errp, "TLS requires GNUTLS support");
> -return -1;
> -}
> -

This codepath is the !GNUTLS branch, so we need to continue
reporting an error here, not return QCRYPTO_TLS_HANDSHAKE_COMPLETE.

> -
> -QCryptoTLSSessionHandshakeStatus
> -qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess)
>  {
>  return QCRYPTO_TLS_HANDSHAKE_COMPLETE;
>  }


With that small change made

Reviewed-by: Daniel P. Berrangé 
Acked-by: Daniel P. Berrangé 


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[RFC PATCH v2 5/8] crypto: Remove qcrypto_tls_session_get_handshake_status

[Fabiano Rosas]

The correct way of calling qcrypto_tls_session_handshake() requires
calling qcrypto_tls_session_get_handshake_status() right after it so
there's no reason to have a separate method.

Refactor qcrypto_tls_session_handshake() to inform the status in its
own return value and alter the callers accordingly.

No functional change.

Suggested-by: Daniel P. Berrangé 
Signed-off-by: Fabiano Rosas 
---
 crypto/tlssession.c | 64 +++--
 include/crypto/tlssession.h | 32 ---
 io/channel-tls.c|  7 ++--
 tests/unit/test-crypto-tlssession.c | 12 ++
 4 files changed, 39 insertions(+), 76 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index d769d7a304..567698f5d9 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -546,45 +546,35 @@ qcrypto_tls_session_handshake(QCryptoTLSSession *session,
   Error **errp)
 {
 int ret = gnutls_handshake(session->handle);
-if (ret == 0) {
+if (!ret) {
 session->handshakeComplete = true;
-} else {
-if (ret == GNUTLS_E_INTERRUPTED ||
-ret == GNUTLS_E_AGAIN) {
-ret = 1;
-} else {
-if (session->rerr || session->werr) {
-error_setg(errp, "TLS handshake failed: %s: %s",
-   gnutls_strerror(ret),
-   error_get_pretty(session->rerr ?
-session->rerr : session->werr));
-} else {
-error_setg(errp, "TLS handshake failed: %s",
-   gnutls_strerror(ret));
-}
-ret = -1;
-}
-}
-error_free(session->rerr);
-error_free(session->werr);
-session->rerr = session->werr = NULL;
-
-return ret;
-}
-
-
-QCryptoTLSSessionHandshakeStatus
-qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session)
-{
-if (session->handshakeComplete) {
 return QCRYPTO_TLS_HANDSHAKE_COMPLETE;
-} else if (gnutls_record_get_direction(session->handle) == 0) {
-return QCRYPTO_TLS_HANDSHAKE_RECVING;
+}
+
+if (ret == GNUTLS_E_INTERRUPTED || ret == GNUTLS_E_AGAIN) {
+int direction = gnutls_record_get_direction(session->handle);
+return direction ? QCRYPTO_TLS_HANDSHAKE_SENDING :
+QCRYPTO_TLS_HANDSHAKE_RECVING;
+}
+
+if (session->rerr || session->werr) {
+error_setg(errp, "TLS handshake failed: %s: %s",
+   gnutls_strerror(ret),
+   error_get_pretty(session->rerr ?
+session->rerr : session->werr));
 } else {
-return QCRYPTO_TLS_HANDSHAKE_SENDING;
+error_setg(errp, "TLS handshake failed: %s",
+   gnutls_strerror(ret));
 }
+
+error_free(session->rerr);
+error_free(session->werr);
+session->rerr = session->werr = NULL;
+
+return -1;
 }
 
+
 int
 qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
 {
@@ -720,14 +710,6 @@ qcrypto_tls_session_check_pending(QCryptoTLSSession 
*session)
 int
 qcrypto_tls_session_handshake(QCryptoTLSSession *sess,
   Error **errp)
-{
-error_setg(errp, "TLS requires GNUTLS support");
-return -1;
-}
-
-
-QCryptoTLSSessionHandshakeStatus
-qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess)
 {
 return QCRYPTO_TLS_HANDSHAKE_COMPLETE;
 }
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index c0f64ce989..d77ae0d423 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -75,12 +75,14 @@
  *  GINT_TO_POINTER(fd));
  *
  *while (1) {
- *   if (qcrypto_tls_session_handshake(sess, errp) < 0) {
+ *   int ret = qcrypto_tls_session_handshake(sess, errp);
+ *
+ *   if (ret < 0) {
  *   qcrypto_tls_session_free(sess);
  *   return -1;
  *   }
  *
- *   switch(qcrypto_tls_session_get_handshake_status(sess)) {
+ *   switch(ret) {
  *   case QCRYPTO_TLS_HANDSHAKE_COMPLETE:
  *   if (qcrypto_tls_session_check_credentials(sess, errp) < )) {
  *   qcrypto_tls_session_free(sess);
@@ -170,7 +172,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSSession, 
qcrypto_tls_session_free)
  *
  * Validate the peer's credentials after a successful
  * TLS handshake. It is an error to call this before
- * qcrypto_tls_session_get_handshake_status() returns
+ * qcrypto_tls_session_handshake() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
  * Returns 0 if the credentials validated, -1 on error
@@ -226,7 +228,7 @@ void qcrypto_tls_session_set_callbacks(QCryptoTLSSession 
*sess,
  * registered with qcrypto_tls_session_set_callbacks()
  *
  * It is an error to call this before
- * qcrypto_tls_session_get_handshake_status() returns
+ * qcrypto_tls_session_handshake() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
  * Returns: th