[PATCH 0 of 2] [RFC] ComputerSystemModifiedIndication
This is still pretty rough around the edges but it seems functional enough that it's worth showing, and I've got a few spots where I could use some input. Right now it does in fact notice a domain that changes and fires an indication. The "previous instance" embedded object has it's Name and UUID fields filled in; I couldn't find any other fields that seemed terribly relevant to fill in. The places where I could use some advice: In _lifecycle_indication, the get_typed_instance args are mostly hardcoded. This seems rather bad, but I'm not quite sure where I should get them. It's not like I have a reference being passed in to pull that out of. The entire sys_name_from_xml feels like an abomination, but sscanf falls short and full xml parsing seems way overkill. Any suggestions on how to make it safer/more sane are welcome. In async_ind, I once again use some hardcoded values because I'm not sure where to get them. In lifecycle_thread, I use lv_connect because I don't think I have the requisite info for the preferred method, but AFAIK lv_connect usage is frowned upon now. I also am not sure what to do here when I get errors. Should I terminate the event loop? Where would I even return an error message to the client?
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1199982797 18000 # Node ID 6f3a50557ab0648b1fa0bde747914d6e58027eef # Parent ca8dc23eacc44970a2d79978bc9de65aaa600b92 Add Virt_ComputerSystemModifiedIndication, which watches for changes in our guests. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r ca8dc23eacc4 -r 6f3a50557ab0 src/Virt_ComputerSystemModifiedIndication.c --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/src/Virt_ComputerSystemModifiedIndication.c Thu Jan 10 11:33:17 2008 -0500 @@ -0,0 +1,379 @@ +/* + * Copyright IBM Corp. 2007 + * + * Authors: + * Jay Gagnon <grendel@linux.vnet.ibm.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ +#include <unistd.h> +#include <stdio.h> +#include <fcntl.h> +#include <string.h> +#include <stdlib.h> +#include <pthread.h> +#include <time.h> + +#include <cmpidt.h> +#include <cmpift.h> +#include <cmpimacs.h> + +#include <libvirt/libvirt.h> + +#include <libcmpiutil.h> +#include <misc_util.h> +#include <std_indication.h> +#include <cs_util.h> + +#include "config.h" + +#include "Virt_ComputerSystem.h" +#include "Virt_ComputerSystemModifiedIndication.h" + +static const CMPIBroker *_BROKER; + +static CMPI_THREAD_TYPE lifecycle_thread_id = 0; + +enum CS_EVENTS { + CS_MODIFIED, +}; + +static pthread_cond_t lifecycle_cond = PTHREAD_COND_INITIALIZER; +static pthread_mutex_t lifecycle_mutex = PTHREAD_MUTEX_INITIALIZER; +static bool lifecycle_enabled = 0; + +#ifdef CMPI_EI_VOID +# define _EI_RTYPE void +# define _EI_RET() return +#else +# define _EI_RTYPE CMPIStatus +# define _EI_RET() return (CMPIStatus){CMPI_RC_OK, NULL} +#endif + +int free_dom_xml (struct dom_xml dom) +{ + free(dom.uuid); + free(dom.xml); + return 0; +} + +static bool _lifecycle_indication(const CMPIBroker *broker, + const CMPIContext *ctx, + CMPIInstance *mod_inst) +{ + CMPIObjectPath *ind_op; + CMPIInstance *ind; + CMPIStatus s; + + ind = get_typed_instance(broker, + "Xen", /* Temporary hack */ + "ComputerSystemModifiedIndication", + CIM_VIRT_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); + + printf("Delivering Indication: %s\n", + CMGetCharPtr(CMObjectPathToString(ind_op, NULL))); + + CBDeliverIndication(_BROKER, + ctx, + CIM_VIRT_NS, + ind); + + + return true; +} + +static bool wait_for_event(void) +{ + struct timespec timeout; + int ret; + + + clock_gettime(CLOCK_REALTIME, &timeout); + timeout.tv_sec += 3; + + ret = pthread_cond_timedwait(&lifecycle_cond, + &lifecycle_mutex, + &timeout); + + return true; +} + +static char *sys_name_from_xml(char *xml) +{ + char *tmp; + char *start; + char *end; + char *name; + + /* Has to be a better way. */ + tmp = strdup(xml); + start = strstr(tmp, "<name>"); + start += 6; + end = strstr(start, "</name>"); + end[0] = '\0'; + + name = strdup(start); + free(tmp); + + return name; +} + +static bool async_ind(CMPIContext *context, + virConnectPtr conn, + struct dom_xml prev_dom) +{ + bool rc; + char *name = NULL; + CMPIInstance *mod_inst; + const char *ns = CIM_VIRT_NS; + + /* Where should we be getting the namespace and classname? */ + mod_inst = get_typed_instance(_BROKER, + "Xen", + "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 = _lifecycle_indication(_BROKER, context, mod_inst); + + out: + free(name); + return rc; +} + +static CMPIStatus doms_to_xml(struct dom_xml **dom_xml_list, + virDomainPtr *dom_ptr_list, + int dom_ptr_count) +{ + int i; + int rc; + char *xml = NULL; + char uuid[VIR_UUID_STRING_BUFLEN]; + CMPIStatus s = {CMPI_RC_OK, NULL}; + + *dom_xml_list = malloc(dom_ptr_count * sizeof(struct dom_xml)); + for (i = 0; i < dom_ptr_count; i++) { + rc = virDomainGetUUIDString(dom_ptr_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; + } + + xml = virDomainGetXMLDesc(dom_ptr_list[i], 0); + if (xml == NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed to get xml desc"); + break; + } + + (*dom_xml_list)[i].xml = strdup(xml); + (*dom_xml_list)[i].uuid = strdup(uuid); + + free(xml); + } + + 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 CMPI_THREAD_RETURN lifecycle_thread(void *params) +{ + int i; + int cur_count; + int prev_count; + virConnectPtr conn; + virDomainPtr *tmp_list; + struct dom_xml *cur_xml = NULL; + struct dom_xml *prev_xml = NULL; + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIContext *context = (CMPIContext *)params; + + /* We can't use this anymore, can we? */ + conn = lv_connect(_BROKER, &s); + + pthread_mutex_lock(&lifecycle_mutex); + + CBAttachThread(_BROKER, context); + + prev_count = get_domain_list(conn, &tmp_list); + s = doms_to_xml(&prev_xml, tmp_list, prev_count); + /* TODO: status check */ + for (i = 0; i < prev_count; i++) { + virDomainFree(tmp_list[i]); + } + + CU_DEBUG("entering event loop"); + while (lifecycle_enabled) { + bool modified; + + cur_count = get_domain_list(conn, &tmp_list); + s = doms_to_xml(&cur_xml, tmp_list, cur_count); + /* TODO: status check */ + for (i = 0; i < cur_count; i++) { + virDomainFree(tmp_list[i]); + } + + for (i = 0; i < prev_count; i++) { + modified = dom_changed(prev_xml[i], cur_xml, cur_count); + if (modified) { + CU_DEBUG("Domain '%s' modified.", + prev_xml[i].uuid); + async_ind(context, conn, prev_xml[i]); + } + + free_dom_xml(prev_xml[i]); + } + + free(prev_xml); + prev_xml = cur_xml; + wait_for_event(); + } + CU_DEBUG("exiting event loop"); + + pthread_mutex_unlock(&lifecycle_mutex); + + return NULL; +} + +static CMPIStatus ActivateFilter(CMPIIndicationMI* mi, + const CMPIContext* ctx, + const CMPISelectExp* se, + const char *ns, + const CMPIObjectPath* op, + CMPIBoolean first) +{ + CU_DEBUG("ActivateFilter"); + if (lifecycle_thread_id == 0) { + CMPIContext *thread_context; + + thread_context = CBPrepareAttachThread(_BROKER, ctx); + + lifecycle_thread_id = _BROKER->xft->newThread(lifecycle_thread, + thread_context, + 0); + } + + return (CMPIStatus){CMPI_RC_OK, NULL}; +} + +static CMPIStatus DeActivateFilter(CMPIIndicationMI* mi, + const CMPIContext* ctx, + const CMPISelectExp* se, + const char *ns, + const CMPIObjectPath* op, + CMPIBoolean last) +{ + return (CMPIStatus){CMPI_RC_OK, NULL}; +} + +static _EI_RTYPE EnableIndications(CMPIIndicationMI* mi, + const CMPIContext *ctx) +{ + pthread_mutex_lock(&lifecycle_mutex); + lifecycle_enabled = true; + pthread_mutex_unlock(&lifecycle_mutex); + + CU_DEBUG("ComputerSystemModifiedIndication enabled"); + + _EI_RET(); +} + +static _EI_RTYPE DisableIndications(CMPIIndicationMI* mi, + const CMPIContext *ctx) +{ + pthread_mutex_lock(&lifecycle_mutex); + lifecycle_enabled = false; + pthread_mutex_unlock(&lifecycle_mutex); + + CU_DEBUG("ComputerSystemModifiedIndication disabled"); + + _EI_RET(); +} + +static CMPIStatus trigger_indication(const CMPIContext *context) +{ + pthread_cond_signal(&lifecycle_cond); + CU_DEBUG("CSMI triggered"); + return(CMPIStatus){CMPI_RC_OK, NULL}; +} + +static struct std_indication_handler csmi = { + .raise_fn = NULL, + .trigger_fn = trigger_indication, +}; + +DEFAULT_IND_CLEANUP(); +DEFAULT_AF(); +DEFAULT_MP(); + +STDI_IndicationMIStub(, Virt_ComputerSystemModifiedIndicationProvider, + _BROKER, libvirt_cim_init(), &csmi); + +/* + * Local Variables: + * mode: C + * c-set-style: "K&R" + * tab-width: 8 + * c-basic-offset: 8 + * indent-tabs-mode: nil + * End: + */ diff -r ca8dc23eacc4 -r 6f3a50557ab0 src/Virt_ComputerSystemModifiedIndication.h --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/src/Virt_ComputerSystemModifiedIndication.h Thu Jan 10 11:33:17 2008 -0500 @@ -0,0 +1,37 @@ +/* + * Copyright IBM Corp. 2007 + * + * Authors: + * Jay Gagnon <grendel@linux.vnet.ibm.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +struct dom_xml { + char *uuid; + char *xml; +}; + +int free_dom_xml(struct dom_xml dom); + +/* + * Local Variables: + * mode: C + * c-set-style: "K&R" + * tab-width: 8 + * c-basic-offset: 8 + * indent-tabs-mode: nil + * End: + */
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@us.ibm.com
Dan Smith wrote:
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?
Huh, I hadn't even thought of that. Yea, that could probably work. I don't think integrating should be too tough. Only issue might be that my async_ind() function is a bit different from the CSI one.
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.
Sure, no prob.
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.
I found the comment further down about prefix; I'll respond 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 :)
Oops. :)
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 */ }
Ah, this explains my frustration with sscanf. I had been operating under the assumption that I need some sort of wildcarding before and after the relevant string, and was getting very upset with sscanf for not appearing to support wildcarding. Not sure why I had determined I needed it, but this can work.
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.
Okay.
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?
Heh, good call.
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? Heh, I think my not-C roots are showing there. I was paying too much attention to being safe with memory and totally didn't realize I was wasting work there.
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).
Okay, yea that sounds fine. As I said before, the actual process of firing the indication might be a little hairy to merge in, but I think I'll manage.
JG> + for (i = 0; i < prev_count; i++) { JG> + virDomainFree(tmp_list[i]); JG> + }
Perhaps we need a cleanup_domain_list() function?
Yea, I do have that block twice in the same function, after all. I didn't make it separate earlier with some sort of "it's only three lines" rationalization, but it's definitely more in sync with how we do things.
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.
I can certainly give it a shot. I'm more comfortable with indications in general now, so modifying it won't seem so daunting.
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.
I had gone back in forth on that one, so whichever way you prefer is fine by me. Okay, so lots of stuff, which is good; definitely better than, "I dunno looks fine here," since I knew it was a bit shaky. I'll fix up the small stuff and then start trying to wedge it into the existing CSI. -- -Jay
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1199982800 18000 # Node ID ddafd609bda2f33a7c4cb7943ee8e08e22a3f32f # Parent 6f3a50557ab0648b1fa0bde747914d6e58027eef Makefile changes and schema file additions for ComputerSystemModifiedIndication. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 6f3a50557ab0 -r ddafd609bda2 Makefile.am --- a/Makefile.am Thu Jan 10 11:33:17 2008 -0500 +++ b/Makefile.am Thu Jan 10 11:33:20 2008 -0500 @@ -24,6 +24,7 @@ MOFS = \ schema/RegisteredProfile.mof \ schema/ElementConformsToProfile.mof \ schema/ComputerSystemIndication.mof \ + schema/ComputerSystemModifiedIndication.mof \ schema/ResourceAllocationSettingData.mof \ schema/ResourcePoolConfigurationService.mof \ schema/ResourcePoolConfigurationCapabilities.mof \ @@ -65,6 +66,7 @@ REGS = \ schema/DiskPool.registration \ schema/HostedResourcePool.registration \ schema/ComputerSystemIndication.registration \ + schema/ComputerSystemModifiedIndication.registration \ schema/ResourceAllocationSettingData.registration \ schema/ResourcePoolConfigurationService.registration \ schema/ResourcePoolConfigurationCapabilities.registration \ diff -r 6f3a50557ab0 -r ddafd609bda2 src/Makefile.am --- a/src/Makefile.am Thu Jan 10 11:33:17 2008 -0500 +++ b/src/Makefile.am Thu Jan 10 11:33:20 2008 -0500 @@ -30,6 +30,7 @@ provider_LTLIBRARIES = libVirt_ComputerS libVirt_Device.la \ libVirt_SystemDevice.la \ libVirt_ComputerSystemIndication.la \ + libVirt_ComputerSystemModifiedIndication.la \ libVirt_RASD.la \ libVirt_HostSystem.la \ libVirt_VirtualSystemManagementService.la \ @@ -61,6 +62,10 @@ libVirt_ComputerSystemIndication_la_DEPE libVirt_ComputerSystemIndication_la_DEPENDENCIES = libVirt_ComputerSystem.la libVirt_ComputerSystemIndication_la_SOURCES = Virt_ComputerSystemIndication.c libVirt_ComputerSystemIndication_la_LIBADD = -lVirt_ComputerSystem -lpthread -lrt + +libVirt_ComputerSystemModifiedIndication_la_DEPENDENCIES = libVirt_ComputerSystem.la +libVirt_ComputerSystemModifiedIndication_la_SOURCES = Virt_ComputerSystemModifiedIndication.c +libVirt_ComputerSystemModifiedIndication_la_LIBADD = -lVirt_ComputerSystem -lpthread -lrt libVirt_VirtualSystemManagementService_la_DEPENDENCIES = libVirt_ComputerSystem.la libVirt_ComputerSystemIndication.la libVirt_RASD.la libVirt_HostSystem.la libVirt_VirtualSystemManagementService_la_SOURCES = Virt_VirtualSystemManagementService.c
Jay Gagnon wrote:
This is still pretty rough around the edges but it seems functional enough that it's worth showing, and I've got a few spots where I could use some input. Right now it does in fact notice a domain that changes and fires an indication. The "previous instance" embedded object has it's Name and UUID fields filled in; I couldn't find any other fields that seemed terribly relevant to fill in.
The places where I could use some advice:
In _lifecycle_indication, the get_typed_instance args are mostly hardcoded. This seems rather bad, but I'm not quite sure where I should get them. It's not like I have a reference being passed in to pull that out of.
The entire sys_name_from_xml feels like an abomination, but sscanf falls short and full xml parsing seems way overkill. Any suggestions on how to make it safer/more sane are welcome.
In async_ind, I once again use some hardcoded values because I'm not sure where to get them.
In lifecycle_thread, I use lv_connect because I don't think I have the requisite info for the preferred method, but AFAIK lv_connect usage is frowned upon now. I also am not sure what to do here when I get errors. Should I terminate the event loop? Where would I even return an error message to the client?
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
One thing I think I forgot to mention here (and any rate haven't seen any comments on) is what I should do when the indication event loop is doing its thing, it doesn't really know what to do with an error. Say for example, that it fails at getting the xml description for a domain. In most providers, I would use cu_statusf and bail, but here I've got nobody to give my status to and nowhere to bail to, other than what amounts to an untimely death. Is there a mechanism for indications reporting errors, beyond CU_DEBUG'ing "help" and hoping to recover? -- -Jay
JG> Is there a mechanism for indications reporting errors, beyond JG> CU_DEBUG'ing "help" and hoping to recover? I would expect that the answer will be that there isn't much that can be done. A CU_DEBUG() indicating the issue is certainly required, but I'm not sure what else we have. I tend to think that indications are mostly for convenience and optimization, to avoid pinging the providers unnecessarily, in which case an error wouldn't be catastrophic. Multiple clients monitoring a single provider would use them to notice when the other makes a change. Maybe Heidi has a better answer for this? -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> Is there a mechanism for indications reporting errors, beyond JG> CU_DEBUG'ing "help" and hoping to recover?
I would expect that the answer will be that there isn't much that can be done. A CU_DEBUG() indicating the issue is certainly required, but I'm not sure what else we have.
CU_DEBUG() is the providers possibility to report an error. A mechanism to report a failure in the indication processing itself to the subscriber of the ComputerSystemModfiedIndication can not be done via the ComputerSystemModifiedIndication. This indication will only be thrown, if all went fine. So an indication in general is only reporting positive results - means, if the filter requirement is fulfilled, the indication is thrown. To monitor a failing indication processing an additional indication provider would be necessary. This one would then handle "please inform me, if something in the processing of the ComputerSystemModfiedIndication failed". So a client would need to subscribe 1) to the ComputerSystemModifiedIndication to get informed of the modification and 2) to the AlertIndication monitoring the indication processing to get informed of a failing processing.
I tend to think that indications are mostly for convenience and optimization, to avoid pinging the providers unnecessarily, in which case an error wouldn't be catastrophic. Multiple clients monitoring a single provider would use them to notice when the other makes a change.
I absolutely agree. For now I would say, its more important to have the ComputerSystemModifiedIndication. But at a later point in time - an indication provider who reports processing failures could be added as a feature. -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
Dan Smith wrote:
JG> Is there a mechanism for indications reporting errors, beyond JG> CU_DEBUG'ing "help" and hoping to recover?
I would expect that the answer will be that there isn't much that can be done. A CU_DEBUG() indicating the issue is certainly required, but I'm not sure what else we have.
CU_DEBUG() is the providers possibility to report an error. A mechanism to report a failure in the indication processing itself to the subscriber of the ComputerSystemModfiedIndication can not be done via the ComputerSystemModifiedIndication. This indication will only be thrown, if all went fine. So an indication in general is only reporting positive results - means, if the filter requirement is fulfilled, the indication is thrown. To monitor a failing indication processing an additional indication provider would be necessary. This one would then handle "please inform me, if something in the processing of the ComputerSystemModfiedIndication failed". So a client would need to subscribe 1) to the ComputerSystemModifiedIndication to get informed of the modification and 2) to the AlertIndication monitoring the indication processing to get informed of a failing processing. Excellent explanation, thanks for that. :) Fortunately it matches
Heidi Eckhart wrote: pretty closely to my assumption for how things would work, which is always nice.
I tend to think that indications are mostly for convenience and optimization, to avoid pinging the providers unnecessarily, in which case an error wouldn't be catastrophic. Multiple clients monitoring a single provider would use them to notice when the other makes a change.
I absolutely agree. For now I would say, its more important to have the ComputerSystemModifiedIndication. But at a later point in time - an indication provider who reports processing failures could be added as a feature.
Good, we're all on the same page here. I'll worry about the failure reporting later (I'll leave the CU_DEBUG calls in for our testing convenience). -- -Jay
participants (3)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon