12:24 p.m.
JG> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
JG> +++ b/src/Virt_ComputerSystemModifiedIndication.c Thu Jan 10 11:33:17 2008 -0500
I guess I had assumed that the modified indication would be integrated
into the ComputerSystemIndication provider, instead of broken out
alone (since the other handles Create and Destroy as well). Is there
any reason that we shouldn't merge the two?
JG> +int free_dom_xml (struct dom_xml dom)
JG> +{
JG> + free(dom.uuid);
JG> + free(dom.xml);
JG> + return 0;
JG> +}
Looks like this could probably be a void function, which is what we do
for the cleanup functions in device_parsing.
JG> +static bool _lifecycle_indication(const CMPIBroker *broker,
JG> + const CMPIContext *ctx,
JG> + CMPIInstance *mod_inst)
JG> +{
JG> + CMPIObjectPath *ind_op;
JG> + CMPIInstance *ind;
JG> + CMPIStatus s;
JG> +
JG> + ind = get_typed_instance(broker,
JG> + "Xen", /* Temporary hack */
JG> + "ComputerSystemModifiedIndication",
JG> + CIM_VIRT_NS);
To determine the prefix, couldn't you get the prefix from mod_inst? I
assume that mod_inst is a Xen_ComputerSystem (or similar), so you
could grab the prefix from there.
JG> + CBDeliverIndication(_BROKER,
JG> + ctx,
JG> + CIM_VIRT_NS,
JG> + ind);
JG> +
JG> +
JG> + return true;
You've got an extra blank line in there :)
JG> +static char *sys_name_from_xml(char *xml)
JG> +{
JG> + char *tmp;
JG> + char *start;
JG> + char *end;
JG> + char *name;
JG> +
JG> + /* Has to be a better way. */
JG> + tmp = strdup(xml);
JG> + start = strstr(tmp, "<name>");
JG> + start += 6;
JG> + end = strstr(start, "</name>");
JG> + end[0] = '\0';
Perhaps something like this:
char *name = NULL;
int ret;
ret = sscanf(xml, "<name>%a[^<]</name>", &name);
if (ret != 1) {
/* Error */
}
JG> +static bool async_ind(CMPIContext *context,
JG> + virConnectPtr conn,
JG> + struct dom_xml prev_dom)
JG> +{
JG> + bool rc;
JG> + char *name = NULL;
JG> + CMPIInstance *mod_inst;
JG> + const char *ns = CIM_VIRT_NS;
JG> +
JG> + /* Where should we be getting the namespace and classname? */
JG> + mod_inst = get_typed_instance(_BROKER,
JG> + "Xen",
JG> + "ComputerSystem",
JG> + ns);
I think using CIM_VIRT_NS is the right approach here. We use it
in other places where we have to create an instance with no
reference.
Read on for a comment about the prefix.
JG> +static CMPIStatus doms_to_xml(struct dom_xml **dom_xml_list,
JG> + virDomainPtr *dom_ptr_list,
JG> + int dom_ptr_count)
JG> +{
JG> + int i;
JG> + int rc;
JG> + char *xml = NULL;
JG> + char uuid[VIR_UUID_STRING_BUFLEN];
JG> + CMPIStatus s = {CMPI_RC_OK, NULL};
JG> +
JG> + *dom_xml_list = malloc(dom_ptr_count * sizeof(struct dom_xml));
Using calloc() here would be more conventional.
JG> + for (i = 0; i < dom_ptr_count; i++) {
JG> + rc = virDomainGetUUIDString(dom_ptr_list[i], uuid);
Why not just get the UUID directly to the dom_xml_list?
rc = virDomainGetUUIDString(dom_ptr_list[i], (*dom_xml_list)[i].uuid);
JG> + xml = virDomainGetXMLDesc(dom_ptr_list[i], 0);
JG> + if (xml == NULL) {
JG> + cu_statusf(broker, &s,
JG> + CMPI_RC_ERR_FAILED,
JG> + "Failed to get xml desc");
JG> + break;
JG> + }
JG> +
JG> + (*dom_xml_list)[i].xml = strdup(xml);
Do you need to strdup() the xml? It's just a malloc'd string anyway,
right?
JG> +static CMPI_THREAD_RETURN lifecycle_thread(void *params)
JG> +{
JG> + int i;
JG> + int cur_count;
JG> + int prev_count;
JG> + virConnectPtr conn;
JG> + virDomainPtr *tmp_list;
JG> + struct dom_xml *cur_xml = NULL;
JG> + struct dom_xml *prev_xml = NULL;
JG> + CMPIStatus s = {CMPI_RC_OK, NULL};
JG> + CMPIContext *context = (CMPIContext *)params;
JG> +
JG> + /* We can't use this anymore, can we? */
Correct :)
JG> + conn = lv_connect(_BROKER, &s);
This will only connect to Xen (if present) or KVM (if present), but
not both.
You need to get the prefix of yourself (i.e. if this is a KVM_ or Xen_
indication) and use that to connect_by_classname(). Thus, if a client
subscribes to a Xen_ indication, and another subscribes to a KVM_
indication, then there will be two threads.
I think that logic would be another good reason to merge this into the
original ComputerSystemIndication provider, so you can share that
logic (the other provider currently lacks it as well).
JG> + for (i = 0; i < prev_count; i++) {
JG> + virDomainFree(tmp_list[i]);
JG> + }
Perhaps we need a cleanup_domain_list() function?
JG> +
JG> + CU_DEBUG("entering event loop");
JG> + while (lifecycle_enabled) {
JG> + bool modified;
JG> +
JG> + cur_count = get_domain_list(conn, &tmp_list);
JG> + s = doms_to_xml(&cur_xml, tmp_list, cur_count);
JG> + /* TODO: status check */
JG> + for (i = 0; i < cur_count; i++) {
JG> + virDomainFree(tmp_list[i]);
JG> + }
JG> +
JG> + for (i = 0; i < prev_count; i++) {
JG> + modified = dom_changed(prev_xml[i], cur_xml, cur_count);
JG> + if (modified) {
JG> + CU_DEBUG("Domain '%s'
modified.",
JG> + prev_xml[i].uuid);
JG> + async_ind(context, conn, prev_xml[i]);
JG> + }
JG> +
JG> + free_dom_xml(prev_xml[i]);
JG> + }
JG> +
JG> + free(prev_xml);
JG> + prev_xml = cur_xml;
JG> + wait_for_event();
JG> + }
See, I think the above loop can detect additions, subtractions, and
changes. This will avoid having multiple threads banging on libvirt
every cycle.
JG> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
JG> +++ b/src/Virt_ComputerSystemModifiedIndication.h Thu Jan 10 11:33:17 2008 -0500
JG> @@ -0,0 +1,37 @@
<snip>
JG> +struct dom_xml {
JG> + char *uuid;
JG> + char *xml;
JG> +};
JG> +
JG> +int free_dom_xml(struct dom_xml dom);
I don't think these need to be known outside of the provider, do they?
In that case, you can just do away with this whole header and define
the struct in the .c file.
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com