keepalives refactor

CMakeLists.txt

@@ -58,15 +58,15 @@ set(CMAKE_MACOSX_RPATH TRUE)
 # micro version is changed with a set of small changes or bugfixes anywhere in the project.
 set(LIBNETCONF2_MAJOR_VERSION 3)
 set(LIBNETCONF2_MINOR_VERSION 0)
-set(LIBNETCONF2_MICRO_VERSION 20)
+set(LIBNETCONF2_MICRO_VERSION 21)
 set(LIBNETCONF2_VERSION ${LIBNETCONF2_MAJOR_VERSION}.${LIBNETCONF2_MINOR_VERSION}.${LIBNETCONF2_MICRO_VERSION})
 
 # Version of the library
 # Major version is changed with every backward non-compatible API/ABI change in libyang, minor version changes
 # with backward compatible change and micro version is connected with any internal change of the library.
 set(LIBNETCONF2_MAJOR_SOVERSION 4)
 set(LIBNETCONF2_MINOR_SOVERSION 1)
-set(LIBNETCONF2_MICRO_SOVERSION 17)
+set(LIBNETCONF2_MICRO_SOVERSION 18)
 set(LIBNETCONF2_SOVERSION_FULL ${LIBNETCONF2_MAJOR_SOVERSION}.${LIBNETCONF2_MINOR_SOVERSION}.${LIBNETCONF2_MICRO_SOVERSION})
 set(LIBNETCONF2_SOVERSION ${LIBNETCONF2_MAJOR_SOVERSION})
 

src/server_config.c

@@ -1437,10 +1437,6 @@ nc_server_config_keepalives(const struct lyd_node *node, NC_OPERATION op)
         } else {
             endpt->ka.enabled = 0;
         }
-        ret = nc_sock_configure_ka(bind->sock, endpt->ka.enabled);
-        if (ret) {
-            goto cleanup;
-        }
     } else if (is_ch(node) && equal_parent_name(node, 1, "tcp-client-parameters")) {
         /* LOCK */
         if (nc_server_config_get_ch_client_with_lock(node, &ch_client)) {
@@ -1492,10 +1488,6 @@ nc_server_config_idle_time(const struct lyd_node *node, NC_OPERATION op)
             /* delete -> set to default */
             endpt->ka.idle_time = 7200;
         }
-        ret = nc_sock_configure_ka_idle_time(bind->sock, endpt->ka.idle_time);
-        if (ret) {
-            goto cleanup;
-        }
     } else if (is_ch(node) && equal_parent_name(node, 2, "tcp-client-parameters")) {
         /* LOCK */
         if (nc_server_config_get_ch_client_with_lock(node, &ch_client)) {
@@ -1548,10 +1540,6 @@ nc_server_config_max_probes(const struct lyd_node *node, NC_OPERATION op)
             /* delete -> set to default */
             endpt->ka.max_probes = 9;
         }
-        ret = nc_sock_configure_ka_max_probes(bind->sock, endpt->ka.max_probes);
-        if (ret) {
-            goto cleanup;
-        }
     } else if (is_ch(node) && equal_parent_name(node, 2, "tcp-client-parameters")) {
         /* LOCK */
         if (nc_server_config_get_ch_client_with_lock(node, &ch_client)) {
@@ -1604,10 +1592,6 @@ nc_server_config_probe_interval(const struct lyd_node *node, NC_OPERATION op)
             /* delete -> set to default */
             endpt->ka.probe_interval = 75;
         }
-        ret = nc_sock_configure_ka_probe_interval(bind->sock, endpt->ka.probe_interval);
-        if (ret) {
-            goto cleanup;
-        }
     } else if (is_ch(node) && equal_parent_name(node, 2, "tcp-client-parameters")) {
         /* LOCK */
         if (nc_server_config_get_ch_client_with_lock(node, &ch_client)) {

src/session.c

@@ -263,59 +263,39 @@ nc_is_pk_subject_public_key_info(const char *b64)
 #endif /* NC_ENABLED_SSH_TLS */
 
 int
-nc_sock_configure_ka(int sock, int enabled)
+nc_sock_configure_ka(int sock, const struct nc_keepalives *ka)
 {
-    if (setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, &enabled, sizeof enabled) == -1) {
+    int opt;
+
+    opt = ka->enabled;
+    if (setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, &opt, sizeof opt) == -1) {
         ERR(NULL, "Failed to set SO_KEEPALIVE (%s).", strerror(errno));
         return -1;
     }
 
-    return 0;
-}
-
-int
-nc_sock_configure_ka_idle_time(int sock, int idle_time)
-{
-    if (idle_time < 1) {
-        ERRARG(NULL, "idle_time");
+    if (!ka->enabled) {
+        return 0;
     }
 
 #ifdef TCP_KEEPIDLE
-    if (setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &idle_time, sizeof idle_time) == -1) {
+    opt = ka->idle_time;
+    if (setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &opt, sizeof opt) == -1) {
         ERR(NULL, "Failed to set TCP_KEEPIDLE (%s).", strerror(errno));
         return -1;
     }
 #endif
 
-    return 0;
-}
-
-int
-nc_sock_configure_ka_max_probes(int sock, int max_probes)
-{
-    if (max_probes < 1) {
-        ERRARG(NULL, "max_probes");
-    }
-
 #ifdef TCP_KEEPCNT
-    if (setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT, &max_probes, sizeof max_probes) == -1) {
+    opt = ka->max_probes;
+    if (setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT, &opt, sizeof opt) == -1) {
         ERR(NULL, "Failed to set TCP_KEEPCNT (%s).", strerror(errno));
         return -1;
     }
 #endif
 
-    return 0;
-}
-
-int
-nc_sock_configure_ka_probe_interval(int sock, int probe_interval)
-{
-    if (probe_interval < 1) {
-        ERRARG(NULL, "probe_interval");
-    }
-
 #ifdef TCP_KEEPINTVL
-    if (setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL, &probe_interval, sizeof probe_interval) == -1) {
+    opt = ka->probe_interval;
+    if (setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL, &opt, sizeof opt) == -1) {
         ERR(NULL, "Failed to set TCP_KEEPINTVL (%s).", strerror(errno));
         return -1;
     }

src/session_client.c

@@ -1548,7 +1548,7 @@ nc_saddr2str(const struct sockaddr *saddr, char **str_ip, uint16_t *port)
  * @return Connected socket or -1 on error.
  */
 static int
-sock_connect(int timeout_ms, int *sock_pending, struct addrinfo *res, struct nc_keepalives *ka)
+sock_connect(int timeout_ms, int *sock_pending, struct addrinfo *res, const struct nc_keepalives *ka)
 {
     int flags, ret, error;
     int sock = -1;
@@ -1624,20 +1624,9 @@ sock_connect(int timeout_ms, int *sock_pending, struct addrinfo *res, struct nc_
     }
 
     /* configure keepalives */
-    if (nc_sock_configure_ka(sock, ka->enabled)) {
+    if (nc_sock_configure_ka(sock, ka)) {
         goto cleanup;
     }
-    if (ka->enabled) {
-        if (nc_sock_configure_ka_idle_time(sock, ka->idle_time)) {
-            goto cleanup;
-        }
-        if (nc_sock_configure_ka_max_probes(sock, ka->max_probes)) {
-            goto cleanup;
-        }
-        if (nc_sock_configure_ka_probe_interval(sock, ka->probe_interval)) {
-            goto cleanup;
-        }
-    }
 
     /* connected */
     if (sock_pending) {
@@ -1748,7 +1737,7 @@ nc_client_ch_add_bind_listen(const char *address, uint16_t port, const char *hos
 
     NC_CHECK_ARG_RET(NULL, address, port, -1);
 
-    sock = nc_sock_listen_inet(address, port, &client_opts.ka);
+    sock = nc_sock_listen_inet(address, port);
     if (sock == -1) {
         return -1;
     }
@@ -1852,6 +1841,13 @@ nc_accept_callhome(int timeout, struct ly_ctx *ctx, struct nc_session **session)
         return sock;
     }
 
+    /* configure keepalives */
+    if (nc_sock_configure_ka(sock, &client_opts.ka)) {
+        free(host);
+        close(sock);
+        return -1;
+    }
+
     if (client_opts.ch_binds_aux[idx].ti == NC_TI_LIBSSH) {
         *session = nc_accept_callhome_ssh_sock(sock, host, port, ctx, NC_TRANSPORT_TIMEOUT);
     } else if (client_opts.ch_binds_aux[idx].ti == NC_TI_OPENSSL) {

src/session_p.h

@@ -830,37 +830,10 @@ int nc_poll(struct pollfd *pfd, uint16_t pfd_count, int timeout);
  * @brief Enables/disables TCP keepalives.
  *
  * @param[in] sock Socket to set this option for.
- * @param[in] enabled 1 to enable, 0 to disable keepalives.
+ * @param[in] ka Keepalives to set.
  * @return 0 on success, -1 on fail.
  */
-int nc_sock_configure_ka(int sock, int enabled);
-
-/**
- * @brief Set TCP keepalives idle time.
- *
- * @param[in] sock Socket to set this option for.
- * @param[in] idle_time Time in seconds before keepalive packets are sent.
- * @return 0 on success, -1 on fail.
- */
-int nc_sock_configure_ka_idle_time(int sock, int idle_time);
-
-/**
- * @brief Set TCP keepalives max probes.
- *
- * @param[in] sock Socket to set this option for.
- * @param[in] max_probes Maximum number of probes sent before dropping the connection.
- * @return 0 on success, -1 on fail.
- */
-int nc_sock_configure_ka_max_probes(int sock, int max_probes);
-
-/**
- * @brief Set TCP keepalives probe interval.
- *
- * @param[in] sock Socket to set this option for.
- * @param[in] probe_interval Time in seconds between keepalive probes.
- * @return 0 on success, -1 on fail.
- */
-int nc_sock_configure_ka_probe_interval(int sock, int probe_interval);
+int nc_sock_configure_ka(int sock, const struct nc_keepalives *ka);
 
 struct nc_session *nc_new_session(NC_SIDE side, int shared_ti);
 
@@ -968,10 +941,9 @@ int nc_sock_accept(int sock, int timeout, char **peer_host, uint16_t *peer_port)
  *
  * @param[in] address IP address to listen on.
  * @param[in] port Port to listen on.
- * @param[in] ka Keepalives parameters.
  * @return Listening socket, -1 on error.
  */
-int nc_sock_listen_inet(const char *address, uint16_t port, struct nc_keepalives *ka);
+int nc_sock_listen_inet(const char *address, uint16_t port);
 
 /**
  * @brief Accept a new connection on a listening socket.

src/session_server.c

@@ -270,7 +270,7 @@ nc_server_ch_set_dispatch_data(nc_server_ch_session_acquire_ctx_cb acquire_ctx_c
 #endif
 
 int
-nc_sock_listen_inet(const char *address, uint16_t port, struct nc_keepalives *ka)
+nc_sock_listen_inet(const char *address, uint16_t port)
 {
     int opt;
     int is_ipv4, sock;
@@ -302,22 +302,6 @@ nc_sock_listen_inet(const char *address, uint16_t port, struct nc_keepalives *ka
         goto fail;
     }
 
-    /* configure keepalives */
-    if (nc_sock_configure_ka(sock, ka->enabled)) {
-        goto fail;
-    }
-    if (ka->enabled) {
-        if (nc_sock_configure_ka_idle_time(sock, ka->idle_time)) {
-            goto fail;
-        }
-        if (nc_sock_configure_ka_max_probes(sock, ka->max_probes)) {
-            goto fail;
-        }
-        if (nc_sock_configure_ka_probe_interval(sock, ka->probe_interval)) {
-            goto fail;
-        }
-    }
-
     memset(&saddr, 0, sizeof(struct sockaddr_storage));
     if (is_ipv4) {
         saddr4 = (struct sockaddr_in *)&saddr;
@@ -1987,7 +1971,7 @@ nc_server_set_address_port(struct nc_endpt *endpt, struct nc_bind *bind, const c
         if (endpt->ti == NC_TI_UNIX) {
             sock = nc_sock_listen_unix(endpt->opts.unixsock);
         } else {
-            sock = nc_sock_listen_inet(address, port, &endpt->ka);
+            sock = nc_sock_listen_inet(address, port);
         }
 
         if (sock == -1) {
@@ -2252,7 +2236,7 @@ API NC_MSG_TYPE
 nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session)
 {
     NC_MSG_TYPE msgtype;
-    int sock, ret;
+    int sock = -1, ret;
     char *host = NULL;
     uint16_t port, bind_idx;
     struct timespec ts_cur;
@@ -2271,36 +2255,37 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session)
 
     if (!server_opts.endpt_count) {
         ERR(NULL, "No endpoints to accept sessions on.");
-        /* CONFIG UNLOCK */
-        pthread_rwlock_unlock(&server_opts.config_lock);
-        return NC_MSG_ERROR;
+        msgtype = NC_MSG_ERROR;
+        goto cleanup;
     }
 
     ret = nc_sock_accept_binds(server_opts.binds, server_opts.endpt_count, &server_opts.bind_lock, timeout, &host, &port, &bind_idx);
     if (ret < 1) {
-        free(host);
-        /* CONFIG UNLOCK */
-        pthread_rwlock_unlock(&server_opts.config_lock);
-        if (!ret) {
-            return NC_MSG_WOULDBLOCK;
-        }
-        return NC_MSG_ERROR;
+        msgtype = (!ret ? NC_MSG_WOULDBLOCK : NC_MSG_ERROR);
+        goto cleanup;
     }
-
     sock = ret;
 
+    /* configure keepalives */
+    if (nc_sock_configure_ka(sock, &server_opts.endpts[bind_idx].ka)) {
+        msgtype = NC_MSG_ERROR;
+        goto cleanup;
+    }
+
     *session = nc_new_session(NC_SERVER, 0);
-    NC_CHECK_ERRMEM_GOTO(!(*session), close(sock); free(host); msgtype = NC_MSG_ERROR, cleanup);
+    NC_CHECK_ERRMEM_GOTO(!(*session), msgtype = NC_MSG_ERROR, cleanup);
     (*session)->status = NC_STATUS_STARTING;
     (*session)->ctx = (struct ly_ctx *)ctx;
     (*session)->flags = NC_SESSION_SHAREDCTX;
     (*session)->host = host;
+    host = NULL;
     (*session)->port = port;
 
     /* sock gets assigned to session or closed */
 #ifdef NC_ENABLED_SSH_TLS
     if (server_opts.endpts[bind_idx].ti == NC_TI_LIBSSH) {
         ret = nc_accept_ssh_session(*session, server_opts.endpts[bind_idx].opts.ssh, sock, NC_TRANSPORT_TIMEOUT);
+        sock = -1;
         if (ret < 0) {
             msgtype = NC_MSG_ERROR;
             goto cleanup;
@@ -2311,6 +2296,7 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session)
     } else if (server_opts.endpts[bind_idx].ti == NC_TI_OPENSSL) {
         (*session)->data = server_opts.endpts[bind_idx].opts.tls;
         ret = nc_accept_tls_session(*session, server_opts.endpts[bind_idx].opts.tls, sock, NC_TRANSPORT_TIMEOUT);
+        sock = -1;
         if (ret < 0) {
             msgtype = NC_MSG_ERROR;
             goto cleanup;
@@ -2323,13 +2309,13 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session)
     if (server_opts.endpts[bind_idx].ti == NC_TI_UNIX) {
         (*session)->data = server_opts.endpts[bind_idx].opts.unixsock;
         ret = nc_accept_unix(*session, sock);
+        sock = -1;
         if (ret < 0) {
             msgtype = NC_MSG_ERROR;
             goto cleanup;
         }
     } else {
         ERRINT;
-        close(sock);
         msgtype = NC_MSG_ERROR;
         goto cleanup;
     }
@@ -2362,6 +2348,10 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session)
     /* CONFIG UNLOCK */
     pthread_rwlock_unlock(&server_opts.config_lock);
 
+    free(host);
+    if (sock > -1) {
+        close(sock);
+    }
     nc_session_free(*session, NULL);
     *session = NULL;
     return msgtype;

tests/CMakeLists.txt

@@ -70,13 +70,6 @@ if(ENABLE_SSH_TLS)
     libnetconf2_test(NAME test_crl)
     libnetconf2_test(NAME test_ch PORT_COUNT 2)
     libnetconf2_test(NAME test_runtime_changes PORT_COUNT 2)
-    libnetconf2_test(NAME test_client_ssh
-        WRAP_FUNCS connect ssh_connect ssh_userauth_none ssh_userauth_kbdint ssh_is_connected
-            ssh_channel_open_session ssh_channel_request_subsystem ssh_channel_is_close ssh_channel_write
-            ssh_channel_poll_timeout ssh_userauth_password nc_handshake_io nc_ctx_check_and_fill
-            ssh_userauth_try_publickey ssh_userauth_publickey nc_sock_listen_inet nc_sock_accept_binds nc_accept_callhome_ssh_sock)
-    libnetconf2_test(NAME test_client_tls
-        WRAP_FUNCS connect SSL_connect nc_send_hello_io nc_handshake_io nc_ctx_check_and_fill)
     libnetconf2_test(NAME test_authkeys)
     if (LIBPAM_HAVE_CONFDIR)
         libnetconf2_test(NAME test_pam WRAP_FUNCS pam_start)