This patch use lock 'lifecycle_mutex' to protect gloable shared data in lifecycle_thread(). This lock exist in Activate/Deactivate Enable/Disable method, which is meant to protect the gloable shared data, but forgot to be added in new libvirt based CSI thread. This patch can avoid following risk at least: Original code have a small chance to free thread->args in child thread just after main thread malloc it, for that thread->id is set to zero allowing main thread to enter that code. This patch focus on adding missing lock, the CSI can be still improved as: smaller lock, folder lock into a structure with data to tip better what it is doing. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystemIndication.c | 39 ++++++++++++++++++++++++++++++++-- 1 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 8250850..3df47fa 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -174,9 +174,11 @@ static void csi_free_thread_data(void *data) if (data == NULL) return; + pthread_mutex_lock(&lifecycle_mutex); list_free(thread->dom_list); thread->dom_list = NULL; stdi_free_ind_args(&thread->args); + pthread_mutex_unlock(&lifecycle_mutex); } void set_source_inst_props(const CMPIBroker *broker, @@ -491,6 +493,8 @@ static int update_domain_list(virConnectPtr conn, csi_thread_data_t *thread) return s.rc; } +/* following function will protect global data with lifecycle_mutex. + TODO: improve it with seperate lock later. */ static void csi_domain_event_cb(virConnectPtr conn, virDomainPtr dom, int event, @@ -503,10 +507,13 @@ static void csi_domain_event_cb(virConnectPtr conn, char *prefix = class_prefix_name(thread->args->classname); CMPIStatus s = {CMPI_RC_OK, NULL}; + pthread_mutex_lock(&lifecycle_mutex); if (lifecycle_enabled == false || thread->active_filters <= 0) { CU_DEBUG("%s indications deactivated, return", prefix); + pthread_mutex_unlock(&lifecycle_mutex); return; } + pthread_mutex_unlock(&lifecycle_mutex); CU_DEBUG("Event: Domain %s(%d) event: %d detail: %d\n", virDomainGetName(dom), virDomainGetID(dom), event, detail); @@ -538,7 +545,9 @@ static void csi_domain_event_cb(virConnectPtr conn, if (cs_event != CS_CREATED) { char uuid[VIR_UUID_STRING_BUFLEN] = {0}; virDomainGetUUIDString(dom, &uuid[0]); + pthread_mutex_lock(&lifecycle_mutex); dom_xml = list_find(thread->dom_list, uuid); + pthread_mutex_unlock(&lifecycle_mutex); } if (dom_xml == NULL) { @@ -546,12 +555,16 @@ static void csi_domain_event_cb(virConnectPtr conn, goto end; } + pthread_mutex_lock(&lifecycle_mutex); async_ind(thread->args, cs_event, dom_xml, prefix); + pthread_mutex_unlock(&lifecycle_mutex); /* Update the domain list accordingly */ if (event == VIR_DOMAIN_EVENT_DEFINED) { if (detail == VIR_DOMAIN_EVENT_DEFINED_ADDED) { + pthread_mutex_lock(&lifecycle_mutex); csi_thread_dom_list_append(thread, dom_xml); + pthread_mutex_unlock(&lifecycle_mutex); } else if (detail == VIR_DOMAIN_EVENT_DEFINED_UPDATED) { free(dom_xml->name); free(dom_xml->xml); @@ -559,7 +572,9 @@ static void csi_domain_event_cb(virConnectPtr conn, } } else if (event == VIR_DOMAIN_EVENT_DEFINED && detail == VIR_DOMAIN_EVENT_UNDEFINED_REMOVED) { + pthread_mutex_lock(&lifecycle_mutex); list_remove(thread->dom_list, dom_xml); + pthread_mutex_unlock(&lifecycle_mutex); } end: @@ -580,10 +595,12 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) if (prefix == NULL) goto init_out; + pthread_mutex_lock(&lifecycle_mutex); conn = connect_by_classname(_BROKER, args->classname, &s); if (conn == NULL) { CU_DEBUG("Unable to start lifecycle thread: " "Failed to connect (cn: %s)", args->classname); + pthread_mutex_unlock(&lifecycle_mutex); goto conn_out; } @@ -595,33 +612,47 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) if (cb_id == -1) { CU_DEBUG("Failed to register domain event watch for '%s'", - args->classname) + args->classname); + pthread_mutex_unlock(&lifecycle_mutex); goto cb_out; } CBAttachThread(_BROKER, args->context); /* Get currently defined domains */ - if (update_domain_list(conn, thread) != CMPI_RC_OK) + if (update_domain_list(conn, thread) != CMPI_RC_OK) { + pthread_mutex_unlock(&lifecycle_mutex); goto end; + } + pthread_mutex_unlock(&lifecycle_mutex); CU_DEBUG("Entering CSI event loop (%s)", prefix); - while (thread->active_filters > 0) { + while (1) { + pthread_mutex_lock(&lifecycle_mutex); + if (thread->active_filters <= 0) { + pthread_mutex_unlock(&lifecycle_mutex); + break; + } + pthread_mutex_unlock(&lifecycle_mutex); if (virEventRunDefaultImpl() < 0) { virErrorPtr err = virGetLastError(); CU_DEBUG("Failed to run event loop: %s\n", err && err->message ? err->message : "Unknown error"); } + usleep(1); } CU_DEBUG("Exiting CSI event loop (%s)", prefix); + pthread_mutex_lock(&lifecycle_mutex); CBDetachThread(_BROKER, args->context); + pthread_mutex_unlock(&lifecycle_mutex); end: virConnectDomainEventDeregisterAny(conn, cb_id); cb_out: + pthread_mutex_lock(&lifecycle_mutex); thread->id = 0; thread->active_filters = 0; @@ -629,6 +660,8 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) if (thread->args != NULL) stdi_free_ind_args(&thread->args); + pthread_mutex_unlock(&lifecycle_mutex); + conn_out: virConnectClose(conn); -- 1.7.1