[Libvirt-cim] [PATCH 1 of 2] Add Virt_ComputerSystemModifiedIndication, which watches for changes in our guests
Jay Gagnon
grendel at linux.vnet.ibm.com
Thu Jan 10 19:48:07 UTC 2008
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
More information about the Libvirt-cim
mailing list