Re: [Libvirt-cim] [PATCH V4] VSSD, fix a error report issue in VSSD enum
by John Ferlan
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
11 years, 6 months
Re: [Libvirt-cim] [PATCH V3 09/11] CSI Discard libvirt event by default
by John Ferlan
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
11 years, 6 months
Re: [Libvirt-cim] [PATCH V3 08/11] device parsing, add debug print
by John Ferlan
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
11 years, 6 months
Re: [Libvirt-cim] [PATCH V3 01/11] remove script for bridge network
by John Ferlan
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;
11 years, 6 months