Bug#35703454 NdbSocket refactoring - move close calls [NOCLOSE]

In preparation for next patch that pass NdbSocket either by const
reference (const NdbSocket&) for temporary use or rvalue reference
(NdbSocket&&) for transfer ownership, calls to close must move since
only owner may call close (or any other non const method).

Calls to NdbSocket::close are moved from caller to callee for functions
that takes ownership using NdbSocket&& since the caller must not use the
moved socket after call.

And since callee will to the close there is no need to pass back close
with reset flag to caller and that is removed from transporter_connect
and connect_server methods.

Calls to NdbSocket::close are moved from callee to caller for functions
that are changed to "borrow" socket using const NdbSocket& since callee
can not call non const methods.

To keep the semantics of the function making the socket unusable on
failure a new method NdbSocket::shutdown is introduced that only stop
further communication on socket but still leave socket open (until
caller calls close).

Change-Id: I9666ba76e03e268406677c1e32d76d7d219e9a56

Author: zmur

storage/ndb/include/portlib/ndb_socket_posix.h

@@ -78,6 +78,12 @@ int ndb_socket_configure_reuseaddr(ndb_socket_t s, int enable)
   return ndb_socket_reuseaddr(s, enable);
 }
 
+static inline
+int ndb_socket_shutdown_both(ndb_socket_t s)
+{
+  return shutdown(s.s, SHUT_RDWR);
+}
+
 static inline
 int ndb_socket_close(ndb_socket_t s)
 {

storage/ndb/include/portlib/ndb_socket_win32.h

@@ -83,6 +83,12 @@ int ndb_socket_configure_reuseaddr(ndb_socket_t s, int enable)
   return ndb_setsockopt(s, SOL_SOCKET, SO_EXCLUSIVEADDRUSE, &on);
 }
 
+static inline
+int ndb_socket_shutdown_both(ndb_socket_t s)
+{
+  return shutdown(s.s, SD_BOTH);
+}
+
 static inline
 int ndb_socket_close(ndb_socket_t s)
 {

storage/ndb/include/transporter/TransporterRegistry.hpp

@@ -249,16 +249,12 @@ class TransporterRegistry
      @param sockfd           the socket to handshake
      @param msg              error message describing why handshake failed,
                              to be filled in when function return
-     @param close_with_reset allows the function to indicate to the caller
-                             how the socket should be closed when function
-                             returns false
      @param log_failure      whether a failure to connect is log-worthy
 
      @returns false on failure and true on success
   */
   bool connect_server(NdbSocket & sockfd,
                       BaseString& msg,
-                      bool& close_with_reset,
                       bool& log_failure);
 
   bool connect_client(NdbMgmHandle *h);

storage/ndb/include/util/NdbSocket.h

@@ -134,7 +134,7 @@ class NdbSocket {
         and SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER.
      See SSL_set_mode(3) for reference on flags.
   */
-  int set_nonblocking(int on);
+  int set_nonblocking(int on) const;
 
   /* ndb_socket.h */
   ssize_t recv(char * buf, size_t len, int peek = 0) const;
@@ -156,9 +156,10 @@ class NdbSocket {
   int readln(int timeout, int *time, char *buf, int len, NdbMutex *) const;
   int write(int timeout, int *, const char * buf, int len) const;
 
+  int shutdown() const;
   /* For SSL sockets, close() removes the SSL from the SSL Socket Table */
   int close();
-  void close_with_reset(bool with_reset);
+  void close_with_reset();
 
   /* ndb_socket_poller.h */
   uint add_readable(ndb_socket_poller *) const;
@@ -179,6 +180,7 @@ class NdbSocket {
   int ssl_readln(int timeout, int *, char * buf, int len, NdbMutex *) const;
   int ssl_write(int timeout, int *, const char * buf, int len) const;
 
+  int ssl_shutdown() const;
   bool ssl_handshake();
   void ssl_close();
 
@@ -266,6 +268,13 @@ ssize_t NdbSocket::writev(const struct iovec *vec, int nvec) const {
   return ndb_socket_writev(s, vec, nvec);
 }
 
+inline
+int NdbSocket::shutdown() const {
+  if (ssl) ssl_shutdown();
+  int r = ndb_socket_shutdown_both(s);
+  return r;
+}
+
 inline
 int NdbSocket::close() {
   if(ssl) ssl_close();
@@ -276,9 +285,10 @@ int NdbSocket::close() {
 }
 
 inline
-void NdbSocket::close_with_reset(bool with_reset) {
+void NdbSocket::close_with_reset() {
   if(ssl) ssl_close();
   disable_locking();
+  constexpr bool with_reset = true;
   ndb_socket_close_with_reset(s, with_reset);
   invalidate_socket_handle();
 }

storage/ndb/src/common/transporter/Transporter.cpp

@@ -343,7 +343,7 @@ Transporter::connect_client()
    /** Socket Authentication */
     if(m_socket_client->authenticate(secureSocket) < SocketAuthSimple::AuthOk)
     {
-      DEBUG_FPRINTF((stderr, "Socket Authenticator failed\n"));
+      secureSocket.close();
       DBUG_RETURN(false);
     }
 
@@ -361,6 +361,7 @@ Transporter::connect_client(NdbSocket & socket)
   {
     DBUG_PRINT("error", ("Already connected"));
     DEBUG_FPRINTF((stderr, "Already connected\n"));
+    socket.close();
     DBUG_RETURN(true);
   }
 
@@ -369,6 +370,7 @@ Transporter::connect_client(NdbSocket & socket)
     DBUG_PRINT("error", ("Socket %s is not valid",
                          socket.to_string().c_str()));
     DEBUG_FPRINTF((stderr, "Socket not valid\n"));
+    socket.close();
     DBUG_RETURN(false);
   }
 
@@ -404,6 +406,7 @@ Transporter::connect_client(NdbSocket & socket)
   if (helloLen < 0)
   {
     DBUG_PRINT("error", ("Failed to buffer hello %d", helloLen));
+    socket.close();
     DBUG_RETURN(false);
   }
   /**

storage/ndb/src/common/transporter/TransporterRegistry.cpp

@@ -130,21 +130,18 @@ SocketServer::Session * TransporterService::newSession(ndb_socket_t sockfd)
     if(r < SocketAuthenticator::AuthOk)
     {
       DEBUG_FPRINTF((stderr, "Failed to authenticate new session\n"));
-      secureSocket.close_with_reset(true); // Close with reset
-      DBUG_RETURN(0);
+      secureSocket.close_with_reset();
+      DBUG_RETURN(nullptr);
     }
   }
 
   BaseString msg;
-  bool close_with_reset = true;
   bool log_failure = false;
   if (!m_transporter_registry->connect_server(secureSocket,
                                               msg,
-                                              close_with_reset,
                                               log_failure))
   {
     DEBUG_FPRINTF((stderr, "New session failed in connect_server\n"));
-    secureSocket.close_with_reset(close_with_reset);
     if (log_failure)
     {
       g_eventLogger->warning("TR : %s", msg.c_str());
@@ -508,7 +505,6 @@ TransporterRegistry::init(TransporterReceiveHandle& recvhandle)
 bool
 TransporterRegistry::connect_server(NdbSocket & socket,
                                     BaseString & msg,
-                                    bool& close_with_reset,
                                     bool& log_failure)
 {
   DBUG_ENTER("TransporterRegistry::connect_server(sockfd)");
@@ -526,6 +522,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
                "read 'hello' from client");
     DBUG_PRINT("error", ("%s", msg.c_str()));
     DEBUG_FPRINTF((stderr, "%s", msg.c_str()));
+    socket.close_with_reset();
     DBUG_RETURN(false);
   }
 
@@ -555,6 +552,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
                "parse 'hello' from client.  >%s<", buf);
     DBUG_PRINT("error", ("%s", msg.c_str()));
     DEBUG_FPRINTF((stderr, "%s", msg.c_str()));
+    socket.close_with_reset();
     DBUG_RETURN(false);
   }
 
@@ -581,6 +579,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
     msg.assfmt("Ignored connection attempt as client "
                "nodeid %u out of range", nodeId);
     DBUG_PRINT("error", ("%s", msg.c_str()));
+    socket.close_with_reset();
     DBUG_RETURN(false);
   }
 
@@ -595,6 +594,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
                "nodeid %u is undefined.",
                nodeId);
     DBUG_PRINT("error", ("%s", msg.c_str()));
+    socket.close_with_reset();
     DBUG_RETURN(false);
   }
 
@@ -609,6 +609,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
                nodeId,
                remote_transporter_type,
                t->m_type);
+    socket.close_with_reset();
     DBUG_RETURN(false);
   }
 
@@ -627,6 +628,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
                  serverNodeId,
                  t->getLocalNodeId());
       DBUG_PRINT("error", ("%s", msg.c_str()));
+      socket.close_with_reset();
       DBUG_RETURN(false);
     }
   }
@@ -728,6 +730,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
         msg.assfmt("Ignored connection attempt from node %u as multi "
                    "transporter instance %d specified for non multi-transporter",
                    nodeId, multi_transporter_instance);
+        socket.close_with_reset();
         DBUG_RETURN(false);
       }
     }
@@ -783,6 +786,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
     {
       // Failed to request client close
       DBUG_PRINT("error", ("Failed to send client BYE"));
+      socket.close_with_reset();
       DBUG_RETURN(false);
     }
 
@@ -791,11 +795,12 @@ TransporterRegistry::connect_server(NdbSocket & socket,
     if (socket.read(read_eof_timeout, buf, sizeof(buf)) == 0)
     {
       // Client gracefully closed connection, turn off close_with_reset
-      close_with_reset = false;
+      socket.close();
       DBUG_RETURN(false);
     }
 
     // Failed to request client close
+    socket.close_with_reset();
     DBUG_RETURN(false);
   }
 
@@ -808,6 +813,7 @@ TransporterRegistry::connect_server(NdbSocket & socket,
                "reply to client node %u",
                nodeId);
     DBUG_PRINT("error", ("%s", msg.c_str()));
+    socket.close_with_reset();
     DBUG_RETURN(false);
   }
 

storage/ndb/src/common/util/NdbSocket.cpp

@@ -110,7 +110,7 @@ X509 * NdbSocket::peer_certificate() const {
   return SSL_get_peer_certificate(ssl);
 }
 
-int NdbSocket::set_nonblocking(int on) {
+int NdbSocket::set_nonblocking(int on) const {
   if(ssl) {
     if(on) {
       SSL_clear_mode(ssl, SSL_MODE_AUTO_RETRY);
@@ -230,6 +230,19 @@ bool NdbSocket::key_update_pending() const {
   return (bool) SSL_renegotiate_pending(ssl);
 }
 
+int NdbSocket::ssl_shutdown() const {
+  int err;
+  {
+    Guard2 guard(mutex);
+    set_nonblocking(false);
+    int r = SSL_shutdown(ssl);
+    if (r) return 0;
+    err = SSL_get_error(ssl, r);
+  }
+  Debug_Log("SSL_shutdown(): ERR %d", err);
+  return handle_ssl_error(err, "SSL_shutdown");
+}
+
 ssize_t NdbSocket::ssl_recv(char *buf, size_t len) const
 {
   bool r;

storage/ndb/src/common/util/SocketClient.cpp

@@ -221,11 +221,11 @@ SocketClient::authenticate(NdbSocket & secureSocket)
 {
   assert(m_auth);
   int r = m_auth->client_authenticate(secureSocket);
-  if (r < SocketAuthenticator::AuthOk)
+  if (r != SocketAuthenticator::AuthOk)
   {
     g_eventLogger->error("Socket authentication failed: %s\n",
                          m_auth->error(r));
-    secureSocket.close();
+    secureSocket.shutdown(); // Make it unusable, caller should close
   }
   return r;
 }

storage/ndb/src/common/util/SocketServer.cpp

@@ -414,11 +414,6 @@ sessionThread_C(void* _sc){
 
   if(!si->m_stop)
     si->runSession();
-  else
-  {
-    ndb_socket_close(si->m_socket);
-    ndb_socket_invalidate(&si->m_socket);
-  }
 
   // Mark the thread as stopped to allow the
   // session resources to be released

storage/ndb/src/mgmsrv/MgmtSrvr.cpp

@@ -5125,12 +5125,11 @@ MgmtSrvr::getConnectionDbParameter(int node1, int node2,
 bool
 MgmtSrvr::transporter_connect(NdbSocket & socket,
                               BaseString& msg,
-                              bool& close_with_reset,
                               bool& log_failure)
 {
   DBUG_ENTER("MgmtSrvr::transporter_connect");
   TransporterRegistry* tr= theFacade->get_registry();
-  if (!tr->connect_server(socket, msg, close_with_reset, log_failure))
+  if (!tr->connect_server(socket, msg, log_failure))
     DBUG_RETURN(false);
 
   /**

storage/ndb/src/mgmsrv/MgmtSrvr.hpp

@@ -358,7 +358,6 @@ class MgmtSrvr : private ConfigSubscriber, public trp_client {
 
   bool transporter_connect(NdbSocket & socket,
                            BaseString& errormsg,
-                           bool& close_with_reset,
                            bool& log_failure);
 
   SocketServer *get_socket_server() { return &m_socket_server; }

storage/ndb/src/mgmsrv/Services.cpp

@@ -1889,11 +1889,9 @@ void
 MgmApiSession::transporter_connect(Parser_t::Context &ctx,
 				   Properties const &args)
 {
-  bool close_with_reset = true;
   bool log_failure = false;
   BaseString errormsg;
-  if (!m_mgmsrv.transporter_connect(m_secure_socket,
-                                    errormsg, close_with_reset, log_failure))
+  if (!m_mgmsrv.transporter_connect(m_secure_socket, errormsg, log_failure))
   {
     // Connection not allowed or failed
     if (log_failure)
@@ -1903,18 +1901,6 @@ MgmApiSession::transporter_connect(Parser_t::Context &ctx,
                              name(),
                              errormsg.c_str());
     }
-    // Close the socket to indicate failure to client
-    m_secure_socket.close_with_reset(close_with_reset);
-  }
-  else
-  {
-    /*
-      Conversion to transporter succeeded
-      Stop this session thread and release resources
-      but don't close the socket, it's been taken over
-      by the transporter
-    */
-    NdbSocket s = std::move(m_secure_socket);
   }
 
   m_stop= true; // Stop the session