March 2013 - CIM - libvirt List Archives

Re: [Libvirt-cim] [PATCH V3 00/11] bug fix and enhancement patch rebase for 6.2
by John Ferlan 12 Mar '13

12 Mar '13

Now that I've gone through the series - hopefully that gives you enough to put out another series. Once we get through it, then we can work with DV in order to go through the next steps. I'll look at the libcmpiutil changes next. John

2 1

Re: [Libvirt-cim] [PATCH V4] VSSD, fix a error report issue in VSSD enum
by John Ferlan 12 Mar '13

12 Mar '13

Since I don't have the original email to reply-to, here is a link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00038.html In instance_from_dom(): * virDomainIsActive returns: Returns 1 if running, 0 if inactive, -1 on error. You are testing for < 0 (error condition) and an else with no determination about whether it's really active or not. Your comments in the code and the CU_DEBUG statements don't seem to sync. How is VSSD_RECOVERABLE true when active = -1? NOTE: In looking at get_dominfo() again - and thinking about this as a caller - why does it matter if autostart is set or not? I really think the "get_domain_list()" should change to use virConnectListAllDomains() - it may solve more timing issues. John

2 1

Re: [Libvirt-cim] [PATCH V3 09/11] CSI Discard libvirt event by default
by John Ferlan 12 Mar '13

12 Mar '13

Since I don't have the original email to reply-to, here is the link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00032.html Seems this was "pulled back" from some other commit - might be nice to reference the commit id where it was removed and now replaced - at least for historical purposes. src/Virt_ComputerSystem.c In trigger_mod_indication() * The "goto out" if/when "(ind == NULL)" will perhaps trigger a false positive this succeeded since "s" is initialized to CMPI_RC_OK. src/Virt_ComputerSystemIndication.c In doms_to_xml(), * the calloc() has no check for successful return of memory leading to possible NULL pointer deref of *dom_xml_list. In async_ind_native(): * The CU_DEBUG() message "...indication diliv..." should be "...indication delivery..." * The CU_DEBUG("Unrecognized indication type"); should report what 'ind_type' is (btw: it's unrecognised) In lifecycle_thread_native(): * Seems inefficient to traverse lists so many times. In the for loop running through prev_xml, it would seem that if 'dom_in_list()' returns 1, then we could call 'dom_changed()' and possibly 'async_ind_native()' if it has changed. * Since this routine keeps lifecycle_mutex for the duration - I believe it makes it even more important to separate it from 'dom_list' in your previous change... NOTE: the get_domain_list() call could be made better by getting all "Active and Defined" domains in one virConnectListAllDomains() call, but that's mostly unrelated to this code. Getting the list of All domains at one time is "preferred" since you won't run into odd timing issues by making two calls. In raise_indication() * The returns for the strdup()'s are not checked. John

2 1

Re: [Libvirt-cim] [PATCH V3 08/11] device parsing, add debug print
by John Ferlan 12 Mar '13

12 Mar '13

Since I didn't have the original email to reply-to, here is the link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00029.html Should the following return 0 as well?: + if (ret != 1) { + CU_DEBUG("Failed to translate xml into struct domain"); + } Especially since get_dominfo_from_xml() will free and initialize dominfo to NULL. You'll also need a "free(xml);" if you return. That's a memory leak. What probably should be done is (btw: 8 space tabs are hard to do!) ret = get_dominfo_from_xml(xml, dominfo); if (ret == 0) { CU_DEBUG("Failed to translate xml into struct domain"); goto out; } ret = virDomainGetAutostart(dom, &start); if (ret != 0) { CU_DEBUG("Failed to get dom autostart with libvirt API."); goto out; } (*dominfo)->autostrt = start; out: free(xml); return ret; John

2 1

Re: [Libvirt-cim] [PATCH V3 01/11] remove script for bridge network
by John Ferlan 12 Mar '13

12 Mar '13

Since I don't have the original email to reply-to for a code review, here is a link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00030.html NACK As previously noted, a script is supported (and perhaps required) in the Xen guest. Consider the following, which adds a parameter to the call in order to allow for Xen: -static const char *bridge_net_to_xml(xmlNodePtr nic, struct net_device *dev) +static const char *bridge_net_to_xml(xmlNodePtr nic, struct net_device *dev, + int domtype) { const char *script = "vif-bridge"; xmlNodePtr tmp; const char *msg = NULL; - tmp = xmlNewChild(nic, NULL, BAD_CAST "script", NULL); - if (tmp == NULL) - return XML_ERROR; - xmlNewProp(tmp, BAD_CAST "path", BAD_CAST script); + /* Scripts only supported on Xen guests see 'libvirt' + * commit id 1734cdb99 (since 0.9.10) */ + if (domtype == DOMAIN_XENPV || domtype == DOMAIN_XENFV) { + tmp = xmlNewChild(nic, NULL, BAD_CAST "script", NULL); + if (tmp == NULL) + return XML_ERROR; + xmlNewProp(tmp, BAD_CAST "path", BAD_CAST script); + } msg = set_net_source(nic, dev, "bridge"); @@ -375,13 +380,13 @@ static const char *net_xml(xmlNodePtr root, struct domain } #endif - if (STREQ(dev->dev.net.type, "network")) + if (STREQ(dev->dev.net.type, "network")) { msg = set_net_source(nic, net, "network"); - else if (STREQ(dev->dev.net.type, "bridge")) - msg = bridge_net_to_xml(nic, net); - else if (STREQ(dev->dev.net.type, "user")) + } else if (STREQ(dev->dev.net.type, "bridge")) { + msg = bridge_net_to_xml(nic, net, dominfo->type); + } else if (STREQ(dev->dev.net.type, "user")) { continue; - else if (STREQ(dev->dev.net.type, "direct")) { + } else if (STREQ(dev->dev.net.type, "direct")) { msg = set_net_source(nic, net, "direct"); if (net->vsi.vsi_type != NULL) { struct vsi_device *vsi = &dev->dev.net.vsi;

2 1

Re: [Libvirt-cim] [PATCH V3 2/2] libcmpiutil: add time and thread info in debug log
by John Ferlan 11 Mar '13

11 Mar '13

Since I don't have the original email to reply-to, here's the link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00020.html ACK

1 0

Re: [Libvirt-cim] [PATCH V3 1/2] libcmpiutil: fix potential debug print crash
by John Ferlan 11 Mar '13

11 Mar '13

Since I don't have the original email to reply-to, here's a link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00019.html ACK John

1 0

Re: [Libvirt-cim] [PATCH V3 10/11] VSSD, fix a error report issue in VSSD enum
by John Ferlan 11 Mar '13

11 Mar '13

Since I don't have the original email to reply-to, here is a link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00031.html NACK since this has been replaced by: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00038.html Which I will review separately. John

1 0

Re: [Libvirt-cim] [PATCH V3 07/11] RASDIndication, fix debug print crash
by John Ferlan 11 Mar '13

11 Mar '13

Since I don't have the original email to reply-to, here is a link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00027.html ACK Although the 3 of these could be "one patch" realistically since they fix the same problem. I know there are a lot of calls to CU_DEBUG, perhaps it'd be best to go through all of them to make sure there are no other similar issues lurking. John

1 0

Re: [Libvirt-cim] [PATCH V3 06/11] DevicePool, reimplement get_diskpool_config with libvirt
by John Ferlan 11 Mar '13

11 Mar '13

Since I don't have the original email to reply-to, here is a link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00023.html NACK this since it's replaced by: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00039.html I will review that one separately. John

1 0