session server UPDATE remove endpoint socket retries

Use CH configuration max attempts instead.
Fixes CESNET/netopeer2#1278

Author: michalvasko

src/session_client.c

@@ -1399,12 +1399,18 @@ nc_connect_unix(const char *address, struct ly_ctx *ctx)
     return NULL;
 }
 
-/*
-   Helper for a non-blocking connect (which is required because of the locking
-   concept for e.g. call home settings). For more details see nc_sock_connect().
+/**
+ * @brief Try to connect a socket, optionally a pending one from a previous attempt.
+ *
+ * @param[in] timeout_ms Timeout in ms to wait for the connection to be fully established, -1 to block.
+ * @param[in,out] sock_pending Optional previously created socked that was not fully connected yet. If provided and
+ * connected, is set to -1.
+ * @param[in] res Addrinfo resource to use when creating a new socket.
+ * @param[in] ka Keepalives to set.
+ * @return Connected socket or -1 on error.
  */
 static int
-_non_blocking_connect(int timeout, int *sock_pending, struct addrinfo *res, struct nc_keepalives *ka)
+sock_connect(int timeout_ms, int *sock_pending, struct addrinfo *res, struct nc_keepalives *ka)
 {
     int flags, ret, error;
     int sock = -1;
@@ -1453,20 +1459,23 @@ _non_blocking_connect(int timeout, int *sock_pending, struct addrinfo *res, stru
             }
         }
     }
-    ts.tv_sec = timeout;
-    ts.tv_usec = 0;
 
     FD_ZERO(&wset);
     FD_SET(sock, &wset);
 
-    if ((ret = select(sock + 1, NULL, &wset, NULL, (timeout != -1) ? &ts : NULL)) < 0) {
+    /* wait for some data on the socket */
+    if (timeout_ms != -1) {
+        ts.tv_sec = timeout_ms / 1000;
+        ts.tv_usec = (timeout_ms % 1000) * 1000;
+    }
+    if ((ret = select(sock + 1, NULL, &wset, NULL, (timeout_ms != -1) ? &ts : NULL)) < 0) {
         ERR(NULL, "select() failed (%s).", strerror(errno));
         goto cleanup;
     }
 
     if (ret == 0) {
         /* there was a timeout */
-        VRB(NULL, "Timed out after %ds (%s).", timeout, strerror(errno));
+        VRB(NULL, "Timed out after %d ms (%s).", timeout_ms, strerror(errno));
         if (sock_pending) {
             /* no sock-close, we'll try it again */
             *sock_pending = sock;
@@ -1494,6 +1503,10 @@ _non_blocking_connect(int timeout, int *sock_pending, struct addrinfo *res, stru
         goto cleanup;
     }
 
+    /* connected */
+    if (sock_pending) {
+        *sock_pending = -1;
+    }
     return sock;
 
 cleanup:
@@ -1504,24 +1517,16 @@ _non_blocking_connect(int timeout, int *sock_pending, struct addrinfo *res, stru
     return -1;
 }
 
-/* A given timeout value limits the time how long the function blocks. If it has to block
-   only for some seconds, a socket connection might not yet have been fully established.
-   Therefore the active (pending) socket will be stored in *sock_pending, but the return
-   value will be -1. In such a case a subsequent invokation is required, by providing the
-   stored sock_pending, again.
-   In general, if this function returns -1, when a timeout has been given, this function
-   has to be invoked, until it returns a valid socket.
- */
 int
-nc_sock_connect(const char *host, uint16_t port, int timeout, struct nc_keepalives *ka, int *sock_pending, char **ip_host)
+nc_sock_connect(const char *host, uint16_t port, int timeout_ms, struct nc_keepalives *ka, int *sock_pending, char **ip_host)
 {
     int i, opt;
     int sock = sock_pending ? *sock_pending : -1;
     struct addrinfo hints, *res_list = NULL, *res;
     char *buf, port_s[6]; /* length of string representation of short int */
     void *addr;
 
-    DBG(NULL, "nc_sock_connect(%s, %u, %d, %d)", host, port, timeout, sock);
+    DBG(NULL, "nc_sock_connect(%s, %u, %d, %d)", host, port, timeout_ms, sock);
 
     /* no pending socket */
     if (sock == -1) {
@@ -1538,7 +1543,7 @@ nc_sock_connect(const char *host, uint16_t port, int timeout, struct nc_keepaliv
         }
 
         for (res = res_list; res != NULL; res = res->ai_next) {
-            sock = _non_blocking_connect(timeout, sock_pending, res, ka);
+            sock = sock_connect(timeout_ms, sock_pending, res, ka);
             if (sock == -1) {
                 if (!sock_pending || (*sock_pending == -1)) {
                     /* try the next resource */
@@ -1582,7 +1587,7 @@ nc_sock_connect(const char *host, uint16_t port, int timeout, struct nc_keepaliv
     } else {
         /* try to get a connection with the pending socket */
         assert(sock_pending);
-        sock = _non_blocking_connect(timeout, sock_pending, NULL, ka);
+        sock = sock_connect(timeout_ms, sock_pending, NULL, ka);
     }
 
     return sock;

src/session_p.h

@@ -277,7 +277,6 @@ struct nc_server_opts {
             char *address;
             uint16_t port;
             int sock_pending;
-            int sock_retries;
             struct nc_keepalives ka;
 
             union {
@@ -350,19 +349,14 @@ struct nc_server_opts {
 #define NC_CH_NO_ENDPT_WAIT 1000
 
 /**
- * Number of sockets kept waiting to be accepted.
+ * Timeout in msec for a Call Home socket to establish its connection.
  */
-#define NC_REVERSE_QUEUE 5
+#define NC_CH_CONNECT_TIMEOUT 500
 
 /**
- * Timeout for connecting Call Home socket to a client (s).
- */
-#define NC_SOCKET_CH_TIMEOUT 5
-
-/**
- * Number of retires of connection Call Home socket to a client.
+ * Number of sockets kept waiting to be accepted.
  */
-#define NC_SOCKET_CH_RETRIES 5
+#define NC_REVERSE_QUEUE 5
 
 /**
  * @brief Type of the session
@@ -662,13 +656,15 @@ NC_MSG_TYPE nc_handshake_io(struct nc_session *session);
  *
  * @param[in] host Hostname to connect to.
  * @param[in] port Port to connect on.
- * @param[in] timeout for blocking the connect+select call (-1 for infinite).
+ * @param[in] timeout_ms Timeout in ms for blocking the connect + select call (-1 for infinite).
  * @param[in] ka Keepalives parameters.
- * @param[in,out] sock_pending for exchanging the pending socket, if the blocking timeout was != -1
+ * @param[in,out] sock_pending Previous pending socket. If set, equal to -1, and the connection is still in progress
+ * after @p timeout, it is set to the pending socket but -1 is returned. If NULL, the socket is closed on timeout.
  * @param[out] ip_host Optional parameter with string IP address of the connected host.
  * @return Connected socket or -1 on error.
  */
-int nc_sock_connect(const char *host, uint16_t port, int timeout, struct nc_keepalives *ka, int *sock_pending, char **ip_host);
+int nc_sock_connect(const char *host, uint16_t port, int timeout_ms, struct nc_keepalives *ka, int *sock_pending,
+        char **ip_host);
 
 /**
  * @brief Accept a new socket connection.

src/session_server.c

@@ -3295,7 +3295,18 @@ nc_server_ch_client_set_max_attempts(const char *client_name, uint8_t max_attemp
     return 0;
 }
 
-/* client lock is expected to be held */
+/**
+ * @brief Create a connection for an endpoint.
+ *
+ * Client lock is expected to be held.
+ *
+ * @param[in] endpt Endpoint to use.
+ * @param[in] acquire_ctx_cb Callback for acquiring the libyang context.
+ * @param[in] release_ctx_cb Callback for releasing the libyang context.
+ * @param[in] ctx_cb_data Context callbacks data.
+ * @param[out] session Created NC session.
+ * @return NC_MSG values.
+ */
 static NC_MSG_TYPE
 nc_connect_ch_endpt(struct nc_ch_endpt *endpt, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb,
         nc_server_ch_session_release_ctx_cb release_ctx_cb, void *ctx_cb_data, struct nc_session **session)
@@ -3306,21 +3317,10 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, nc_server_ch_session_acquire_ctx_
     struct timespec ts_cur;
     char *ip_host;
 
-    sock = nc_sock_connect(endpt->address, endpt->port, NC_SOCKET_CH_TIMEOUT, &endpt->ka, &endpt->sock_pending, &ip_host);
+    sock = nc_sock_connect(endpt->address, endpt->port, NC_CH_CONNECT_TIMEOUT, &endpt->ka, &endpt->sock_pending, &ip_host);
     if (sock < 0) {
-        if (endpt->sock_pending > -1) {
-            ++endpt->sock_retries;
-            if (endpt->sock_retries == NC_SOCKET_CH_RETRIES) {
-                ERR(NULL, "Failed to connect socket %d after %d retries, closing.", endpt->sock_pending, NC_SOCKET_CH_RETRIES);
-                close(endpt->sock_pending);
-                endpt->sock_pending = -1;
-                endpt->sock_retries = 0;
-            }
-        }
         return NC_MSG_ERROR;
     }
-    /* no need to store the socket as pending any longer */
-    endpt->sock_pending = -1;
 
     /* acquire context */
     ctx = acquire_ctx_cb(ctx_cb_data);
@@ -3548,8 +3548,10 @@ nc_ch_client_thread(void *arg)
     cur_endpt = &client->ch_endpts[0];
     cur_endpt_name = strdup(cur_endpt->name);
 
-    VRB(NULL, "Call Home client \"%s\" connecting...", data->client_name);
     while (1) {
+        if (!cur_attempts) {
+            VRB(NULL, "Call Home client \"%s\" endpoint \"%s\" connecting...", data->client_name, cur_endpt_name);
+        }
         msgtype = nc_connect_ch_endpt(cur_endpt, data->acquire_ctx_cb, data->release_ctx_cb, data->ctx_cb_data, &session);
 
         if (msgtype == NC_MSG_HELLO) {
@@ -3648,18 +3650,28 @@ nc_ch_client_thread(void *arg)
 
             if (next_endpt_index >= client->ch_endpt_count) {
                 /* endpoint was removed, start with the first one */
+                VRB(NULL, "Call Home client \"%s\" endpoint \"%s\" removed.", data->client_name, cur_endpt_name);
                 next_endpt_index = 0;
                 cur_attempts = 0;
             } else if (cur_attempts == client->max_attempts) {
                 /* we have tried to connect to this endpoint enough times */
+                VRB(NULL, "Call Home client \"%s\" endpoint \"%s\" failed connection attempt limit %" PRIu8 " reached.",
+                        data->client_name, cur_endpt_name, client->max_attempts);
+
+                /* clear a pending socket, if any */
+                cur_endpt = &client->ch_endpts[next_endpt_index];
+                if (cur_endpt->sock_pending > -1) {
+                    close(cur_endpt->sock_pending);
+                    cur_endpt->sock_pending = -1;
+                }
+
                 if (next_endpt_index < client->ch_endpt_count - 1) {
                     /* just go to the next endpoint */
                     ++next_endpt_index;
                 } else {
                     /* cur_endpoint is the last, start with the first one */
                     next_endpt_index = 0;
                 }
-
                 cur_attempts = 0;
             } /* else we keep the current one */
         }