From: Hyman Huang <yong.huang@smartx.com> v1: 1. Encapsulate the retry logic inside virNetTLSSession{Read,Write} 2. Use VIR_DEBUG instead of VIR_WARN to log the retry operation rfc: https://patchew.org/Libvirt/d716a59dc2c61916917c6d2e07d62055745606d5.1744044211.git.yong.huang@smartx.com/ Please review, thanks. Yong Hyman Huang (1): rpc: Add the retry argument for virNetTLSSession{Read,Write} src/rpc/virnetclient.c | 2 +- src/rpc/virnetsocket.c | 4 ++-- src/rpc/virnettlscontext.c | 28 ++++++++++++++++++++++++++-- src/rpc/virnettlscontext.h | 6 ++++-- 4 files changed, 33 insertions(+), 7 deletions(-) -- 2.27.0 From: Hyman Huang <yong.huang@smartx.com> As advised by the GNU TLS, the caller should attempt again if the gnutls_record_{recv,send} return EAGAIN or EINTR; check the following link to view the details: https://www.gnutls.org/manual/html_node/Data-transfer-and-termination.html Add the retry parameter for virNetTLSSession{Read,Write} functions in accordance with this guideline. This prevents the upper application from encountering the following error message when it calls the virConnectOpenAuth API: Unable to read TLS confirmation: Resource temporarily unavailable Signed-off-by: Hyman Huang <yong.huang@smartx.com> --- src/rpc/virnetclient.c | 2 +- src/rpc/virnetsocket.c | 4 ++-- src/rpc/virnettlscontext.c | 28 ++++++++++++++++++++++++++-- src/rpc/virnettlscontext.h | 6 ++++-- 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index XXXXXXX..XXXXXXX 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -XXX,XX +XXX,XX @@ int virNetClientSetTLSSession(virNetClient *client, ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); #endif /* !WIN32 */ - len = virNetTLSSessionRead(client->tls, buf, 1); + len = virNetTLSSessionRead(client->tls, buf, 1, true); if (len < 0 && errno != ENOMSG) { virReportSystemError(errno, "%s", _("Unable to read TLS confirmation")); diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index XXXXXXX..XXXXXXX 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -XXX,XX +XXX,XX @@ static ssize_t virNetSocketReadWire(virNetSocket *sock, char *buf, size_t len) if (sock->tlsSession && virNetTLSSessionGetHandshakeStatus(sock->tlsSession) == VIR_NET_TLS_HANDSHAKE_COMPLETE) { - ret = virNetTLSSessionRead(sock->tlsSession, buf, len); + ret = virNetTLSSessionRead(sock->tlsSession, buf, len, false); } else { ret = read(sock->fd, buf, len); } @@ -XXX,XX +XXX,XX @@ static ssize_t virNetSocketWriteWire(virNetSocket *sock, const char *buf, size_t if (sock->tlsSession && virNetTLSSessionGetHandshakeStatus(sock->tlsSession) == VIR_NET_TLS_HANDSHAKE_COMPLETE) { - ret = virNetTLSSessionWrite(sock->tlsSession, buf, len); + ret = virNetTLSSessionWrite(sock->tlsSession, buf, len, false); } else { ret = write(sock->fd, buf, len); /* sc_avoid_write */ } diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index XXXXXXX..XXXXXXX 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -XXX,XX +XXX,XX @@ void virNetTLSSessionSetIOCallbacks(virNetTLSSession *sess, ssize_t virNetTLSSessionWrite(virNetTLSSession *sess, - const char *buf, size_t len) + const char *buf, size_t len, + bool retry) { ssize_t ret; + rewrite: virObjectLock(sess); ret = gnutls_record_send(sess->session, buf, len); if (ret >= 0) goto cleanup; + if (retry && (ret == GNUTLS_E_AGAIN || ret == GNUTLS_E_INTERRUPTED)) { + /* + * GNU TLS advises calling the function again to obtain the data if EAGAIN is returned. + * See reference: https://www.gnutls.org/manual/html_node/Data-transfer-and-termination.html + * */ + VIR_DEBUG("Try writing data from the TLS session again"); + virObjectUnlock(sess); + goto rewrite; + } + switch (ret) { case GNUTLS_E_AGAIN: errno = EAGAIN; @@ -XXX,XX +XXX,XX @@ ssize_t virNetTLSSessionWrite(virNetTLSSession *sess, } ssize_t virNetTLSSessionRead(virNetTLSSession *sess, - char *buf, size_t len) + char *buf, size_t len, + bool retry) { ssize_t ret; + reread: virObjectLock(sess); ret = gnutls_record_recv(sess->session, buf, len); if (ret >= 0) goto cleanup; + if (retry && (ret == GNUTLS_E_AGAIN || ret == GNUTLS_E_INTERRUPTED)) { + /* + * GNU TLS advises calling the function again to obtain the data if EAGAIN is returned. + * See reference: https://www.gnutls.org/manual/html_node/Data-transfer-and-termination.html + * */ + VIR_DEBUG("Try reading data from the TLS session again"); + virObjectUnlock(sess); + goto reread; + } + switch (ret) { case GNUTLS_E_AGAIN: errno = EAGAIN; diff --git a/src/rpc/virnettlscontext.h b/src/rpc/virnettlscontext.h index XXXXXXX..XXXXXXX 100644 --- a/src/rpc/virnettlscontext.h +++ b/src/rpc/virnettlscontext.h @@ -XXX,XX +XXX,XX @@ void virNetTLSSessionSetIOCallbacks(virNetTLSSession *sess, void *opaque); ssize_t virNetTLSSessionWrite(virNetTLSSession *sess, - const char *buf, size_t len); + const char *buf, size_t len, + bool retry); ssize_t virNetTLSSessionRead(virNetTLSSession *sess, - char *buf, size_t len); + char *buf, size_t len, + bool retry); int virNetTLSSessionHandshake(virNetTLSSession *sess); -- 2.27.0 From: Hyman Huang <yong.huang@smartx.com> v2: 1. Move the retry logic outside of virNetTLSSession{Read,Write} 2. Try re-polling when handing EAGAIN suggested by Daniel v1: 1. Encapsulate the retry logic inside virNetTLSSession{Read,Write} 2. Use VIR_DEBUG instead of VIR_WARN to log the retry operation rfc: https://patchew.org/Libvirt/d716a59dc2c61916917c6d2e07d62055745606d5.1744044211.git.yong.huang@smartx.com/ Please review, thanks. Yong Hyman Huang (1): rpc: Add the {repoll,retry} logic in virNetClientSetTLSSession src/rpc/virnetclient.c | 8 ++++++++ 1 file changed, 8 insertions(+) -- 2.27.0 From: Hyman Huang <yong.huang@smartx.com> As advised by the GNU TLS, the caller should attempt again if the gnutls_record_{recv,send} return EAGAIN or EINTR; check the following link to view the details: https://www.gnutls.org/manual/html_node/Data-transfer-and-termination.html virNetClientSetTLSSession failed to handle EINTR/EGAIN, though EGAIN seems like it ought to be unlikely given that the caller waited for G_IO_IN. Add the {repoll, retry} logic to handle EINTR/EGAIN that may happen theoretically. This may reduce the likelihood that the upper application receives the following error message utmostly when it calls the virConnectOpenAuth API: Unable to read TLS confirmation: Resource temporarily unavailable Note that in order to fully avoid the mentioned problem, the upper application should retry virConnectOpenAuth. --- src/rpc/virnetclient.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index XXXXXXX..XXXXXXX 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -XXX,XX +XXX,XX @@ int virNetClientSetTLSSession(virNetClient *client, * etc. If we make the grade, it will send us a '\1' byte. */ +repoll: source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), G_IO_IN, client->eventCtx, @@ -XXX,XX +XXX,XX @@ int virNetClientSetTLSSession(virNetClient *client, ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); #endif /* !WIN32 */ +retry: len = virNetTLSSessionRead(client->tls, buf, 1); + if (len < 0 && errno == EINTR) { + goto retry; + } + if (len < 0 && errno == EAGAIN) { + goto repoll; + } if (len < 0 && errno != ENOMSG) { virReportSystemError(errno, "%s", _("Unable to read TLS confirmation")); -- 2.27.0