[PATCH 0 of 2] Virt_ComputerSystemIndication
This one stops leaking some of the memory from the ind_args struct and adds a little more verbosity to some error messages per Dan's request. I didn't add system name to the "Failed to create ind" one, because it was not readily available already and the extra code to get it didn't seem worth it, but I put in all the information that is available at that point.
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1201036568 18000 # Node ID 977df56c5d87f5a90fc90400ecdc7d0687ce2d88 # Parent ca8dc23eacc44970a2d79978bc9de65aaa600b92 Add Modified to the list of available indications in ComputerSystemIndication. This is the integration of the previously discussed CSMI provider into the existing CSI provider. Most of the integration changes are actually done to how CSI works, as CSMI featured several improvements. The schema changes are in here too because they're just so small and simple compared to everything else that I didn't bother to break them out this time. If keeping them separate is preferred I can do that. Changes from #1 to #2: Fixed up ind_args so we don't try to pass around stale pointers. Made sure lifecycle_thread_id variable is reset when event loop exits, allowing unsubscribe/re-subscribe to happen successfully. Changes from #2 to #3: Use existing free_domain_list, since it's already in misc_util.c and most likely compiles down to the exact same instructions. Go Jay. Use double pointer in free_ind_args to prevent leaking args. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r ca8dc23eacc4 -r 977df56c5d87 schema/ComputerSystemIndication.mof --- a/schema/ComputerSystemIndication.mof Wed Jan 09 13:08:17 2008 +0100 +++ b/schema/ComputerSystemIndication.mof Tue Jan 22 16:16:08 2008 -0500 @@ -14,6 +14,13 @@ class Xen_ComputerSystemDeletedIndicatio { }; +[Description ("Xen_ComputerSystem modified"), + Provider("cmpi::Virt_ComputerSystemIndication") +] +class Xen_ComputerSystemModifiedIndication : CIM_InstModification +{ +}; + [Description ("KVM_ComputerSystem lifecycle"), Provider("cmpi::Virt_ComputerSystemIndication") ] @@ -27,3 +34,10 @@ class KVM_ComputerSystemDeletedIndicatio class KVM_ComputerSystemDeletedIndication : CIM_InstDeletion { }; + +[Description ("KVM_ComputerSystem modified"), + Provider("cmpi::Virt_ComputerSystemIndication") +] +class KVM_ComputerSystemModifiedIndication : CIM_InstModification +{ +}; diff -r ca8dc23eacc4 -r 977df56c5d87 schema/ComputerSystemIndication.registration --- a/schema/ComputerSystemIndication.registration Wed Jan 09 13:08:17 2008 +0100 +++ b/schema/ComputerSystemIndication.registration Tue Jan 22 16:16:08 2008 -0500 @@ -2,5 +2,7 @@ # Classname Namespace ProviderName ProviderModule ProviderTypes Xen_ComputerSystemCreatedIndication root/virt Virt_ComputerSystemIndication Virt_ComputerSystemIndication indication method Xen_ComputerSystemDeletedIndication root/virt Virt_ComputerSystemIndication Virt_ComputerSystemIndication indication method +Xen_ComputerSystemModifiedIndication root/virt Virt_ComputerSystemIndication Virt_ComputerSystemIndication indication method KVM_ComputerSystemCreatedIndication root/virt Virt_ComputerSystemIndication Virt_ComputerSystemIndication indication method KVM_ComputerSystemDeletedIndication root/virt Virt_ComputerSystemIndication Virt_ComputerSystemIndication indication method +KVM_ComputerSystemModifiedIndication root/virt Virt_ComputerSystemIndication Virt_ComputerSystemIndication indication method diff -r ca8dc23eacc4 -r 977df56c5d87 src/Virt_ComputerSystemIndication.c --- a/src/Virt_ComputerSystemIndication.c Wed Jan 09 13:08:17 2008 +0100 +++ b/src/Virt_ComputerSystemIndication.c Tue Jan 22 16:16:08 2008 -0500 @@ -62,6 +62,101 @@ static bool lifecycle_enabled = 0; # define _EI_RET() return (CMPIStatus){CMPI_RC_OK, NULL} #endif +struct dom_xml { + char uuid[VIR_UUID_STRING_BUFLEN]; + char *xml; +}; + +struct ind_args { + CMPIContext *context; + char *ns; + char *classname; +}; + +static void free_dom_xml (struct dom_xml dom) +{ + free(dom.xml); +} + +static void free_ind_args (struct ind_args **args) +{ + free((*args)->ns); + free((*args)->classname); + free(*args); + *args = NULL; +} + +static char *sys_name_from_xml(char *xml) +{ + char *tmp = NULL; + char *name = NULL; + int rc; + + tmp = strstr(xml, "<name>"); + if (tmp == NULL) + goto out; + + rc = sscanf(tmp, "<name>%a[^<]s</name>", &name); + if (rc != 1) + name = NULL; + + out: + return name; +} + +static CMPIStatus doms_to_xml(struct dom_xml **dom_xml_list, + virDomainPtr *dom_ptr_list, + int dom_ptr_count) +{ + int i; + int rc; + CMPIStatus s = {CMPI_RC_OK, NULL}; + + *dom_xml_list = calloc(dom_ptr_count, sizeof(struct dom_xml)); + for (i = 0; i < dom_ptr_count; i++) { + rc = virDomainGetUUIDString(dom_ptr_list[i], + (*dom_xml_list)[i].uuid); + if (rc == -1) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get UUID"); + /* If any domain fails, we fail. */ + break; + } + + (*dom_xml_list)[i].xml = virDomainGetXMLDesc(dom_ptr_list[i], + 0); + if ((*dom_xml_list)[i].xml == NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get xml desc"); + break; + } + } + + return s; +} + +static bool dom_changed(struct dom_xml prev_dom, + struct dom_xml *cur_xml, + int cur_count) +{ + int i; + bool ret = false; + + for (i = 0; i < cur_count; i++) { + if (strcmp(cur_xml[i].uuid, prev_dom.uuid) != 0) + continue; + + if (strcmp(cur_xml[i].xml, prev_dom.xml) != 0) + ret = true; + + break; + } + + return ret; +} + static bool _lifecycle_indication(const CMPIBroker *broker, const CMPIContext *ctx, const CMPIObjectPath *newsystem, @@ -97,6 +192,45 @@ static bool _lifecycle_indication(const NAMESPACE(newsystem), ind); + + return true; +} + +static bool _do_modified_indication(const CMPIBroker *broker, + const CMPIContext *ctx, + CMPIInstance *mod_inst, + char *prefix, + char *ns) +{ + CMPIObjectPath *ind_op; + CMPIInstance *ind; + CMPIStatus s; + + ind = get_typed_instance(broker, + prefix, + "ComputerSystemModifiedIndication", + ns); + if (ind == NULL) { + CU_DEBUG("Failed to create ind"); + return false; + } + + ind_op = CMGetObjectPath(ind, &s); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Failed to get ind_op"); + return false; + } + + CMSetProperty(ind, "PreviousInstance", + (CMPIValue *)&mod_inst, CMPI_instance); + + CU_DEBUG("Delivering Indication: %s\n", + CMGetCharPtr(CMObjectPathToString(ind_op, NULL))); + + CBDeliverIndication(_BROKER, + ctx, + CIM_VIRT_NS, + ind); return true; } @@ -177,17 +311,56 @@ static bool async_ind(CMPIContext *conte return _lifecycle_indication(_BROKER, context, op, type_name); } +static bool mod_ind(CMPIContext *context, + virConnectPtr conn, + struct dom_xml prev_dom, + char *prefix, + char *ns) +{ + bool rc; + char *name = NULL; + CMPIInstance *mod_inst; + + mod_inst = get_typed_instance(_BROKER, + prefix, + "ComputerSystem", + ns); + + name = sys_name_from_xml(prev_dom.xml); + CU_DEBUG("Name for system: '%s'", name); + if (name == NULL) { + rc = false; + goto out; + } + + CMSetProperty(mod_inst, "Name", + (CMPIValue *)name, CMPI_chars); + CMSetProperty(mod_inst, "UUID", + (CMPIValue *)prev_dom.uuid, CMPI_chars); + + rc = _do_modified_indication(_BROKER, context, mod_inst, prefix, ns); + + out: + free(name); + return rc; +} + static CMPI_THREAD_RETURN lifecycle_thread(void *params) { - CMPIContext *context = (CMPIContext *)params; + struct ind_args *args = (struct ind_args *)params; + CMPIContext *context = args->context; CMPIStatus s; int prev_count; int cur_count; virDomainPtr *prev_list; virDomainPtr *cur_list; + struct dom_xml *cur_xml = NULL; + struct dom_xml *prev_xml = NULL; virConnectPtr conn; + char *prefix = class_prefix_name(args->classname); + char *ns = args->ns; - conn = lv_connect(_BROKER, &s); + conn = connect_by_classname(_BROKER, args->classname, &s); if (conn == NULL) { CU_DEBUG("Failed to connect: %s", CMGetCharPtr(s.msg)); return NULL; @@ -198,12 +371,20 @@ static CMPI_THREAD_RETURN lifecycle_thre CBAttachThread(_BROKER, context); prev_count = get_domain_list(conn, &prev_list); + s = doms_to_xml(&prev_xml, prev_list, prev_count); + if (s.rc != CMPI_RC_OK) + CU_DEBUG("doms_to_xml failed. Attempting to continue."); + + CU_DEBUG("entering event loop"); while (lifecycle_enabled) { int i; bool res; cur_count = get_domain_list(conn, &cur_list); + s = doms_to_xml(&cur_xml, cur_list, cur_count); + if (s.rc != CMPI_RC_OK) + CU_DEBUG("doms_to_xml failed."); for (i = 0; i < cur_count; i++) { res = dom_in_list(cur_list[i], prev_count, prev_list); @@ -221,15 +402,33 @@ static CMPI_THREAD_RETURN lifecycle_thre conn, virDomainGetName(prev_list[i]), CS_DELETED); - virDomainFree(prev_list[i]); } + for (i = 0; i < prev_count; i++) { + res = dom_changed(prev_xml[i], cur_xml, cur_count); + if (res) { + CU_DEBUG("Domain '%s' modified.", prev_xml[i].uuid); + mod_ind(context, conn, prev_xml[i], prefix, ns); + } + free_dom_xml(prev_xml[i]); + } + + free_domain_list(prev_list, prev_count); + free(prev_list); + free(prev_xml); prev_list = cur_list; + prev_xml = cur_xml; prev_count = cur_count; wait_for_event(); } + pthread_mutex_unlock(&lifecycle_mutex); + free_ind_args(&args); + free(prefix); + virConnectClose(conn); + + lifecycle_thread_id = 0; return NULL; } @@ -241,17 +440,29 @@ static CMPIStatus ActivateFilter(CMPIInd const CMPIObjectPath* op, CMPIBoolean first) { + CU_DEBUG("ActivateFilter"); + CMPIStatus s = {CMPI_RC_OK, NULL}; + struct ind_args *args = malloc(sizeof(struct ind_args)); + + if (CMIsNullObject(op)) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "No ObjectPath given"); + goto out; + } + args->ns = strdup(NAMESPACE(op)); + args->classname = strdup(CLASSNAME(op)); + if (lifecycle_thread_id == 0) { - CMPIContext *thread_context; - - thread_context = CBPrepareAttachThread(_BROKER, ctx); + args->context = CBPrepareAttachThread(_BROKER, ctx); lifecycle_thread_id = _BROKER->xft->newThread(lifecycle_thread, - thread_context, + args, 0); } - return (CMPIStatus){CMPI_RC_OK, NULL}; + out: + return s; } static CMPIStatus DeActivateFilter(CMPIIndicationMI* mi,
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1201036935 18000 # Node ID 07117ac8c5fb5447dadc8efed51e8cf7077345d9 # Parent 977df56c5d87f5a90fc90400ecdc7d0687ce2d88 Clean up CSI/CSMI integration The functions responsible for actually firing the indication weren't really integrated in the first patch, so much as co-existing. This one attempts to reconcile the two approaches. Changes from #1 to #2: Fixed a segfault produced by a hard-coded constant (CS_MODIFIED) that should have been a variable (ind_type). Changes from #2 to #3: Added some more information to some failure messages in _do_indication. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 977df56c5d87 -r 07117ac8c5fb src/Virt_ComputerSystemIndication.c --- a/src/Virt_ComputerSystemIndication.c Tue Jan 22 16:16:08 2008 -0500 +++ b/src/Virt_ComputerSystemIndication.c Tue Jan 22 16:22:15 2008 -0500 @@ -48,6 +48,7 @@ static CMPI_THREAD_TYPE lifecycle_thread enum CS_EVENTS {CS_CREATED, CS_DELETED, + CS_MODIFIED, }; static pthread_cond_t lifecycle_cond = PTHREAD_COND_INITIALIZER; @@ -157,82 +158,70 @@ static bool dom_changed(struct dom_xml p return ret; } -static bool _lifecycle_indication(const CMPIBroker *broker, - const CMPIContext *ctx, - const CMPIObjectPath *newsystem, - const char *type) +static bool _do_indication(const CMPIBroker *broker, + const CMPIContext *ctx, + CMPIInstance *affected_inst, + int ind_type, + const char *ind_type_name, + char *prefix, + char *ns) { + CMPIObjectPath *affected_op; CMPIObjectPath *ind_op; CMPIInstance *ind; CMPIStatus s; + bool ret = true; ind = get_typed_instance(broker, - "Xen", /* Temporary hack */ - type, - NAMESPACE(newsystem)); + prefix, + ind_type_name, + ns); + + /* Generally report errors and hope to continue, since we have no one + to actually return status to. */ if (ind == NULL) { - CU_DEBUG("Failed to create ind"); - return false; + CU_DEBUG("Failed to create ind, type '%s:%s_%s'", + ns, + prefix, + ind_type_name); + ret = false; } ind_op = CMGetObjectPath(ind, &s); if (s.rc != CMPI_RC_OK) { - CU_DEBUG("Failed to get ind_op"); - return false; + CU_DEBUG("Failed to get ind_op. Error: '%s'", s.msg); + ret = false; } - CMSetProperty(ind, "AffectedSystem", - (CMPIValue *)&newsystem, CMPI_ref); + switch (ind_type) { + case CS_CREATED: + case CS_DELETED: + affected_op = CMGetObjectPath(affected_inst, &s); + if (s.rc != CMPI_RC_OK) { + ret = false; + CU_DEBUG("problem getting affected_op: '%s'", s.msg); + goto out; + } + CMSetProperty(ind, "AffectedSystem", + (CMPIValue *)&affected_op, CMPI_ref); + break; + case CS_MODIFIED: + CMSetProperty(ind, "PreviousInstance", + (CMPIValue *)&affected_inst, CMPI_instance); + break; + } CU_DEBUG("Delivering Indication: %s", - CMGetCharPtr(CMObjectPathToString(ind_op, NULL))); - - CBDeliverIndication(_BROKER, - ctx, - NAMESPACE(newsystem), - ind); - - - return true; -} - -static bool _do_modified_indication(const CMPIBroker *broker, - const CMPIContext *ctx, - CMPIInstance *mod_inst, - char *prefix, - char *ns) -{ - CMPIObjectPath *ind_op; - CMPIInstance *ind; - CMPIStatus s; - - ind = get_typed_instance(broker, - prefix, - "ComputerSystemModifiedIndication", - ns); - if (ind == NULL) { - CU_DEBUG("Failed to create ind"); - return false; - } - - ind_op = CMGetObjectPath(ind, &s); - if (s.rc != CMPI_RC_OK) { - CU_DEBUG("Failed to get ind_op"); - return false; - } - - CMSetProperty(ind, "PreviousInstance", - (CMPIValue *)&mod_inst, CMPI_instance); - - CU_DEBUG("Delivering Indication: %s\n", CMGetCharPtr(CMObjectPathToString(ind_op, NULL))); - CBDeliverIndication(_BROKER, + CBDeliverIndication(broker, ctx, - CIM_VIRT_NS, + ns, ind); + CU_DEBUG("Delivered"); - return true; + out: + return ret; } static bool wait_for_event(void) @@ -251,18 +240,12 @@ static bool wait_for_event(void) return true; } -static bool dom_in_list(virDomainPtr dom, int count, virDomainPtr *list) +static bool dom_in_list(char *uuid, int count, struct dom_xml *list) { - const char *_name; int i; - _name = virDomainGetName(dom); - for (i = 0; i < count; i++) { - const char *name; - - name = virDomainGetName(list[i]); - if (STREQ(name, _name)) + if (STREQ(uuid, list[i].uuid)) return true; } @@ -271,60 +254,20 @@ static bool dom_in_list(virDomainPtr dom static bool async_ind(CMPIContext *context, virConnectPtr conn, - const char *name, - int type) -{ - CMPIInstance *newinst; - CMPIObjectPath *op; - CMPIStatus s; - const char *type_name; - char *type_cn = NULL; - const char *ns = CIM_VIRT_NS; - - /* FIXME: Hmm, need to get the namespace a better way */ - - if (type == CS_CREATED) { - type_name = "ComputerSystemCreatedIndication"; - type_cn = get_typed_class(pfx_from_conn(conn), type_name); - - op = CMNewObjectPath(_BROKER, ns, type_cn, &s); - } else if (type == CS_DELETED) { - type_name = "ComputerSystemDeletedIndication"; - type_cn = get_typed_class(pfx_from_conn(conn), type_name); - - op = CMNewObjectPath(_BROKER, ns, type_cn, &s); - } else { - CU_DEBUG("Unknown event type: %i", type); - return false; - } - - if (type != CS_DELETED) - newinst = instance_from_name(_BROKER, conn, (char *)name, op); - else - /* A deleted domain will have no instance to lookup */ - newinst = CMNewInstance(_BROKER, op, &s); - - op = CMGetObjectPath(newinst, NULL); - - free(type_cn); - - return _lifecycle_indication(_BROKER, context, op, type_name); -} - -static bool mod_ind(CMPIContext *context, - virConnectPtr conn, - struct dom_xml prev_dom, - char *prefix, - char *ns) + int ind_type, + struct dom_xml prev_dom, + char *prefix, + char *ns) { bool rc; char *name = NULL; - CMPIInstance *mod_inst; + char *type_name = NULL; + CMPIInstance *affected_inst; - mod_inst = get_typed_instance(_BROKER, - prefix, - "ComputerSystem", - ns); + affected_inst = get_typed_instance(_BROKER, + prefix, + "ComputerSystem", + ns); name = sys_name_from_xml(prev_dom.xml); CU_DEBUG("Name for system: '%s'", name); @@ -333,12 +276,25 @@ static bool mod_ind(CMPIContext *context goto out; } - CMSetProperty(mod_inst, "Name", + switch (ind_type) { + case CS_CREATED: + type_name = "ComputerSystemCreatedIndication"; + break; + case CS_DELETED: + type_name = "ComputerSystemDeletedIndication"; + break; + case CS_MODIFIED: + type_name = "ComputerSystemModifiedIndication"; + break; + } + + CMSetProperty(affected_inst, "Name", (CMPIValue *)name, CMPI_chars); - CMSetProperty(mod_inst, "UUID", + CMSetProperty(affected_inst, "UUID", (CMPIValue *)prev_dom.uuid, CMPI_chars); - rc = _do_modified_indication(_BROKER, context, mod_inst, prefix, ns); + rc = _do_indication(_BROKER, context, affected_inst, + ind_type, type_name, prefix, ns); out: free(name); @@ -352,8 +308,7 @@ static CMPI_THREAD_RETURN lifecycle_thre CMPIStatus s; int prev_count; int cur_count; - virDomainPtr *prev_list; - virDomainPtr *cur_list; + virDomainPtr *tmp_list; struct dom_xml *cur_xml = NULL; struct dom_xml *prev_xml = NULL; virConnectPtr conn; @@ -363,66 +318,65 @@ static CMPI_THREAD_RETURN lifecycle_thre conn = connect_by_classname(_BROKER, args->classname, &s); if (conn == NULL) { CU_DEBUG("Failed to connect: %s", CMGetCharPtr(s.msg)); - return NULL; + goto out; } pthread_mutex_lock(&lifecycle_mutex); CBAttachThread(_BROKER, context); - prev_count = get_domain_list(conn, &prev_list); - s = doms_to_xml(&prev_xml, prev_list, prev_count); + prev_count = get_domain_list(conn, &tmp_list); + s = doms_to_xml(&prev_xml, tmp_list, prev_count); if (s.rc != CMPI_RC_OK) CU_DEBUG("doms_to_xml failed. Attempting to continue."); - + free_domain_list(tmp_list, prev_count); + free(tmp_list); CU_DEBUG("entering event loop"); while (lifecycle_enabled) { int i; bool res; - cur_count = get_domain_list(conn, &cur_list); - s = doms_to_xml(&cur_xml, cur_list, cur_count); + cur_count = get_domain_list(conn, &tmp_list); + s = doms_to_xml(&cur_xml, tmp_list, cur_count); if (s.rc != CMPI_RC_OK) CU_DEBUG("doms_to_xml failed."); + free_domain_list(tmp_list, cur_count); + free(tmp_list); for (i = 0; i < cur_count; i++) { - res = dom_in_list(cur_list[i], prev_count, prev_list); + res = dom_in_list(cur_xml[i].uuid, prev_count, prev_xml); if (!res) - async_ind(context, - conn, - virDomainGetName(cur_list[i]), - CS_CREATED); + async_ind(context, conn, CS_CREATED, + cur_xml[i], prefix, ns); + } for (i = 0; i < prev_count; i++) { - res = dom_in_list(prev_list[i], cur_count, cur_list); + res = dom_in_list(prev_xml[i].uuid, cur_count, cur_xml); if (!res) - async_ind(context, - conn, - virDomainGetName(prev_list[i]), - CS_DELETED); + async_ind(context, conn, CS_DELETED, + prev_xml[i], prefix, ns); } for (i = 0; i < prev_count; i++) { res = dom_changed(prev_xml[i], cur_xml, cur_count); if (res) { - CU_DEBUG("Domain '%s' modified.", prev_xml[i].uuid); - mod_ind(context, conn, prev_xml[i], prefix, ns); + async_ind(context, conn, CS_MODIFIED, + prev_xml[i], prefix, ns); + } free_dom_xml(prev_xml[i]); } - free_domain_list(prev_list, prev_count); - - free(prev_list); free(prev_xml); - prev_list = cur_list; prev_xml = cur_xml; prev_count = cur_count; wait_for_event(); } + + out: pthread_mutex_unlock(&lifecycle_mutex); free_ind_args(&args); free(prefix);
Jay Gagnon wrote:
This one stops leaking some of the memory from the ind_args struct and adds a little more verbosity to some error messages per Dan's request. I didn't add system name to the "Failed to create ind" one, because it was not readily available already and the extra code to get it didn't seem worth it, but I put in all the information that is available at that point.
Ah, forgot the #3 on the subject. -- -Jay
Jay Gagnon wrote:
This one stops leaking some of the memory from the ind_args struct and adds a little more verbosity to some error messages per Dan's request. I didn't add system name to the "Failed to create ind" one, because it was not readily available already and the extra code to get it didn't seem worth it, but I put in all the information that is available at that point.
No complaints with this set. +1 -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
participants (2)
-
Jay Gagnon -
Kaitlin Rupert