ipcpd: Revised locking

The state lock was reverted to an rwlock to avoid interference of
management functions with the fast path. IPCPs now close without
calling unsafe functions in the signal handler.

3 files changed, 75 insertions, 86 deletions

diff --git a/src/ipcpd/normal/fmgr.c b/src/ipcpd/normal/fmgr.c
index 70afff37..1c8330a8 100644
--- a/src/ipcpd/normal/fmgr.c
+++ b/src/ipcpd/normal/fmgr.c
@@ -81,17 +81,16 @@ static void * fmgr_listen(void * o)
         char * ae_name;
 
         while (true) {
-                pthread_mutex_lock(&_ipcp->state_lock);
-                while (!(_ipcp->state == IPCP_ENROLLED ||
-                         _ipcp->state == IPCP_SHUTDOWN))
-                        pthread_cond_wait(&_ipcp->state_cond,
-                                          &_ipcp->state_lock);
-
-                if (_ipcp->state == IPCP_SHUTDOWN) {
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                ipcp_wait_state(_ipcp, IPCP_ENROLLED, NULL);
+
+                pthread_rwlock_rdlock(&_ipcp->state_lock);
+
+                if (ipcp_get_state(_ipcp) == IPCP_SHUTDOWN) {
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         return 0;
                 }
-                pthread_mutex_unlock(&_ipcp->state_lock);
+
+                pthread_rwlock_unlock(&_ipcp->state_lock);
 
                 fd = flow_accept(&ae_name);
                 if (fd < 0) {
diff --git a/src/ipcpd/normal/main.c b/src/ipcpd/normal/main.c
index 4173246d..9479b806 100644
--- a/src/ipcpd/normal/main.c
+++ b/src/ipcpd/normal/main.c
@@ -75,13 +75,11 @@ void ipcp_sig_handler(int sig, siginfo_t * info, void * c)
                         LOG_DBG("Terminating by order of %d. Bye.",
                                 info->si_pid);
 
-                        pthread_mutex_lock(&_ipcp->state_lock);
+                        pthread_rwlock_wrlock(&_ipcp->state_lock);
 
-                        ipcp_state_change(_ipcp, IPCP_SHUTDOWN);
+                        ipcp_set_state(_ipcp, IPCP_SHUTDOWN);
 
-                        pthread_mutex_unlock(&_ipcp->state_lock);
-
-                        pthread_cancel(normal_data(_ipcp)->mainloop);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
 
                         if (fmgr_fini())
                                 LOG_ERR("Failed to finalize flow manager.");
@@ -99,21 +97,15 @@ void ipcp_sig_handler(int sig, siginfo_t * info, void * c)
 
 static int normal_ipcp_name_reg(char * name)
 {
-        pthread_mutex_lock(&_ipcp->state_lock);
-
-        if (_ipcp->state != IPCP_ENROLLED) {
-                pthread_mutex_unlock(&_ipcp->state_lock);
-                LOG_ERR("Won't register with non-enrolled IPCP.");
-                return -1; /* -ENOTENROLLED */
-        }
+        pthread_rwlock_rdlock(&_ipcp->state_lock);
 
         if (ipcp_data_add_reg_entry(_ipcp->data, name)) {
-                pthread_mutex_unlock(&_ipcp->state_lock);
+                pthread_rwlock_unlock(&_ipcp->state_lock);
                 LOG_ERR("Failed to add %s to local registry.", name);
                 return -1;
         }
 
-        pthread_mutex_unlock(&_ipcp->state_lock);
+        pthread_rwlock_unlock(&_ipcp->state_lock);
 
         LOG_DBG("Registered %s.", name);
 
@@ -122,11 +114,11 @@ static int normal_ipcp_name_reg(char * name)
 
 static int normal_ipcp_name_unreg(char * name)
 {
-        pthread_mutex_lock(&_ipcp->state_lock);
+        pthread_rwlock_rdlock(&_ipcp->state_lock);
 
         ipcp_data_del_reg_entry(_ipcp->data, name);
 
-        pthread_mutex_unlock(&_ipcp->state_lock);
+        pthread_rwlock_unlock(&_ipcp->state_lock);
 
         return 0;
 }
@@ -135,62 +127,59 @@ static int normal_ipcp_enroll(char * dif_name)
 {
         struct timespec timeout = {(ENROLL_TIMEOUT / 1000),
                                    (ENROLL_TIMEOUT % 1000) * MILLION};
-        struct timespec abstime;
-
-        clock_gettime(PTHREAD_COND_CLOCK, &abstime);
-        ts_add(&abstime, &timeout, &abstime);
 
-        pthread_mutex_lock(&_ipcp->state_lock);
+        pthread_rwlock_rdlock(&_ipcp->state_lock);
 
-        if (_ipcp->state != IPCP_INIT) {
-                pthread_mutex_unlock(&_ipcp->state_lock);
+        if (ipcp_get_state(_ipcp) != IPCP_INIT) {
+                pthread_rwlock_unlock(&_ipcp->state_lock);
                 LOG_ERR("Won't enroll an IPCP that is not in INIT.");
                 return -1; /* -ENOTINIT */
         }
 
-        pthread_mutex_unlock(&_ipcp->state_lock);
-
         if (fmgr_mgmt_flow(dif_name)) {
-                pthread_mutex_unlock(&_ipcp->state_lock);
+                pthread_rwlock_unlock(&_ipcp->state_lock);
                 LOG_ERR("Failed to establish management flow.");
                 return -1;
         }
 
-        pthread_mutex_lock(&_ipcp->state_lock);
-        while (_ipcp->state != IPCP_ENROLLED)
-                if (pthread_cond_timedwait(&_ipcp->state_cond,
-                                           &_ipcp->state_lock,
-                                           &abstime) == ETIMEDOUT) {
-                        pthread_mutex_unlock(&_ipcp->state_lock);
-                        LOG_ERR("Enrollment didn't complete in time.");
-                        return -1;
-                }
-        pthread_mutex_unlock(&_ipcp->state_lock);
+        pthread_rwlock_unlock(&_ipcp->state_lock);
+
+        if (ipcp_wait_state(_ipcp, IPCP_ENROLLED, &timeout) == -ETIMEDOUT) {
+                LOG_ERR("Enrollment timed out.");
+                return -1;
+        }
+
+        pthread_rwlock_rdlock(&_ipcp->state_lock);
+
+        if (ipcp_get_state(_ipcp) != IPCP_ENROLLED) {
+                pthread_rwlock_unlock(&_ipcp->state_lock);
+                return -1;
+        }
 
         return 0;
 }
 
 static int normal_ipcp_bootstrap(struct dif_config * conf)
 {
-        pthread_mutex_lock(&_ipcp->state_lock);
+        pthread_rwlock_wrlock(&_ipcp->state_lock);
 
-        if (_ipcp->state != IPCP_INIT) {
-                pthread_mutex_unlock(&_ipcp->state_lock);
+        if (ipcp_get_state(_ipcp) != IPCP_INIT) {
+                pthread_rwlock_unlock(&_ipcp->state_lock);
                 LOG_ERR("Won't bootstrap an IPCP that is not in INIT.");
                 return -1; /* -ENOTINIT */
         }
 
         if (ribmgr_bootstrap(conf)) {
-                pthread_mutex_unlock(&_ipcp->state_lock);
+                pthread_rwlock_unlock(&_ipcp->state_lock);
                 LOG_ERR("Failed to bootstrap RIB manager.");
                 return -1;
         }
 
-        ipcp_state_change(_ipcp, IPCP_ENROLLED);
+        ipcp_set_state(_ipcp, IPCP_ENROLLED);
 
         _ipcp->data->dif_name = conf->dif_name;
 
-        pthread_mutex_unlock(&_ipcp->state_lock);
+        pthread_rwlock_unlock(&_ipcp->state_lock);
 
         LOG_DBG("Bootstrapped in DIF %s.", conf->dif_name);
 
@@ -247,9 +236,9 @@ void normal_ipcp_data_destroy()
         if (_ipcp == NULL)
                 return;
 
-        pthread_mutex_lock(&_ipcp->state_lock);
+        pthread_rwlock_rdlock(&_ipcp->state_lock);
 
-        if (_ipcp->state != IPCP_SHUTDOWN)
+        if (ipcp_get_state(_ipcp) != IPCP_SHUTDOWN)
                 LOG_WARN("Cleaning up while not in shutdown.");
 
         if (normal_data(_ipcp)->dum != NULL)
@@ -257,7 +246,7 @@ void normal_ipcp_data_destroy()
         if (normal_data(_ipcp)->rb != NULL)
                 shm_ap_rbuff_close(normal_data(_ipcp)->rb);
 
-        pthread_mutex_unlock(&_ipcp->state_lock);
+        pthread_rwlock_unlock(&_ipcp->state_lock);
 
         ipcp_data_destroy(_ipcp->data);
 }
@@ -331,8 +320,6 @@ int main(int argc, char * argv[])
                 exit(EXIT_FAILURE);
         }
 
-        pthread_mutex_lock(&_ipcp->state_lock);
-
         pthread_sigmask(SIG_BLOCK, &sigset, NULL);
 
         pthread_create(&normal_data(_ipcp)->mainloop, NULL,
@@ -340,8 +327,6 @@ int main(int argc, char * argv[])
 
         pthread_sigmask(SIG_UNBLOCK, &sigset, NULL);
 
-        pthread_mutex_unlock(&_ipcp->state_lock);
-
         if (ipcp_create_r(getpid())) {
                 LOG_ERR("Failed to notify IRMd we are initialized.");
                 normal_ipcp_data_destroy();
diff --git a/src/ipcpd/normal/ribmgr.c b/src/ipcpd/normal/ribmgr.c
index c8d517b5..9b61e180 100644
--- a/src/ipcpd/normal/ribmgr.c
+++ b/src/ipcpd/normal/ribmgr.c
@@ -254,15 +254,15 @@ int ribmgr_cdap_write(struct cdap * instance,
         static_info_msg_t * msg;
         int ret = 0;
 
-        pthread_mutex_lock(&_ipcp->state_lock);
-        if (_ipcp->state == IPCP_PENDING_ENROLL &&
+        pthread_rwlock_wrlock(&_ipcp->state_lock);
+        if (ipcp_get_state(_ipcp) == IPCP_PENDING_ENROLL &&
             strcmp(name, STATIC_INFO) == 0) {
                 LOG_DBG("Received static DIF information.");
 
                 msg = static_info_msg__unpack(NULL, len, data);
                 if (msg == NULL) {
-                        ipcp_state_change(_ipcp, IPCP_INIT);
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        ipcp_set_state(_ipcp, IPCP_INIT);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         cdap_send_reply(instance, invoke_id, -1, NULL, 0);
                         LOG_ERR("Failed to unpack static info message.");
                         return -1;
@@ -280,8 +280,8 @@ int ribmgr_cdap_write(struct cdap * instance,
                 rib->address = msg->address;
 
                 if (frct_init(&rib->dtc, rib->address)) {
-                        ipcp_state_change(_ipcp, IPCP_INIT);
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        ipcp_set_state(_ipcp, IPCP_INIT);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         cdap_send_reply(instance, invoke_id, -1, NULL, 0);
                         static_info_msg__free_unpacked(msg, NULL);
                         LOG_ERR("Failed to init FRCT");
@@ -289,9 +289,11 @@ int ribmgr_cdap_write(struct cdap * instance,
                 }
 
                 static_info_msg__free_unpacked(msg, NULL);
-        } else
+        } else {
                 ret = -1;
-        pthread_mutex_unlock(&_ipcp->state_lock);
+        }
+
+        pthread_rwlock_unlock(&_ipcp->state_lock);
 
         if (cdap_send_reply(instance, invoke_id, ret, NULL, 0)) {
                 LOG_ERR("Failed to send reply to write request.");
@@ -332,13 +334,13 @@ int ribmgr_cdap_start(struct cdap * instance,
         size_t len = 0;
         int iid = 0;
 
-        pthread_mutex_lock(&_ipcp->state_lock);
-        if (_ipcp->state == IPCP_ENROLLED &&
+        pthread_rwlock_wrlock(&_ipcp->state_lock);
+        if (ipcp_get_state(_ipcp) == IPCP_ENROLLED &&
             strcmp(name, ENROLLMENT) == 0) {
                 LOG_DBG("New enrollment request.");
 
                 if (cdap_send_reply(instance, invoke_id, 0, NULL, 0)) {
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         LOG_ERR("Failed to send reply to enrollment request.");
                         return -1;
                 }
@@ -357,14 +359,14 @@ int ribmgr_cdap_start(struct cdap * instance,
 
                 len = static_info_msg__get_packed_size(&stat_info);
                 if (len == 0) {
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         LOG_ERR("Failed to get size of static information.");
                         return -1;
                 }
 
                 data = malloc(len);
                 if (data == NULL) {
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         LOG_ERR("Failed to allocate memory.");
                         return -1;
                 }
@@ -378,7 +380,7 @@ int ribmgr_cdap_start(struct cdap * instance,
                 iid = cdap_send_write(instance, STATIC_INFO, data, len, 0);
                 if (iid < 0) {
                         pthread_mutex_unlock(&rib->cdap_reqs_lock);
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         free(data);
                         LOG_ERR("Failed to send static information.");
                         return -1;
@@ -386,7 +388,7 @@ int ribmgr_cdap_start(struct cdap * instance,
 
                 if (cdap_result_wait(instance, WRITE, STATIC_INFO, iid)) {
                         pthread_mutex_unlock(&rib->cdap_reqs_lock);
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         free(data);
                         LOG_ERR("Remote did not receive static information.");
                         return -1;
@@ -402,7 +404,7 @@ int ribmgr_cdap_start(struct cdap * instance,
                 iid = cdap_send_stop(instance, ENROLLMENT);
                 if (iid < 0) {
                         pthread_mutex_unlock(&rib->cdap_reqs_lock);
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         free(data);
                         LOG_ERR("Failed to send stop of enrollment.");
                         return -1;
@@ -410,7 +412,7 @@ int ribmgr_cdap_start(struct cdap * instance,
 
                 if (cdap_result_wait(instance, STOP, ENROLLMENT, iid)) {
                         pthread_mutex_unlock(&rib->cdap_reqs_lock);
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         free(data);
                         LOG_ERR("Remote failed to complete enrollment.");
                         return -1;
@@ -420,12 +422,12 @@ int ribmgr_cdap_start(struct cdap * instance,
                 free(data);
         } else {
                 if (cdap_send_reply(instance, invoke_id, -1, NULL, 0)) {
-                        pthread_mutex_unlock(&_ipcp->state_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         LOG_ERR("Failed to send reply to start request.");
                         return -1;
                 }
         }
-        pthread_mutex_unlock(&_ipcp->state_lock);
+        pthread_rwlock_unlock(&_ipcp->state_lock);
 
         return 0;
 }
@@ -436,21 +438,21 @@ int ribmgr_cdap_stop(struct cdap * instance,
 {
         int ret = 0;
 
-        pthread_mutex_lock(&_ipcp->state_lock);
-        if (_ipcp->state == IPCP_PENDING_ENROLL &&
+        pthread_rwlock_wrlock(&_ipcp->state_lock);
+        if (ipcp_get_state(_ipcp) == IPCP_PENDING_ENROLL &&
             strcmp(name, ENROLLMENT) == 0) {
                 LOG_DBG("Stop enrollment received.");
 
-                ipcp_state_change(_ipcp, IPCP_ENROLLED);
+                ipcp_set_state(_ipcp, IPCP_ENROLLED);
         } else
                 ret = -1;
 
         if (cdap_send_reply(instance, invoke_id, ret, NULL, 0)) {
-                pthread_mutex_unlock(&_ipcp->state_lock);
+                pthread_rwlock_unlock(&_ipcp->state_lock);
                 LOG_ERR("Failed to send reply to stop request.");
                 return -1;
         }
-        pthread_mutex_unlock(&_ipcp->state_lock);
+        pthread_rwlock_unlock(&_ipcp->state_lock);
 
         return 0;
 }
@@ -486,11 +488,11 @@ int ribmgr_add_flow(int fd)
         flow->instance = instance;
         flow->fd = fd;
 
-        pthread_mutex_lock(&_ipcp->state_lock);
+        pthread_rwlock_wrlock(&_ipcp->state_lock);
         pthread_rwlock_wrlock(&rib->flows_lock);
         if (list_empty(&rib->flows) &&
-            _ipcp->state == IPCP_INIT) {
-                ipcp_state_change(_ipcp, IPCP_PENDING_ENROLL);
+            ipcp_get_state(_ipcp) == IPCP_INIT) {
+                ipcp_set_state(_ipcp, IPCP_PENDING_ENROLL);
 
                 pthread_mutex_lock(&rib->cdap_reqs_lock);
                 iid = cdap_send_start(instance,
@@ -498,6 +500,7 @@ int ribmgr_add_flow(int fd)
                 if (iid < 0) {
                         pthread_mutex_unlock(&rib->cdap_reqs_lock);
                         pthread_rwlock_unlock(&rib->flows_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         LOG_ERR("Failed to start enrollment.");
                         cdap_destroy(instance);
                         free(flow);
@@ -507,6 +510,7 @@ int ribmgr_add_flow(int fd)
                 if (cdap_result_wait(instance, START, ENROLLMENT, iid)) {
                         pthread_mutex_unlock(&rib->cdap_reqs_lock);
                         pthread_rwlock_unlock(&rib->flows_lock);
+                        pthread_rwlock_unlock(&_ipcp->state_lock);
                         LOG_ERR("Failed to start enrollment.");
                         cdap_destroy(instance);
                         free(flow);
@@ -514,10 +518,10 @@ int ribmgr_add_flow(int fd)
                 }
                 pthread_mutex_unlock(&rib->cdap_reqs_lock);
         }
-        pthread_mutex_unlock(&_ipcp->state_lock);
 
         list_add(&flow->next, &rib->flows);
         pthread_rwlock_unlock(&rib->flows_lock);
+        pthread_rwlock_unlock(&_ipcp->state_lock);
 
         return 0;
 }
@@ -534,6 +538,7 @@ int ribmgr_remove_flow(int fd)
                         if (cdap_destroy(flow->instance))
                                 LOG_ERR("Failed to destroy CDAP instance.");
                         list_del(&flow->next);
+                        pthread_rwlock_unlock(&rib->flows_lock);
                         free(flow);
                         return 0;
                 }