10:34 a.m.
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?
10:34 a.m.
New subject: [PATCH 1 of 2] Add Virt_ComputerSystemModifiedIndication, which watches for changes in our guests
# HG changeset patch
# User Jay Gagnon <grendel(a)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(a)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(a)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(a)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:
+ */
12:24 p.m.
New subject: [PATCH 1 of 2] Add Virt_ComputerSystemModifiedIndication, which watches for changes in our guests
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
1:48 p.m.
New subject: [PATCH 1 of 2] Add Virt_ComputerSystemModifiedIndication, which watches for changes in our guests
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
10:34 a.m.
New subject: [PATCH 2 of 2] Makefile changes and schema file additions for ComputerSystemModifiedIndication
# HG changeset patch
# User Jay Gagnon <grendel(a)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(a)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
3:35 p.m.
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(a)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
3:45 p.m.
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(a)us.ibm.com
5:34 a.m.
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
8:57 a.m.
Heidi Eckhart wrote:
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
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
6140
days inactive
6147
days old
8 comments
3 participants
participants (3)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon