# HG changeset patch # User 'Lennart Lund' # Date 1461124487 -25200 # Wed Apr 20 10:54:47 2016 +0700 # Node ID 1bc165ff8bb8570c1311856c791c51e0d6f900c9 # Parent 99f1d25bb6968ecea2ed9dbec171b34137ae5aef log: add mutex protection for common resource in log agent [#1705] There was an race condition between client thread and recovery thread accessing to common resource `client_list`. Add mutex protection. diff --git a/osaf/libs/agents/saf/lga/lga.h b/osaf/libs/agents/saf/lga/lga.h --- a/osaf/libs/agents/saf/lga/lga.h +++ b/osaf/libs/agents/saf/lga/lga.h @@ -89,7 +89,7 @@ typedef enum { */ LGS_UP /* Server is up */ -} lgs_state_t; +} lgs_state_e; /* Agent internal states */ typedef enum { @@ -104,7 +104,7 @@ typedef enum { LGA_RECOVERY2 /* Auto recover remaining clients and streams * After recovery1 timeout */ -} lga_state_t; +} lga_state_e; /* * The LGA control block is the master anchor structure for all LGA * instantiations within a process. @@ -114,8 +114,7 @@ typedef struct { lga_client_hdl_rec_t *client_list; /* LGA client handle database */ MDS_HDL mds_hdl; /* MDS handle */ MDS_DEST lgs_mds_dest; /* LGS absolute/virtual address */ - lgs_state_t lgs_state; /* Indicate current server MDS state */ - lga_state_t lga_state; /* Indicate current state of the agent */ + lgs_state_e lgs_state; /* Indicate current server MDS state */ /* LGS LGA sync params */ int lgs_sync_awaited; NCS_SEL_OBJ lgs_sync_sel; @@ -124,6 +123,11 @@ typedef struct { /* lga_saf_api.c */ extern lga_cb_t lga_cb; +typedef struct { + lga_state_e state; + pthread_mutex_t lock; +} lga_state_t; + /* lga_mds.c */ extern uint32_t lga_mds_init(lga_cb_t *cb); extern void lga_mds_finalize(lga_cb_t *cb); diff --git a/osaf/libs/agents/saf/lga/lga_api.c b/osaf/libs/agents/saf/lga/lga_api.c --- a/osaf/libs/agents/saf/lga/lga_api.c +++ b/osaf/libs/agents/saf/lga/lga_api.c @@ -41,7 +41,6 @@ /* The main controle block */ lga_cb_t lga_cb = { .cb_lock = PTHREAD_MUTEX_INITIALIZER, - .lga_state = LGA_NORMAL, .lgs_state = LGS_START }; @@ -124,6 +123,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT SaAisErrorT ais_rc = SA_AIS_OK; int rc; uint32_t client_id = 0; + bool is_locked = false; TRACE_ENTER(); @@ -155,12 +155,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT * Synchronize with mds and recovery thread (mutex) * NOTE: Nothing to handle if recovery state 1 */ - pthread_mutex_lock(&lga_cb.cb_lock); - lgs_state_t lgs_state = lga_cb.lgs_state; - lga_state_t lga_state = lga_cb.lga_state; - pthread_mutex_unlock(&lga_cb.cb_lock); - - if (lgs_state == LGS_NO_ACTIVE) { + if (is_lgs_state(LGS_NO_ACTIVE)) { /* We have a server but it is temporary unavailable. Client may * try again */ @@ -169,7 +164,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT goto done; } - if (lga_state == LGA_NO_SERVER) { + if (is_lga_state(LGA_NO_SERVER)) { /* We have no server and cannot initialize. * The client may try again */ @@ -178,7 +173,10 @@ SaAisErrorT saLogInitialize(SaLogHandleT goto done; } - if (lga_state == LGA_RECOVERY2) { + /* Synchronize b/w client/recovery thread */ + recovery2_lock(&is_locked); + + if (is_lga_state(LGA_RECOVERY2)) { /* Auto recovery is ongoing. We have to wait for it to finish. * The client may try again */ @@ -252,6 +250,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT } done: + recovery2_unlock(&is_locked); TRACE_LEAVE2("client_id = %d", client_id); return ais_rc; } @@ -451,6 +450,7 @@ SaAisErrorT saLogFinalize(SaLogHandleT l lga_client_hdl_rec_t *hdl_rec; SaAisErrorT ais_rc = SA_AIS_OK; uint32_t rc; + bool is_locked = false; TRACE_ENTER(); @@ -466,12 +466,7 @@ SaAisErrorT saLogFinalize(SaLogHandleT l * Handle states * Synchronize with mds and recovery thread (mutex) */ - pthread_mutex_lock(&lga_cb.cb_lock); - lgs_state_t lgs_state = lga_cb.lgs_state; - lga_state_t lga_state = lga_cb.lga_state; - pthread_mutex_unlock(&lga_cb.cb_lock); - - if (lgs_state == LGS_NO_ACTIVE) { + if (is_lgs_state(LGS_NO_ACTIVE)) { /* We have a server but it is temporary unavailable. Client may * try again */ @@ -480,7 +475,7 @@ SaAisErrorT saLogFinalize(SaLogHandleT l goto done_give_hdl; } - if (lga_state == LGA_NO_SERVER) { + if (is_lga_state(LGA_NO_SERVER)) { /* We have no server but can still finalize client. * In this situation no message to server is sent */ @@ -489,7 +484,10 @@ SaAisErrorT saLogFinalize(SaLogHandleT l goto done_give_hdl; } - if (lga_state == LGA_RECOVERY2) { + /* Synchronize b/w client/recovery thread */ + recovery2_lock(&is_locked); + + if (is_lga_state(LGA_RECOVERY2)) { /* Auto recovery is ongoing. We have to wait for it to finish. * The client may try again */ @@ -498,7 +496,7 @@ SaAisErrorT saLogFinalize(SaLogHandleT l goto done_give_hdl; } - if (lga_state == LGA_RECOVERY1) { + if (is_lga_state(LGA_RECOVERY1)) { /* We are in recovery state 1. Client may or may not have been * initialized. If initialized a finalize request must be sent * to the server else the client is finalized in the agent only @@ -520,7 +518,7 @@ SaAisErrorT saLogFinalize(SaLogHandleT l if (ais_rc == SA_AIS_OK) { TRACE("%s delete_one_client", __FUNCTION__); - (void) delete_one_client(&lga_cb.client_list, hdl_rec); + (void) lga_hdl_rec_del(&lga_cb.client_list, hdl_rec); } done_give_hdl: @@ -533,6 +531,7 @@ SaAisErrorT saLogFinalize(SaLogHandleT l } done: + recovery2_unlock(&is_locked); TRACE_LEAVE2("ais_rc = %s", saf_error(ais_rc)); return ais_rc; } @@ -740,6 +739,7 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl uint32_t timeout; uint32_t log_stream_id; uint32_t log_header_type = 0; + bool is_locked = false; TRACE_ENTER(); @@ -760,12 +760,7 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl * Handle states * Synchronize with mds and recovery thread (mutex) */ - pthread_mutex_lock(&lga_cb.cb_lock); - lgs_state_t lgs_state = lga_cb.lgs_state; - lga_state_t lga_state = lga_cb.lga_state; - pthread_mutex_unlock(&lga_cb.cb_lock); - - if (lgs_state == LGS_NO_ACTIVE) { + if (is_lgs_state(LGS_NO_ACTIVE)) { /* We have a server but it is temporary unavailable. Client may * try again */ @@ -774,7 +769,7 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl goto done_give_hdl; } - if (lga_state == LGA_NO_SERVER) { + if (is_lga_state(LGA_NO_SERVER)) { /* We have no server and cannot open a stream. * The client may try again */ @@ -783,7 +778,10 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl goto done_give_hdl; } - if (lga_state == LGA_RECOVERY2) { + /* Synchronize b/w client/recovery thread */ + recovery2_lock(&is_locked); + + if (is_lga_state(LGA_RECOVERY2)) { /* Auto recovery is ongoing. We have to wait for it to finish. * The client may try again */ @@ -792,17 +790,17 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl goto done_give_hdl; } - if (lga_state == LGA_RECOVERY1) { + if (is_lga_state(LGA_RECOVERY1)) { /* We are in recovery 1 state. * Recover client and execute the request */ - rc = recover_one_client(hdl_rec); + rc = lga_recover_one_client(hdl_rec); if (rc == -1) { /* Client could not be recovered. Delete client and * return BAD HANDLE */ TRACE("%s delete_one_client", __FUNCTION__); - (void) delete_one_client(&lga_cb.client_list, hdl_rec); + (void) lga_hdl_rec_del(&lga_cb.client_list, hdl_rec); ais_rc = SA_AIS_ERR_BAD_HANDLE; /* Handles are destroyed so we shall not give handles */ goto done; @@ -833,7 +831,7 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl open_param->logFileName = (char *) malloc(strlen(logFileCreateAttributes->logFileName) + 1); if (open_param->logFileName == NULL) { ais_rc = SA_AIS_ERR_NO_MEMORY; - goto done_give_hdl; + goto done_free; } strcpy(open_param->logFileName, logFileCreateAttributes->logFileName); @@ -845,7 +843,7 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl open_param->logFilePathName = (char *) malloc(len); if (open_param->logFilePathName == NULL) { ais_rc = SA_AIS_ERR_NO_MEMORY; - goto done_give_hdl; + goto done_free; } if (logFileCreateAttributes->logFilePathName == NULL) @@ -860,21 +858,21 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl if (timeout < NCS_SAF_MIN_ACCEPT_TIME) { TRACE("Timeout"); ais_rc = SA_AIS_ERR_TIMEOUT; - goto done_give_hdl; + goto done_free; } /* Send a sync MDS message to obtain a log stream id */ ncs_rc = lga_mds_msg_sync_send(&lga_cb, &msg, &o_msg, timeout, MDS_SEND_PRIORITY_HIGH); if (ncs_rc != NCSCC_RC_SUCCESS) { ais_rc = SA_AIS_ERR_TRY_AGAIN; - goto done_give_hdl; + goto done_free; } ais_rc = o_msg->info.api_resp_info.rc; if (SA_AIS_OK != ais_rc) { TRACE("Bad return status!!! rc = %d", ais_rc); lga_msg_destroy(o_msg); - goto done_give_hdl; + goto done_free; } /** Retrieve the log stream id and log stream open id params @@ -884,7 +882,7 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl /** Lock LGA_CB **/ - pthread_mutex_lock(&lga_cb.cb_lock); + osaf_mutex_lock_ordie(&lga_cb.cb_lock); /** Allocate an LGA_LOG_STREAM_HDL_REC structure and insert this * into the list of channel hdl record. @@ -895,15 +893,15 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl logStreamName, log_header_type ); if (lstr_hdl_rec == NULL) { - pthread_mutex_unlock(&lga_cb.cb_lock); + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); lga_msg_destroy(o_msg); ais_rc = SA_AIS_ERR_NO_MEMORY; - goto done_give_hdl; + goto done_free; } /** UnLock LGA_CB **/ - pthread_mutex_unlock(&lga_cb.cb_lock); + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); /** Give the hdl-mgr allocated hdl to the application and free the response * message @@ -912,12 +910,15 @@ SaAisErrorT saLogStreamOpen_2(SaLogHandl lga_msg_destroy(o_msg); - done_give_hdl: - ncshm_give_hdl(logHandle); + done_free: free(open_param->logFileName); free(open_param->logFilePathName); + done_give_hdl: + ncshm_give_hdl(logHandle); + done: + recovery2_unlock(&is_locked); TRACE_LEAVE(); return ais_rc; } @@ -1106,6 +1107,7 @@ SaAisErrorT saLogWriteLogAsync(SaLogStre lgsv_write_log_async_req_t *write_param; SaNameT logSvcUsrName; int rc; + bool is_recovery2_locked = false; memset(&(msg), 0, sizeof(lgsv_msg_t)); write_param = &msg.info.api_info.param.write_log_async; @@ -1168,12 +1170,7 @@ SaAisErrorT saLogWriteLogAsync(SaLogStre * Handle states * Synchronize with mds and recovery thread (mutex) */ - pthread_mutex_lock(&lga_cb.cb_lock); - lgs_state_t lgs_state = lga_cb.lgs_state; - lga_state_t lga_state = lga_cb.lga_state; - pthread_mutex_unlock(&lga_cb.cb_lock); - - if (lgs_state == LGS_NO_ACTIVE) { + if (is_lgs_state(LGS_NO_ACTIVE)) { /* We have a server but it is temporarily unavailable. * Client may try again */ @@ -1182,7 +1179,7 @@ SaAisErrorT saLogWriteLogAsync(SaLogStre goto done_give_hdl_all; } - if (lga_state == LGA_NO_SERVER) { + if (is_lga_state(LGA_NO_SERVER)) { /* We have no server and cannot write. The client may try again */ TRACE("\t LGA_NO_SERVER"); @@ -1190,7 +1187,10 @@ SaAisErrorT saLogWriteLogAsync(SaLogStre goto done_give_hdl_all; } - if (lga_state == LGA_RECOVERY2) { + /* Synchronize b/w client/recovery thread */ + recovery2_lock(&is_recovery2_locked); + + if (is_lga_state(LGA_RECOVERY2)) { /* Auto recovery is ongoing. We have to wait for it to finish. * The client may try again */ @@ -1199,12 +1199,12 @@ SaAisErrorT saLogWriteLogAsync(SaLogStre goto done_give_hdl_all; } - if (lga_state == LGA_RECOVERY1) { + if (is_lga_state(LGA_RECOVERY1)) { /* We are in recovery 1 state. * Recover client and execute the request */ TRACE("\t LGA_RECOVERY1"); - rc = recover_one_client(hdl_rec); + rc = lga_recover_one_client(hdl_rec); if (rc == -1) { /* Client could not be recovered. Delete client and * return BAD HANDLE @@ -1216,7 +1216,7 @@ SaAisErrorT saLogWriteLogAsync(SaLogStre * take a log stream handle */ ncshm_give_hdl(logStreamHandle); - (void) delete_one_client(&lga_cb.client_list, hdl_rec); + (void) lga_hdl_rec_del(&lga_cb.client_list, hdl_rec); ais_rc = SA_AIS_ERR_BAD_HANDLE; /* Handles are destroyed so we shall not give handles */ goto done; @@ -1244,6 +1244,7 @@ SaAisErrorT saLogWriteLogAsync(SaLogStre ncshm_give_hdl(logStreamHandle); done: + recovery2_unlock(&is_recovery2_locked); TRACE_LEAVE(); return ais_rc; } @@ -1263,6 +1264,7 @@ SaAisErrorT saLogStreamClose(SaLogStream SaAisErrorT ais_rc = SA_AIS_OK; uint32_t mds_rc; int rc; + bool is_locked = false; TRACE_ENTER(); @@ -1277,12 +1279,7 @@ SaAisErrorT saLogStreamClose(SaLogStream * Handle states * Synchronize with mds and recovery thread (mutex) */ - pthread_mutex_lock(&lga_cb.cb_lock); - lgs_state_t lgs_state = lga_cb.lgs_state; - lga_state_t lga_state = lga_cb.lga_state; - pthread_mutex_unlock(&lga_cb.cb_lock); - - if (lgs_state == LGS_NO_ACTIVE) { + if (is_lgs_state(LGS_NO_ACTIVE)) { /* We have a server but it is temporarily unavailable. Client may * try again */ @@ -1291,7 +1288,10 @@ SaAisErrorT saLogStreamClose(SaLogStream goto done_give_hdl_stream; } - if (lga_state == LGA_RECOVERY2) { + /* Synchronize b/w client/recovery thread */ + recovery2_lock(&is_locked); + + if (is_lga_state(LGA_RECOVERY2)) { /* Auto recovery is ongoing. We have to wait for it to finish. * The client may try again */ @@ -1308,7 +1308,7 @@ SaAisErrorT saLogStreamClose(SaLogStream goto done_give_hdl_stream; } - if (lga_state == LGA_NO_SERVER) { + if (is_lga_state(LGA_NO_SERVER)) { /* No server is available. Remove the stream from client database. * Server side will manage to release resources of this stream when up. */ @@ -1317,11 +1317,11 @@ SaAisErrorT saLogStreamClose(SaLogStream goto rmv_stream; } - if (lga_state == LGA_RECOVERY1) { + if (is_lga_state(LGA_RECOVERY1)) { /* We are in recovery 1 state. * Recover client and execute the request */ - rc = recover_one_client(hdl_rec); + rc = lga_recover_one_client(hdl_rec); if (rc == -1) { /* Client could not be recovered. Delete client and * return BAD HANDLE @@ -1333,7 +1333,7 @@ SaAisErrorT saLogStreamClose(SaLogStream * take a log stream handle */ ncshm_give_hdl(logStreamHandle); - (void) delete_one_client(&lga_cb.client_list, hdl_rec); + (void) lga_hdl_rec_del(&lga_cb.client_list, hdl_rec); ais_rc = SA_AIS_ERR_BAD_HANDLE; /* Handles are destroyed so we shall not give handles */ goto done; @@ -1370,7 +1370,7 @@ SaAisErrorT saLogStreamClose(SaLogStream rmv_stream: if (ais_rc == SA_AIS_OK) { - pthread_mutex_lock(&lga_cb.cb_lock); + osaf_mutex_lock_ordie(&lga_cb.cb_lock); /** Delete this log stream & the associated resources with this * instance of log stream open. @@ -1380,7 +1380,7 @@ rmv_stream: ais_rc = SA_AIS_ERR_LIBRARY; } - pthread_mutex_unlock(&lga_cb.cb_lock); + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); } done_give_hdl_all: @@ -1389,6 +1389,7 @@ rmv_stream: ncshm_give_hdl(logStreamHandle); done: + recovery2_unlock(&is_locked); TRACE_LEAVE(); return ais_rc; } diff --git a/osaf/libs/agents/saf/lga/lga_mds.c b/osaf/libs/agents/saf/lga/lga_mds.c --- a/osaf/libs/agents/saf/lga/lga_mds.c +++ b/osaf/libs/agents/saf/lga/lga_mds.c @@ -516,12 +516,12 @@ static uint32_t lga_mds_svc_evt(struct n TRACE("%s\t NCSMDS_NO_ACTIVE", __FUNCTION__); /* This is a temporary server down e.g. during switch/fail over*/ if (mds_cb_info->info.svc_evt.i_svc_id == NCSMDS_SVC_ID_LGS) { - pthread_mutex_lock(&lga_cb.cb_lock); + osaf_mutex_lock_ordie(&lga_cb.cb_lock); TRACE("NCSMDS_NO_ACTIVE"); memset(&lga_cb.lgs_mds_dest, 0, sizeof(MDS_DEST)); lga_cb.lgs_state = LGS_NO_ACTIVE; - pthread_mutex_unlock(&lga_cb.cb_lock); + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); } break; case NCSMDS_DOWN: @@ -531,11 +531,11 @@ static uint32_t lga_mds_svc_evt(struct n * no longer valid and clients must register again (initialize) */ if (mds_cb_info->info.svc_evt.i_svc_id == NCSMDS_SVC_ID_LGS) { - pthread_mutex_lock(&lga_cb.cb_lock); + osaf_mutex_lock_ordie(&lga_cb.cb_lock); TRACE("LGS down"); memset(&lga_cb.lgs_mds_dest, 0, sizeof(MDS_DEST)); lga_cb.lgs_state = LGS_DOWN; - pthread_mutex_unlock(&lga_cb.cb_lock); + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); /* The log server is lost */ lga_no_server_state_set(); @@ -548,14 +548,14 @@ static uint32_t lga_mds_svc_evt(struct n TRACE("%s\t NCSMDS_UP" , __FUNCTION__); /** Store the MDS DEST of the LGS **/ - pthread_mutex_lock(&lga_cb.cb_lock); + osaf_mutex_lock_ordie(&lga_cb.cb_lock); lga_cb.lgs_mds_dest = mds_cb_info->info.svc_evt.i_dest; lga_cb.lgs_state = LGS_UP; if (lga_cb.lgs_sync_awaited) { /* signal waiting thread */ m_NCS_SEL_OBJ_IND(&lga_cb.lgs_sync_sel); } - pthread_mutex_unlock(&lga_cb.cb_lock); + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); /* The log server is up */ lga_serv_recov1state_set(); @@ -590,7 +590,7 @@ static uint32_t lga_mds_rcv(struct ncsmd lgsv_msg_t *lgsv_msg = (lgsv_msg_t *)mds_cb_info->info.receive.i_msg; uint32_t rc; - pthread_mutex_lock(&lga_cb.cb_lock); + osaf_mutex_lock_ordie(&lga_cb.cb_lock); /* process the message */ rc = lga_lgs_msg_proc(&lga_cb, lgsv_msg, mds_cb_info->info.receive.i_priority); @@ -598,7 +598,7 @@ static uint32_t lga_mds_rcv(struct ncsmd TRACE_2("lga_lgs_msg_proc returned: %d", rc); } - pthread_mutex_unlock(&lga_cb.cb_lock); + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); return rc; } diff --git a/osaf/libs/agents/saf/lga/lga_state.c b/osaf/libs/agents/saf/lga/lga_state.c --- a/osaf/libs/agents/saf/lga/lga_state.c +++ b/osaf/libs/agents/saf/lga/lga_state.c @@ -28,13 +28,19 @@ * Common data */ +/* And critical sections. Clients APIs must not pass recovery 2 state check if + * recovery 2 thread is started but state has not yet been changed by the + * thread. Also the thread must not start recovery if an API is executing and + * has passed the recovery 2 check + */ +static pthread_mutex_t lga_recov2_lock = PTHREAD_MUTEX_INITIALIZER; + /* Selection object for terminating recovery thread for state 2 (after timeout) * NOTE: Only for recovery2_thread */ static NCS_SEL_OBJ state2_terminate_sel_obj; static pthread_t recovery2_thread_id = 0; -static pthread_mutex_t lga_recov_mutex = PTHREAD_MUTEX_INITIALIZER; /****************************************************************************** * Functions used with server down recovery handling @@ -236,10 +242,8 @@ static int initialize_one_client(lga_cli /* Restore the client Id with the one returned by the LGS and * set the initialized flag */ - osaf_mutex_lock_ordie(&lga_cb.cb_lock); p_client->lgs_client_id = client_id; p_client->initialized_flag = true; - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); done: TRACE_LEAVE2("rc = %d", rc); @@ -278,10 +282,8 @@ static int recover_one_stream(lga_log_st /* Restore the stream Id with the Id returned by the LGS and * set the recovered flag */ - osaf_mutex_lock_ordie(&lga_cb.cb_lock); p_stream->lgs_log_stream_id = stream_id; p_stream->recovered_flag = true; - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); done: TRACE_LEAVE2("rc = %d", rc); @@ -326,9 +328,12 @@ static void *recovery2_thread(void *dumm if (rc == 0) { /* Timeout; Set recovery state 2 */ - osaf_mutex_lock_ordie(&lga_cb.cb_lock); - lga_cb.lga_state = LGA_RECOVERY2; - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); + /* Not allowed to continue if any API is in critical section + * handling client data + */ + lga_recovery2_lock(); + set_lga_state(LGA_RECOVERY2); + lga_recovery2_unlock(); TRACE("%s Poll timeout. Enter LGA_RECOVERY2 state", __FUNCTION__); } else { /* Stop signal received */ @@ -356,17 +361,15 @@ static void *recovery2_thread(void *dumm goto done; } /* Recover clients one at a time */ - rc = recover_one_client(p_client); + rc = lga_recover_one_client(p_client); TRACE("\t Client %d is recovered", p_client->lgs_client_id); if (rc == -1) { - TRACE("%s recover_one_client Fail Deleting cllient (id %d)", + TRACE("%s recover_one_client Fail Deleting client (id %d)", __FUNCTION__, p_client->lgs_client_id); /* Fail to recover this client * Remove (handle invalidated) */ - osaf_mutex_lock_ordie(&lga_cb.cb_lock); - (void) delete_one_client(&lga_cb.client_list, p_client); - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); + (void) lga_hdl_rec_del(&lga_cb.client_list, p_client); } /* Next client */ @@ -378,14 +381,13 @@ static void *recovery2_thread(void *dumm * Change to not recovering state * LGA_NORMAL */ - osaf_mutex_lock_ordie(&lga_cb.cb_lock); - lga_cb.lga_state = LGA_NORMAL; + set_lga_state(LGA_NORMAL); TRACE("\t Setting lga_state = LGA_NORMAL"); - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); done: /* Cleanup and Exit thread */ (void) ncs_sel_obj_destroy(&state2_terminate_sel_obj); + pthread_exit(NULL); TRACE_LEAVE(); return NULL; @@ -405,7 +407,7 @@ static int start_recovery2_thread(void) TRACE_ENTER(); pthread_attr_init(&attr); - pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); + pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); /* Create a selection object for signaling the recovery2 thread */ ncs_rc = ncs_sel_obj_create(&state2_terminate_sel_obj); @@ -438,26 +440,30 @@ done: static void stop_recovery2_thread(void) { uint32_t ncs_rc = 0; + int rc = 0; TRACE_ENTER(); /* Check if the thread is running */ - osaf_mutex_lock_ordie(&lga_cb.cb_lock); - lga_state_t lga_state = lga_cb.lga_state; - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); - - if (lga_state == LGA_NORMAL) { + if (is_lga_state(LGA_NORMAL) || is_lga_state(LGA_NO_SERVER)) { /* No thread to stop */ TRACE("%s LGA_NORMAL no thread to stop", __FUNCTION__); goto done; } - /* Signal the thread to stop */ + /* Signal the thread to terminate */ ncs_rc = ncs_sel_obj_ind(&state2_terminate_sel_obj); if (ncs_rc != NCSCC_RC_SUCCESS) { TRACE("%s ncs_sel_obj_ind Fail", __FUNCTION__); } + /* Join thread to wait for thread termination */ + rc = pthread_join(recovery2_thread_id, NULL); + if (rc != 0 ) { + LOG_NO("%s: Could not join recovery2 thread %s", __FUNCTION__, + strerror(rc)); + } + done: TRACE_LEAVE(); return; @@ -467,14 +473,6 @@ static void stop_recovery2_thread(void) * Recovery state handling functions ******************************************************************************/ -/****************************************************************************** - * Server Down - * - * Initiate recovery handling and set LGA_NO_SERVER state - * LGA_NO_SERVER: State set in MDS event handler when server down event - * lga_no_server_state_set() - ******************************************************************************/ - /** * Recovery state LGA_NO_SERVER * @@ -495,11 +493,11 @@ void lga_no_server_state_set(void) stop_recovery2_thread(); /* Set LGA_NO_SERVER state */ + set_lga_state(LGA_NO_SERVER); + TRACE("\t lga_state = LGA_NO_SERVER"); + + /* Synchronize b/w client/mds threads */ osaf_mutex_lock_ordie(&lga_cb.cb_lock); - lga_cb.lga_state = LGA_NO_SERVER; - TRACE("\t lga_state = LGA_NO_SERVER"); - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); - while (p_client != NULL) { /* Set Client flags for all clients */ p_client->initialized_flag = false; @@ -515,6 +513,7 @@ void lga_no_server_state_set(void) p_client = p_client->next; } + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); TRACE_LEAVE(); } @@ -546,18 +545,12 @@ void lga_serv_recov1state_set(void) { TRACE_ENTER(); - osaf_mutex_lock_ordie(&lga_cb.cb_lock); - if (lga_cb.lga_state != LGA_NO_SERVER) { + if (is_lga_state(LGA_NO_SERVER)) { /* We have not been headless. No recovery shall be done */ - TRACE("%s Previous state was not LGA_NO_SERVER lga_stat = %d", - __FUNCTION__, lga_cb.lga_state); - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); goto done; } else { - lga_cb.lga_state = LGA_RECOVERY1; - TRACE("lga_state = %d (2->RECOVERY1)", - lga_cb.lga_state); - osaf_mutex_unlock_ordie(&lga_cb.cb_lock); + set_lga_state(LGA_RECOVERY1); + TRACE("lga_state = RECOVERY1"); } start_recovery2_thread(); @@ -583,26 +576,20 @@ done: * for that stream. This may cause the client to initialize a new client and * the old client will remain as a resource leek. * + * NOTE: This function is not thread safe. + * * @param p_client[in] Pointer to a client record * @return -1 on error */ -int recover_one_client(lga_client_hdl_rec_t *p_client) +int lga_recover_one_client(lga_client_hdl_rec_t *p_client) { int rc = 0; lga_log_stream_hdl_rec_t *p_stream; TRACE_ENTER(); - /* This function may be called both in the recovery thread and in the - * client thread. A possible scenario is that the recovery 2 thread - * times out and calls this function in recovery mode 2 while there - * a client recovery is still ongoing started in mode 1. The recovery 2 - * thread must wait until ongoing recovery is done - */ - osaf_mutex_lock_ordie(&lga_recov_mutex); - /* We may have been waiting at mutex while the client was recovered - * so it may already been recovered. - */ + /* Synchronize b/w client/mds thread */ + osaf_mutex_lock_ordie(&lga_cb.cb_lock); if (p_client->recovered_flag == true) { /* Client is already recovered */ TRACE("\t Already recovered"); @@ -633,38 +620,91 @@ int recover_one_client(lga_client_hdl_re p_stream = p_stream->next; } - osaf_mutex_lock_ordie(&lga_cb.cb_lock); p_client->recovered_flag = true; +done: osaf_mutex_unlock_ordie(&lga_cb.cb_lock); - -done: - osaf_mutex_unlock_ordie(&lga_recov_mutex); - TRACE_LEAVE(); return rc; } /** - * Delete one client - * Wrapper for function lga_hdl_rec_del() is used (lga_utol.c) - * This wrapper adds a control to make sure that the function cannot - * be used by the recovery 2 thread and the client thread at the same time + * Free the memory allocated for one client handle with mutext protection. * - * @param list_head - * @param rm_node + * @param p_client_hdl + * @return void */ -uint32_t delete_one_client( - lga_client_hdl_rec_t **list_head, - lga_client_hdl_rec_t *rm_node - ) +void lga_free_client_hdl(lga_client_hdl_rec_t **p_client_hdl) { - TRACE_ENTER2(); - uint32_t ncs_rc; + lga_client_hdl_rec_t *client_hdl = NULL; - osaf_mutex_lock_ordie(&lga_recov_mutex); - ncs_rc = lga_hdl_rec_del(list_head, rm_node); - osaf_mutex_unlock_ordie(&lga_recov_mutex); + /* Synchronize b/w client & mds thread */ + osaf_mutex_lock_ordie(&lga_cb.cb_lock); - TRACE_LEAVE(); - return ncs_rc; + client_hdl = *p_client_hdl; + if (client_hdl == NULL) goto done; + + free(client_hdl); + client_hdl = NULL; + +done: + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); } + +void lga_recovery2_lock(void) +{ + osaf_mutex_lock_ordie(&lga_recov2_lock); +} + +void lga_recovery2_unlock(void) +{ + osaf_mutex_unlock_ordie(&lga_recov2_lock); +} + +//> +// Handling lga_cb.lgs_state and lga_state in thread safe +//< + +void set_lgs_state(lgs_state_e state) +{ + osaf_mutex_lock_ordie(&lga_cb.cb_lock); + lga_cb.lgs_state = state; + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); +} + +bool is_lgs_state(lgs_state_e lgs_state_i) +{ + bool rc = false; + + osaf_mutex_lock_ordie(&lga_cb.cb_lock); + if (lgs_state_i == lga_cb.lgs_state) + rc = true; + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); + + return rc; +} + +static lga_state_t lga_state = { + .state = LGA_NORMAL, + .lock = PTHREAD_MUTEX_INITIALIZER +}; + +void set_lga_state(lga_state_e state) +{ + osaf_mutex_lock_ordie(&lga_state.lock); + lga_state.state = state; + osaf_mutex_unlock_ordie(&lga_state.lock); +} + +bool is_lga_state(lga_state_e lga_state_i) +{ + bool rc = false; + + osaf_mutex_lock_ordie(&lga_state.lock); + if (lga_state_i == lga_state.state) + rc = true; + osaf_mutex_unlock_ordie(&lga_state.lock); + + return rc; +} + +//< diff --git a/osaf/libs/agents/saf/lga/lga_state.h b/osaf/libs/agents/saf/lga/lga_state.h --- a/osaf/libs/agents/saf/lga/lga_state.h +++ b/osaf/libs/agents/saf/lga/lga_state.h @@ -27,11 +27,40 @@ extern "C" { /* Recovery functions */ void lga_no_server_state_set(void); void lga_serv_recov1state_set(void); -int recover_one_client(lga_client_hdl_rec_t *p_client); -uint32_t delete_one_client( - lga_client_hdl_rec_t **list_head, - lga_client_hdl_rec_t *rm_node - ); +int lga_recover_one_client(lga_client_hdl_rec_t *p_client); +void lga_recovery2_lock(void); +void lga_recovery2_unlock(void); +void lga_free_client_hdl(lga_client_hdl_rec_t **p_client_hdl); + +void set_lgs_state(lgs_state_e state); +void set_lga_state(lga_state_e state); +bool is_lgs_state(lgs_state_e lgs_state_i); +bool is_lga_state(lga_state_e lga_state_i); + + +/** + * Protect critical areas AIS functions handling global client data so that + * this data is not handled at the same time by the recovery 2 thread + * Lock must be done before checking if lga_state is RECOVERY2 + * Unlock must be done just before returning from function. It is allowed to + * call lga_recovery2_unlock() also if no previous call to lock is done + */ +static inline void recovery2_lock(bool *is_locked) +{ + if (is_lga_state(LGA_RECOVERY1) || is_lga_state(LGA_RECOVERY2)) { + /* TBD: Is this optimization really needed? */ + lga_recovery2_lock(); + *is_locked = true; + } +} + +static inline void recovery2_unlock(bool *is_locked) +{ + if (*is_locked) { + *is_locked = false; + lga_recovery2_unlock(); + } +} #ifdef __cplusplus } diff --git a/osaf/libs/agents/saf/lga/lga_util.c b/osaf/libs/agents/saf/lga/lga_util.c --- a/osaf/libs/agents/saf/lga/lga_util.c +++ b/osaf/libs/agents/saf/lga/lga_util.c @@ -19,6 +19,7 @@ #include #include "lga.h" #include "osaf_poll.h" +#include "lga_state.h" /* Variables used during startup/shutdown only */ static pthread_mutex_t lga_lock = PTHREAD_MUTEX_INITIALIZER; @@ -50,9 +51,9 @@ static unsigned int lga_create(void) */ osaf_poll_one_fd(m_GET_FD_FROM_SEL_OBJ(lga_cb.lgs_sync_sel), 10000); - pthread_mutex_lock(&lga_cb.cb_lock); + osaf_mutex_lock_ordie(&lga_cb.cb_lock); lga_cb.lgs_sync_awaited = 0; - pthread_mutex_unlock(&lga_cb.cb_lock); + osaf_mutex_unlock_ordie(&lga_cb.cb_lock); /* No longer needed */ m_NCS_SEL_OBJ_DESTROY(&lga_cb.lgs_sync_sel); @@ -281,7 +282,7 @@ static uint32_t lga_hdl_cbk_dispatch_blo unsigned int lga_startup(lga_cb_t *cb) { unsigned int rc = NCSCC_RC_SUCCESS; - pthread_mutex_lock(&lga_lock); + osaf_mutex_lock_ordie(&lga_lock); TRACE_ENTER2("lga_use_count: %u", lga_use_count); if (lga_use_count > 0) { @@ -304,11 +305,11 @@ unsigned int lga_startup(lga_cb_t *cb) /* Agent has successfully been started including communication * with server */ - cb->lga_state = LGA_NORMAL; + set_lga_state(LGA_NORMAL); } done: - pthread_mutex_unlock(&lga_lock); + osaf_mutex_unlock_ordie(&lga_lock); TRACE_LEAVE2("rc: %u, lga_use_count: %u", rc, lga_use_count); return rc; @@ -331,7 +332,7 @@ unsigned int lga_shutdown_after_last_cli unsigned int rc = NCSCC_RC_SUCCESS; TRACE_ENTER2("lga_use_count: %u", lga_use_count); - pthread_mutex_lock(&lga_lock); + osaf_mutex_lock_ordie(&lga_lock); if (lga_use_count > 1) { /* Users still exist, just decrement the use count */ @@ -342,7 +343,7 @@ unsigned int lga_shutdown_after_last_cli lga_use_count = 0; } - pthread_mutex_unlock(&lga_lock); + osaf_mutex_unlock_ordie(&lga_lock); TRACE_LEAVE2("rc: %u, lga_use_count: %u", rc, lga_use_count); return rc; @@ -367,14 +368,14 @@ unsigned int lga_force_shutdown(void) { unsigned int rc = NCSCC_RC_SUCCESS; TRACE_ENTER(); - pthread_mutex_lock(&lga_lock); + osaf_mutex_lock_ordie(&lga_lock); if (lga_use_count > 0) { lga_destroy(); rc = ncs_agents_shutdown(); /* Always returns NCSCC_RC_SUCCESS */ lga_use_count = 0; TRACE("%s: Forced shutdown. Handles invalidated\n",__FUNCTION__); } - pthread_mutex_unlock(&lga_lock); + osaf_mutex_unlock_ordie(&lga_lock); TRACE_LEAVE(); return rc; } @@ -449,10 +450,7 @@ void lga_hdl_list_del(lga_client_hdl_rec /** clean up the channel records for this lga-client **/ lga_log_stream_hdl_rec_list_del(&client_hdl->stream_list); - /** remove the association with hdl-mngr - **/ - free(client_hdl); - client_hdl = 0; + lga_free_client_hdl(&client_hdl); } TRACE_LEAVE(); } @@ -546,10 +544,7 @@ uint32_t lga_hdl_rec_del(lga_client_hdl_ /** Free the channel records off this hdl **/ lga_log_stream_hdl_rec_list_del(&rm_node->stream_list); - - /** free the hdl rec - **/ - free(rm_node); + lga_free_client_hdl(&rm_node); rc = NCSCC_RC_SUCCESS; goto out; } else { /* find the rec */ @@ -565,10 +560,7 @@ uint32_t lga_hdl_rec_del(lga_client_hdl_ ncshm_destroy_hdl(NCS_SERVICE_ID_LGA, rm_node->local_hdl); /** Free the channel records off this lga_hdl */ lga_log_stream_hdl_rec_list_del(&rm_node->stream_list); - - /** free the hdl rec */ - free(rm_node); - + lga_free_client_hdl(&rm_node); rc = NCSCC_RC_SUCCESS; goto out; } @@ -708,11 +700,11 @@ lga_client_hdl_rec_t *lga_hdl_rec_add(lg ** CLIENT_HDL_RECORDS for this LGA_CB **/ - pthread_mutex_lock(&cb->cb_lock); + osaf_mutex_lock_ordie(&cb->cb_lock); /* add this to the start of the list */ rec->next = cb->client_list; cb->client_list = rec; - pthread_mutex_unlock(&cb->cb_lock); + osaf_mutex_unlock_ordie(&cb->cb_lock); goto out;