[PATCH 1/4] CSI, fix debug print crash
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystemIndication.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 2d0a94e..b1b9fa0 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -503,7 +503,7 @@ static void csi_domain_event_cb(virConnectPtr conn, CMPIStatus s = {CMPI_RC_OK, NULL}; if (lifecycle_enabled == false || thread->active_filters <= 0) { - CU_DEBUG("%s indications deactivated, return"); + CU_DEBUG("%s indications deactivated, return", prefix); return; } -- 1.7.1
This patch added lock on shared data without which may trigger incorrect data operation. Global lock is used so speed may be slowed down, it can be improved in the future. 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 b1b9fa0..3159ca0 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, @@ -490,6 +492,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, @@ -502,10 +506,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); @@ -537,7 +544,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) { @@ -545,12 +554,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); @@ -558,7 +571,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: @@ -579,10 +594,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; } @@ -594,33 +611,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; @@ -628,6 +659,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
Dangerous bug, may trigger strange error, suggest apply it before investigate any strange bug shown. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_DevicePool.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 202e509..79dc108 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -710,7 +710,7 @@ static bool mempool_set_consumed(CMPIInstance *inst, virConnectPtr conn) dom = virDomainLookupByID(conn, domain_ids[i]); if (dom == NULL) { - CU_DEBUG("Cannot connect to domain %n: excluding", + CU_DEBUG("Cannot connect to domain %d: excluding", domain_ids[i]); continue; } -- 1.7.1
+1 -Sharad Mishra Quoting Wenchao Xia <xiawenc@linux.vnet.ibm.com>:
Dangerous bug, may trigger strange error, suggest apply it before investigate any strange bug shown.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_DevicePool.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 202e509..79dc108 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -710,7 +710,7 @@ static bool mempool_set_consumed(CMPIInstance *inst, virConnectPtr conn)
dom = virDomainLookupByID(conn, domain_ids[i]); if (dom == NULL) { - CU_DEBUG("Cannot connect to domain %n: excluding", + CU_DEBUG("Cannot connect to domain %d: excluding", domain_ids[i]); continue; } -- 1.7.1
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
Oringinal implement have risk, this patch should fix it Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_DevicePool.c | 47 +++++++++++++++++++++++++++++++++++------------ 1 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 79dc108..0cb9124 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -117,52 +117,75 @@ int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) return ret; } +/* This function returns the real number of pools, no negative value should be + returned, if error happens it returns zero. */ static int get_diskpool_config(virConnectPtr conn, struct tmp_disk_pool **_pools) { - int count = 0; + int count = 0, realcount = 0; int i; char ** names = NULL; struct tmp_disk_pool *pools = NULL; + int have_err = 0; count = virConnectNumOfStoragePools(conn); - if (count <= 0) + if (count <= 0) { + have_err = 1; goto out; + } names = calloc(count, sizeof(char *)); if (names == NULL) { CU_DEBUG("Failed to alloc space for %i pool names", count); count = 0; + have_err = 1; goto out; } - if (virConnectListStoragePools(conn, names, count) == -1) { + realcount = virConnectListStoragePools(conn, names, count); + if (realcount == -1) { CU_DEBUG("Failed to get storage pools"); - count = 0; + realcount = 0; + have_err = 1; + goto out; + } + if (realcount == 0) { + CU_DEBUG("zero pools got, but prelist is %d.", count); goto out; } - pools = calloc(count, sizeof(*pools)); + pools = calloc(realcount, sizeof(*pools)); if (pools == NULL) { - CU_DEBUG("Failed to alloc space for %i pool structs", count); + CU_DEBUG("Failed to alloc space for %i pool structs", realcount); + realcount = 0; + have_err = 1; goto out; } - for (i = 0; i < count; i++) { + i = 0; + while (i < realcount) { pools[i].tag = strdup(names[i]); pools[i].primordial = false; + i++; } out: - for (i = 0; i < count; i++) - free(names[i]); - free(names); + if (count > 0) { + i = 0; + while (i < count) { + free(names[i]); + i++; + } + free(names); + } - get_disk_parent(&pools, &count); + if (have_err == 0) { + get_disk_parent(&pools, &realcount); + } *_pools = pools; - return count; + return realcount; } static bool diskpool_set_capacity(virConnectPtr conn, -- 1.7.1
-1 This patch makes it impossible for called to know if "0" was returned as a result of a failure or no diskpools were found. I recommend returning "-1" in error and update the patch to make sure that calling functions are handling a return value of -1 properly. -Sharad Mishra Quoting Wenchao Xia <xiawenc@linux.vnet.ibm.com>:
Oringinal implement have risk, this patch should fix it
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_DevicePool.c | 47 +++++++++++++++++++++++++++++++++++------------ 1 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 79dc108..0cb9124 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -117,52 +117,75 @@ int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) return ret; }
+/* This function returns the real number of pools, no negative value should be + returned, if error happens it returns zero. */ static int get_diskpool_config(virConnectPtr conn, struct tmp_disk_pool **_pools) { - int count = 0; + int count = 0, realcount = 0; int i; char ** names = NULL; struct tmp_disk_pool *pools = NULL; + int have_err = 0;
count = virConnectNumOfStoragePools(conn); - if (count <= 0) + if (count <= 0) { + have_err = 1; goto out; + }
names = calloc(count, sizeof(char *)); if (names == NULL) { CU_DEBUG("Failed to alloc space for %i pool names", count); count = 0; + have_err = 1; goto out; }
- if (virConnectListStoragePools(conn, names, count) == -1) { + realcount = virConnectListStoragePools(conn, names, count); + if (realcount == -1) { CU_DEBUG("Failed to get storage pools"); - count = 0; + realcount = 0; + have_err = 1; + goto out; + } + if (realcount == 0) { + CU_DEBUG("zero pools got, but prelist is %d.", count); goto out; }
- pools = calloc(count, sizeof(*pools)); + pools = calloc(realcount, sizeof(*pools)); if (pools == NULL) { - CU_DEBUG("Failed to alloc space for %i pool structs", count); + CU_DEBUG("Failed to alloc space for %i pool structs", realcount); + realcount = 0; + have_err = 1; goto out; }
- for (i = 0; i < count; i++) { + i = 0; + while (i < realcount) { pools[i].tag = strdup(names[i]); pools[i].primordial = false; + i++; }
out: - for (i = 0; i < count; i++) - free(names[i]); - free(names); + if (count > 0) { + i = 0; + while (i < count) { + free(names[i]); + i++; + } + free(names); + }
- get_disk_parent(&pools, &count); + if (have_err == 0) { + get_disk_parent(&pools, &realcount); + }
*_pools = pools;
- return count; + return realcount; }
static bool diskpool_set_capacity(virConnectPtr conn, -- 1.7.1
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
On Mon, Oct 22, 2012 at 5:38 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
Oringinal implement have risk, this patch should fix it
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_DevicePool.c | 47 +++++++++++++++++++++++++++++++++++------------ 1 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 79dc108..0cb9124 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -117,52 +117,75 @@ int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) return ret; }
+/* This function returns the real number of pools, no negative value should be + returned, if error happens it returns zero. */ static int get_diskpool_config(virConnectPtr conn, struct tmp_disk_pool **_pools) { - int count = 0; + int count = 0, realcount = 0; int i; char ** names = NULL; struct tmp_disk_pool *pools = NULL; + int have_err = 0;
count = virConnectNumOfStoragePools(conn); - if (count <= 0) + if (count <= 0) { + have_err = 1; goto out; + }
names = calloc(count, sizeof(char *)); if (names == NULL) { CU_DEBUG("Failed to alloc space for %i pool names", count); count = 0; + have_err = 1; goto out; }
- if (virConnectListStoragePools(conn, names, count) == -1) { + realcount = virConnectListStoragePools(conn, names, count); + if (realcount == -1) { CU_DEBUG("Failed to get storage pools"); - count = 0; + realcount = 0; + have_err = 1; + goto out; + } + if (realcount == 0) { + CU_DEBUG("zero pools got, but prelist is %d.", count); goto out; }
- pools = calloc(count, sizeof(*pools)); + pools = calloc(realcount, sizeof(*pools)); if (pools == NULL) { - CU_DEBUG("Failed to alloc space for %i pool structs", count); + CU_DEBUG("Failed to alloc space for %i pool structs", realcount); + realcount = 0; + have_err = 1; goto out; }
- for (i = 0; i < count; i++) { + i = 0; + while (i < realcount) { pools[i].tag = strdup(names[i]); pools[i].primordial = false; + i++; }
Any specific reason for changing the for() loop for a while() one??
out: - for (i = 0; i < count; i++) - free(names[i]); - free(names); + if (count > 0) { + i = 0; + while (i < count) { + free(names[i]); + i++; + } + free(names); + }
Same here. Best regards, -- Eduardo de Barros Lima ◤✠◢ eblima@gmail.com
+1 -Sharad Mishra Quoting Wenchao Xia <xiawenc@linux.vnet.ibm.com>:
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystemIndication.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 2d0a94e..b1b9fa0 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -503,7 +503,7 @@ static void csi_domain_event_cb(virConnectPtr conn, CMPIStatus s = {CMPI_RC_OK, NULL};
if (lifecycle_enabled == false || thread->active_filters <= 0) { - CU_DEBUG("%s indications deactivated, return"); + CU_DEBUG("%s indications deactivated, return", prefix); return; }
-- 1.7.1
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
participants (3)
-
Eduardo Lima (Etrunko) -
snmishra＠linux.vnet.ibm.com -
Wenchao Xia