[PATCH 00/10] Adjustments based on recent review
As requested here is a series of patches to address a couple of issues I discovered during my initial foray into using libvirt-cim as well as from my code review. These are all *based on* the code I reviewed. Feel free to pick, choose, and apply. I have run them on my f18 system and things seem to work, but it's possible I've made a configuration error. I also will have a series of cimtest fixes, but I need to tidy them up a bit before sending. I did figure out that rather than uninstalling and reinstalling the package, I can use make preuninstall, make preinstall, restart the CIMOM, make install, and make postinsall which seems to replace the mofs "mostly correctly". There is an error in the postinstall phase which I haven't had the cycles to chase yet. John Ferlan (10): Remove empty newline at bottom Makefile.am: Remove the $(top_srcdir) from subst command xmlgen: Only support script on bridge for xen domains CSI: Fix bug found during 'make distcheck' Makefile.am: Use the top_srcdir rather than direct path in subst libxkutil: Use virConnectListAllDomains() to fetch domains libvirt-cim.spec: Use systemctl for tog-pegasus restart libxkutil: Adjust get_dominfo() logic DevicePool: Use the virConnectListAll interfaces register: Adjust the chatter output Makefile.am | 18 +++--- libvirt-cim.spec.in | 10 ++- libxkutil/cs_util_instance.c | 23 +++++++ libxkutil/device_parsing.c | 10 +-- libxkutil/xmlgen.c | 25 +++++--- provider-register.sh | 9 +-- schema/SwitchService.registration | 1 - src/Virt_ComputerSystemIndication.c | 4 ++ src/Virt_DevicePool.c | 121 +++++++++++++++++++++++++++++++++++- 9 files changed, 190 insertions(+), 31 deletions(-) -- 1.8.1.4
--- schema/SwitchService.registration | 1 - 1 file changed, 1 deletion(-) diff --git a/schema/SwitchService.registration b/schema/SwitchService.registration index b8e4f23..82a5c04 100644 --- a/schema/SwitchService.registration +++ b/schema/SwitchService.registration @@ -3,4 +3,3 @@ Xen_SwitchService root/virt Virt_SwitchService Virt_SwitchService instance KVM_SwitchService root/virt Virt_SwitchService Virt_SwitchService instance LXC_SwitchService root/virt Virt_SwitchService Virt_SwitchService instance - -- 1.8.1.4
于 2013-3-15 6:55, John Ferlan 写道:
--- schema/SwitchService.registration | 1 - 1 file changed, 1 deletion(-)
diff --git a/schema/SwitchService.registration b/schema/SwitchService.registration index b8e4f23..82a5c04 100644 --- a/schema/SwitchService.registration +++ b/schema/SwitchService.registration @@ -3,4 +3,3 @@ Xen_SwitchService root/virt Virt_SwitchService Virt_SwitchService instance KVM_SwitchService root/virt Virt_SwitchService Virt_SwitchService instance LXC_SwitchService root/virt Virt_SwitchService Virt_SwitchService instance -
+1 -- Best Regards Wenchao Xia
During the postinstall and preuninstall phases various variables are modified to build up the list of mofs to be installed. The generated output had ".//usr/local/share/libvirt-cim/*" which caused issues finding files. This is a followup to commit '22022870' which changed the paths using to schema for each of the variables. --- Makefile.am | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Makefile.am b/Makefile.am index 0fdd8bb..b7ee230 100644 --- a/Makefile.am +++ b/Makefile.am @@ -206,21 +206,21 @@ preinstall: # Un/Register the providers and class definitions from/to the current CIMOM. # @CIMSERVER@ is set by the configure script postinstall: - sh provider-register.sh -v -t @CIMSERVER@ -n @CIM_VIRT_NS@ -r $(subst schema,$(pkgdatadir), $(REGS)) -m $(subst schema,$(pkgdatadir), $(MOFS)) - sh provider-register.sh -v -t @CIMSERVER@ -n root/interop -r $(subst schema,$(pkgdatadir), $(INTEROP_REGS)) -m $(subst schema,$(pkgdatadir), $(INTEROP_MOFS)) - sh provider-register.sh -v -t @CIMSERVER@ -n root/cimv2 -r $(subst schema,$(pkgdatadir), $(CIMV2_REGS)) -m $(subst schema,$(pkgdatadir), $(CIMV2_MOFS)) + sh provider-register.sh -v -t @CIMSERVER@ -n @CIM_VIRT_NS@ -r $(subst $(top_srcdir)/schema,$(pkgdatadir), $(REGS)) -m $(subst $(top_srcdir)/schema,$(pkgdatadir), $(MOFS)) + sh provider-register.sh -v -t @CIMSERVER@ -n root/interop -r $(subst $(top_srcdir)/schema,$(pkgdatadir), $(INTEROP_REGS)) -m $(subst $(top_srcdir)/schema,$(pkgdatadir), $(INTEROP_MOFS)) + sh provider-register.sh -v -t @CIMSERVER@ -n root/cimv2 -r $(subst $(top_srcdir)/schema,$(pkgdatadir), $(CIMV2_REGS)) -m $(subst $(top_srcdir)/schema,$(pkgdatadir), $(CIMV2_MOFS)) if [[ @CIMSERVER@ = pegasus ]]; then \ - sh provider-register.sh -v -t @CIMSERVER@ -n root/PG_InterOp -r $(subst schema,$(pkgdatadir), $(PGINTEROP_REGS)) -m $(subst schema,$(pkgdatadir), $(PGINTEROP_MOFS)); \ + sh provider-register.sh -v -t @CIMSERVER@ -n root/PG_InterOp -r $(subst $(top_srcdir)/schema,$(pkgdatadir), $(PGINTEROP_REGS)) -m $(subst $(top_srcdir)/schema,$(pkgdatadir), $(PGINTEROP_MOFS)); \ fi virsh -v | grep -q '^0.3' && cp examples/diskpool.conf $(DISK_POOL_CONFIG) || true mkdir -p $(INFO_STORE) preuninstall: - sh provider-register.sh -v -d -t @CIMSERVER@ -n @CIM_VIRT_NS@ -r $(subst schema,$(pkgdatadir), $(REGS)) -m $(subst schema,$(pkgdatadir), $(MOFS)) - sh provider-register.sh -v -d -t @CIMSERVER@ -n root/interop -r $(subst schema,$(pkgdatadir), $(INTEROP_REGS)) -m $(subst schema,$(pkgdatadir), $(INTEROP_MOFS)) - sh provider-register.sh -v -d -t @CIMSERVER@ -n root/cimv2 -r $(subst schema,$(pkgdatadir), $(CIMV2_REGS)) -m $(subst schema,$(pkgdatadir), $(CIMV2_MOFS)) + sh provider-register.sh -v -d -t @CIMSERVER@ -n @CIM_VIRT_NS@ -r $(subst $(top_srcdir)/schema,$(pkgdatadir), $(REGS)) -m $(subst $(top_srcdir)/schema,$(pkgdatadir), $(MOFS)) + sh provider-register.sh -v -d -t @CIMSERVER@ -n root/interop -r $(subst $(top_srcdir)/schema,$(pkgdatadir), $(INTEROP_REGS)) -m $(subst $(top_srcdir)/schema,$(pkgdatadir), $(INTEROP_MOFS)) + sh provider-register.sh -v -d -t @CIMSERVER@ -n root/cimv2 -r $(subst $(top_srcdir)/schema,$(pkgdatadir), $(CIMV2_REGS)) -m $(subst $(top_srcdir)/schema,$(pkgdatadir), $(CIMV2_MOFS)) if [[ @CIMSERVER@ = pegasus ]]; then \ - sh provider-register.sh -v -d -t @CIMSERVER@ -n root/PG_InterOp -r $(subst schema,$(pkgdatadir), $(PGINTEROP_REGS)) -m $(subst schema,$(pkgdatadir), $(PGINTEROP_MOFS)); \ + sh provider-register.sh -v -d -t @CIMSERVER@ -n root/PG_InterOp -r $(subst $(top_srcdir)/schema,$(pkgdatadir), $(PGINTEROP_REGS)) -m $(subst $(top_srcdir)/schema,$(pkgdatadir), $(PGINTEROP_MOFS)); \ fi rpm: clean -- 1.8.1.4
A change was made in 0.9.10 to disallow a script on a bridge device for qemu guests, see 'libvirt' commit id '1734cdb99'. --- libxkutil/xmlgen.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 2dcd0d2..099fdd2 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -270,16 +270,21 @@ static const char *set_net_source(xmlNodePtr nic, } -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 *dominfo) } #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; -- 1.8.1.4
于 2013-3-15 6:55, John Ferlan 写道:
A change was made in 0.9.10 to disallow a script on a bridge device for qemu guests, see 'libvirt' commit id '1734cdb99'. --- libxkutil/xmlgen.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 2dcd0d2..099fdd2 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -270,16 +270,21 @@ static const char *set_net_source(xmlNodePtr nic, }
-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 *dominfo) } #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;
+1 -- Best Regards Wenchao Xia
The 'ret' variable was set, but never checked. Caller never checks returned status anyway. --- src/Virt_ComputerSystemIndication.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 6b5cdd4..4c087c4 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -726,6 +726,10 @@ static bool wait_for_event(int wait_time) ret = pthread_cond_timedwait(&lifecycle_cond, &lifecycle_mutex, &timeout); + if (ret != 0) { + CU_DEBUG("timed wait failed with ret = %d", ret); + return false; + } return true; } -- 1.8.1.4
于 2013-3-15 6:55, John Ferlan 写道:
The 'ret' variable was set, but never checked. Caller never checks returned status anyway. --- src/Virt_ComputerSystemIndication.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 6b5cdd4..4c087c4 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -726,6 +726,10 @@ static bool wait_for_event(int wait_time) ret = pthread_cond_timedwait(&lifecycle_cond, &lifecycle_mutex, &timeout); + if (ret != 0) { + CU_DEBUG("timed wait failed with ret = %d", ret); + return false; + }
return true; }
+1 -- Best Regards Wenchao Xia
于 2013-3-15 6:55, John Ferlan 写道:
The 'ret' variable was set, but never checked. Caller never checks returned status anyway. --- src/Virt_ComputerSystemIndication.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 6b5cdd4..4c087c4 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -726,6 +726,10 @@ static bool wait_for_event(int wait_time) ret = pthread_cond_timedwait(&lifecycle_cond, &lifecycle_mutex, &timeout); + if (ret != 0) { + CU_DEBUG("timed wait failed with ret = %d", ret); + return false; + }
return true; }
+1 -- Best Regards Wenchao Xia
This is a followup to '9f5e204f' which directly added the './' to the schema path. Use of top_srcdir instead just symbolizes things. --- Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index b7ee230..63ed3c7 100644 --- a/Makefile.am +++ b/Makefile.am @@ -189,7 +189,7 @@ install-data-local: $(install_sh_DATA) -t "$(DESTDIR)$(pkgdatadir)" $(INTEROP_MOFS) $(install_sh_DATA) -t "$(DESTDIR)$(pkgdatadir)" $(INTEROP_REGS) if [[ @CIMSERVER@ != pegasus ]]; then \ - sed -i '/^# --/,/^# --!/d' $(subst ./schema,$(DESTDIR)$(pkgdatadir), $(PGINTEROP_REGS)); \ + sed -i '/^# --/,/^# --!/d' $(subst $(top_srcdir)/schema,$(DESTDIR)$(pkgdatadir), $(PGINTEROP_REGS)); \ fi uninstall-local: -- 1.8.1.4
This is an optimization over using the multistep approach to get a count, get some memory, and get the list of domains (active and defined). Followed other examples to ensure only building the code if the libvirt version is correct. The API was added in 0.9.13. --- libxkutil/cs_util_instance.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/libxkutil/cs_util_instance.c b/libxkutil/cs_util_instance.c index a383147..e34c05d 100644 --- a/libxkutil/cs_util_instance.c +++ b/libxkutil/cs_util_instance.c @@ -34,6 +34,28 @@ #include <libcmpiutil/libcmpiutil.h> int get_domain_list(virConnectPtr conn, virDomainPtr **_list) +#if LIBVIR_VERSION_NUMBER >= 9013 +{ + virDomainPtr *nameList = NULL; + int n_names; + int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE; + + n_names = virConnectListAllDomains(conn, + &nameList, + flags); + if (n_names > 0) { + *_list = nameList; + } else if (n_names == 0) { + /* Since there are no elements, no domain ptrs to free + * but still must free the nameList returned + */ + free(nameList); + } + + return n_names; +} +#else { char **names = NULL; int n_names; @@ -113,6 +135,7 @@ int get_domain_list(virConnectPtr conn, virDomainPtr **_list) return idx; } +#endif /* LIBVIR_VERSION_NUMBER >= 0913 */ void set_instance_class_name(CMPIInstance *instance, char *name) { -- 1.8.1.4
于 2013-3-15 6:55, John Ferlan 写道:
This is an optimization over using the multistep approach to get a count, get some memory, and get the list of domains (active and defined). Followed other examples to ensure only building the code if the libvirt version is correct. The API was added in 0.9.13. --- libxkutil/cs_util_instance.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/libxkutil/cs_util_instance.c b/libxkutil/cs_util_instance.c index a383147..e34c05d 100644 --- a/libxkutil/cs_util_instance.c +++ b/libxkutil/cs_util_instance.c @@ -34,6 +34,28 @@ #include <libcmpiutil/libcmpiutil.h>
int get_domain_list(virConnectPtr conn, virDomainPtr **_list) +#if LIBVIR_VERSION_NUMBER >= 9013 +{ + virDomainPtr *nameList = NULL; + int n_names; + int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE; + + n_names = virConnectListAllDomains(conn, + &nameList, + flags); + if (n_names > 0) { + *_list = nameList; + } else if (n_names == 0) { + /* Since there are no elements, no domain ptrs to free + * but still must free the nameList returned + */ + free(nameList); + } + + return n_names; +} +#else { char **names = NULL; int n_names; @@ -113,6 +135,7 @@ int get_domain_list(virConnectPtr conn, virDomainPtr **_list)
return idx; } +#endif /* LIBVIR_VERSION_NUMBER >= 0913 */
void set_instance_class_name(CMPIInstance *instance, char *name) {
Code seems good, +1. Please reduce version condition macro in function as much as possible, if there are more functions in same file, I think two mirrored functions in two condition will make code easier to read in future: #if LIBVIRT_VERSION_NUMER >= XXX void function1(void) { } void function2(void) { } #else void function1(void) { } void function2(void) { } #endif -- Best Regards Wenchao Xia
On 03/15/2013 04:20 AM, Wenchao Xia wrote:
于 2013-3-15 6:55, John Ferlan 写道:
Code seems good, +1. Please reduce version condition macro in function as much as possible, if there are more functions in same file, I think two mirrored functions in two condition will make code easier to read in future:
Readability +1, maintainability -1 - that's the reason I went with interlaced. I also looked at a few other examples in the code that have used the LIBVIR_VERSION_NUMBER mechanism and they interlace the code. For me it's good coding practice. Another option would be to modify the libvirt.spec.in file and change the following from: Requires: libvirt >= 0.9.0 to Requires: libvirt >= 0.10.2 And the aclocal.m4 file to change the following from: _pkg_min_version=m4_default([$1], [0.9.0]) to _pkg_min_version=m4_default([$1], [0.10.2]) Then just remove all the "#else" conditions... Since we're at 0.9.0 already, I'm not sure why the code has the lesser checks. However, I don't have the "history" to say for 100% the right path to take on this... John
For Fedora 17 and RHEL7 use systemd, so change the pegasus startup to use that and make a dependency upon it. --- libvirt-cim.spec.in | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libvirt-cim.spec.in b/libvirt-cim.spec.in index 7652dee..0679d7f 100644 --- a/libvirt-cim.spec.in +++ b/libvirt-cim.spec.in @@ -27,6 +27,9 @@ BuildRequires: libconfig-devel BuildRequires: libxml2-devel BuildRequires: libcmpiutil-devel +%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7 +BuildRequires: systemd-units +%endif BuildConflicts: sblim-cmpi-devel %description @@ -85,7 +88,12 @@ rm -fr $RPM_BUILD_ROOT %{_datadir}/%{name}/install_base_schema.sh %{_datadir}/%{name} -/etc/init.d/tog-pegasus condrestart +# Fedora 17 / RHEL-7 are first where we use systemd. +%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7 + systemctl restart tog-pegasus +%else + /etc/init.d/tog-pegasus condrestart +%endif %{_datadir}/%{name}/provider-register.sh -t pegasus \ -n @CIM_VIRT_NS@ \ -- 1.8.1.4
于 2013-3-15 6:55, John Ferlan 写道:
For Fedora 17 and RHEL7 use systemd, so change the pegasus startup to use that and make a dependency upon it. --- libvirt-cim.spec.in | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/libvirt-cim.spec.in b/libvirt-cim.spec.in index 7652dee..0679d7f 100644 --- a/libvirt-cim.spec.in +++ b/libvirt-cim.spec.in @@ -27,6 +27,9 @@ BuildRequires: libconfig-devel
BuildRequires: libxml2-devel BuildRequires: libcmpiutil-devel +%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7 +BuildRequires: systemd-units +%endif BuildConflicts: sblim-cmpi-devel
%description @@ -85,7 +88,12 @@ rm -fr $RPM_BUILD_ROOT
%{_datadir}/%{name}/install_base_schema.sh %{_datadir}/%{name}
-/etc/init.d/tog-pegasus condrestart +# Fedora 17 / RHEL-7 are first where we use systemd. +%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7 + systemctl restart tog-pegasus +%else + /etc/init.d/tog-pegasus condrestart +%endif
%{_datadir}/%{name}/provider-register.sh -t pegasus \ -n @CIM_VIRT_NS@ \
+1 -- Best Regards Wenchao Xia
Need to really handle the error on the get_dominfo_from_xml() call Need to be sure to free(xml) before returning --- libxkutil/device_parsing.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 2841662..264d4cc 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -1246,7 +1246,7 @@ int get_dominfo_from_xml(const char *xml, struct domain **dominfo) int get_dominfo(virDomainPtr dom, struct domain **dominfo) { char *xml; - int ret; + int ret = 0; int start; xml = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); @@ -1256,17 +1256,19 @@ int get_dominfo(virDomainPtr dom, struct domain **dominfo) return 0; } - ret = get_dominfo_from_xml(xml, dominfo); - if (ret != 1) { + if (get_dominfo_from_xml(xml, dominfo) == 0) { CU_DEBUG("Failed to translate xml into struct domain"); + goto out; } if (virDomainGetAutostart(dom, &start) != 0) { CU_DEBUG("Failed to get dom autostart with libvirt API."); - return 0; + goto out; } (*dominfo)->autostrt = start; + ret = 1; + out: free(xml); return ret; -- 1.8.1.4
于 2013-3-15 6:56, John Ferlan 写道:
Need to really handle the error on the get_dominfo_from_xml() call
Need to be sure to free(xml) before returning --- libxkutil/device_parsing.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 2841662..264d4cc 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -1246,7 +1246,7 @@ int get_dominfo_from_xml(const char *xml, struct domain **dominfo) int get_dominfo(virDomainPtr dom, struct domain **dominfo) { char *xml; - int ret; + int ret = 0; int start; xml = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); @@ -1256,17 +1256,19 @@ int get_dominfo(virDomainPtr dom, struct domain **dominfo) return 0; }
- ret = get_dominfo_from_xml(xml, dominfo); - if (ret != 1) { + if (get_dominfo_from_xml(xml, dominfo) == 0) { CU_DEBUG("Failed to translate xml into struct domain"); + goto out; } if (virDomainGetAutostart(dom, &start) != 0) { CU_DEBUG("Failed to get dom autostart with libvirt API."); - return 0; + goto out; }
(*dominfo)->autostrt = start; + ret = 1;
+ out: free(xml);
return ret;
Fixed a leak, +1 -- Best Regards Wenchao Xia
Rather than the somewhat unreliable get a count and get a list of active names, use the newer virConnectListAll* interfaces in order to retrieve both a count and list in one call. --- src/Virt_DevicePool.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 2 deletions(-) diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index bf3dd3b..bffa0cf 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -146,13 +146,24 @@ static int get_diskpool_config(virConnectPtr conn, struct tmp_disk_pool **_pools, int *_count) { +#if LIBVIR_VERSION_NUMBER >= 100002 + int i, realcount = 0, count = 0; + virStoragePoolPtr *nameList = NULL; + int flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE; +#else int count = 0, realcount = 0; int i; char ** names = NULL; +#endif struct tmp_disk_pool *pools = NULL; int ret = 0; bool bret; +#if LIBVIR_VERSION_NUMBER >= 100002 + realcount = virConnectListAllStoragePools(conn, + &nameList, + flags); +#else count = virConnectNumOfStoragePools(conn); if (count < 0) { ret = count; @@ -169,6 +180,7 @@ static int get_diskpool_config(virConnectPtr conn, } realcount = virConnectListStoragePools(conn, names, count); +#endif if (realcount < 0) { CU_DEBUG("Failed to get storage pools, return %d.", realcount); ret = realcount; @@ -187,7 +199,13 @@ static int get_diskpool_config(virConnectPtr conn, } for (i = 0; i < realcount; i++) { - pools[i].tag = strdup(names[i]); +#if LIBVIR_VERSION_NUMBER >= 100002 + pools[i].tag = strdup(getVirStoragePoolName(nameList[i])); +#else + /* Just take names[i], since we're free()'ing later */ + pools[i].tag = names[i]; + names[i] = NULL; +#endif if (pools[i].tag == NULL) { CU_DEBUG("Failed in strdup for name '%s'.", names[i]); ret = -3; @@ -213,10 +231,18 @@ static int get_diskpool_config(virConnectPtr conn, free_diskpool(pools, realcount); free_names: +#if LIBVIR_VERSION_NUMBER >= 100002 + if (nameList != NULL) { + for (i = 0; i < realcount; i++) + virStoragePoolFree(nameList[i]); + free(nameList); + } +#else for (i = 0; i < count; i++) { free(names[i]); } free(names); +#endif out: return ret; @@ -529,6 +555,40 @@ static char *diskpool_member_of(const CMPIBroker *broker, static virNetworkPtr bridge_to_network(virConnectPtr conn, const char *bridge) { +#if LIBVIR_VERSION_NUMBER >= 100002 + int num; + virNetworkPtr **nameList = NULL; + virNetworkPtr network = NULL; + flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE; + + num = virConnectListAllNetworks(conn, + &nameList, + flags); + if (num < 0) { + CU_DEBUG("Failed to get network pools."); + return NULL; + } + + for (i = 0; i < num; i++) { + char *_netname; + char *_bridge; + + _netname = virNetworkGetName(nameList[i]); + _bridge = virNetworkGetBridgeName(network); + CU_DEBUG("Network `%s' has bridge `%s'", _netname, _bridge); + if (STREQ(bridge, _bridge)) { + network = nameList[i]; + nameList[i] = NULL; + i = num; /* Loop breaker */ + } + free(_bridge); + } + + for (i = 0; i < num; i++) { + virNetworkFree(nameList[i]); + } + free(nameList); +#else char **networks = NULL; virNetworkPtr network = NULL; int num; @@ -566,7 +626,7 @@ static virNetworkPtr bridge_to_network(virConnectPtr conn, for (i = 0; i < num; i++) free(networks[i]); free(networks); - +#endif return network; } @@ -748,6 +808,29 @@ static bool mempool_set_total(CMPIInstance *inst, virConnectPtr conn) static bool mempool_set_consumed(CMPIInstance *inst, virConnectPtr conn) { uint64_t memory = 0; +#if LIBVIR_VERSION_NUMBER >= 9013 + virDomainPtr *nameList = NULL; + int n_names, i; + int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE; + + n_names = virConnectListAllDomains(conn, + &nameList, + flags); + if (n_names < 0) { + CU_DEBUG("Failed to get a list of all domains"); + goto out; + } + + for (i = 0; i < n_names; i++) { + virDomainInfo dom_info; + if (virDomainGetInfo(nameList[i], &dom_info) == 0) + memory += dom_info.memory; + virDomainFree(nameList[i]); + } + free(nameList); + + out: +#else int *domain_ids = NULL; int count, i = 0; @@ -781,6 +864,7 @@ static bool mempool_set_consumed(CMPIInstance *inst, virConnectPtr conn) out: free(domain_ids); +#endif /* LIBVIR_VERSION_NUMBER >= 0913 */ CMSetProperty(inst, "Reserved", (CMPIValue *)&memory, CMPI_uint64); @@ -1034,6 +1118,10 @@ static CMPIStatus netpool_instance(virConnectPtr conn, char **netnames = NULL; int i; int nets = 0; +#if LIBVIR_VERSION_NUMBER >= 100002 + virNetworkPtr **nameList = NULL; + flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE; +#endif if (id != NULL) { return _netpool_for_network(list, @@ -1044,7 +1132,16 @@ static CMPIStatus netpool_instance(virConnectPtr conn, broker); } +#if LIBVIR_VERSION_NUMBER >= 100002 + nets = virConnectListAllNetworks(conn, + &nameList, + flags); + /* Avoids the need to realloc since we have a true number */ + if (nets >= 0) + nets++; +#else nets = virConnectNumOfNetworks(conn); +#endif if (nets < 0) { virt_set_status(broker, &s, CMPI_RC_ERR_FAILED, @@ -1062,6 +1159,18 @@ static CMPIStatus netpool_instance(virConnectPtr conn, goto out; } +#if LIBVIR_VERSION_NUMBER >= 100002 + for (i = 0; i < nets - 1; i++) { + netnames[i] = strdup(virNetworkGetName(nameList[i])); + if (netnames[i] == NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed to strdup memory for %i net names", + nets); + goto out; + } + } +#else nets = virConnectListNetworks(conn, netnames, nets); nets++; @@ -1072,6 +1181,7 @@ static CMPIStatus netpool_instance(virConnectPtr conn, "Failed to allocate memory for %i net names", nets); goto out; } +#endif netnames[nets - 1] = strdup("0"); @@ -1085,6 +1195,13 @@ static CMPIStatus netpool_instance(virConnectPtr conn, } out: +#if LIBVIR_VERSION_NUMBER >= 100002 + if (nameList != NULL) { + for (i = 0; i < nets - 1; i++) + virNetworkFree(nameList[i]); + free(nameList); + } +#endif if (netnames != NULL) { for (i = 0; i < nets; i++) free(netnames[i]); -- 1.8.1.4
于 2013-3-15 6:56, John Ferlan 写道:
Rather than the somewhat unreliable get a count and get a list of active names, use the newer virConnectListAll* interfaces in order to retrieve both a count and list in one call. --- src/Virt_DevicePool.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 2 deletions(-)
diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index bf3dd3b..bffa0cf 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -146,13 +146,24 @@ static int get_diskpool_config(virConnectPtr conn, struct tmp_disk_pool **_pools, int *_count) { +#if LIBVIR_VERSION_NUMBER >= 100002 + int i, realcount = 0, count = 0; + virStoragePoolPtr *nameList = NULL; + int flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE; +#else int count = 0, realcount = 0; int i; char ** names = NULL; +#endif struct tmp_disk_pool *pools = NULL; int ret = 0; bool bret;
+#if LIBVIR_VERSION_NUMBER >= 100002 + realcount = virConnectListAllStoragePools(conn, + &nameList, + flags); +#else count = virConnectNumOfStoragePools(conn); if (count < 0) { ret = count; @@ -169,6 +180,7 @@ static int get_diskpool_config(virConnectPtr conn, }
realcount = virConnectListStoragePools(conn, names, count); +#endif if (realcount < 0) { CU_DEBUG("Failed to get storage pools, return %d.", realcount); ret = realcount; @@ -187,7 +199,13 @@ static int get_diskpool_config(virConnectPtr conn, }
for (i = 0; i < realcount; i++) { - pools[i].tag = strdup(names[i]); +#if LIBVIR_VERSION_NUMBER >= 100002 + pools[i].tag = strdup(getVirStoragePoolName(nameList[i])); +#else + /* Just take names[i], since we're free()'ing later */ + pools[i].tag = names[i]; + names[i] = NULL; +#endif if (pools[i].tag == NULL) { CU_DEBUG("Failed in strdup for name '%s'.", names[i]); ret = -3; @@ -213,10 +231,18 @@ static int get_diskpool_config(virConnectPtr conn, free_diskpool(pools, realcount);
free_names: +#if LIBVIR_VERSION_NUMBER >= 100002 + if (nameList != NULL) { + for (i = 0; i < realcount; i++) + virStoragePoolFree(nameList[i]); + free(nameList); + } +#else for (i = 0; i < count; i++) { free(names[i]); } free(names); +#endif
out: return ret; @@ -529,6 +555,40 @@ static char *diskpool_member_of(const CMPIBroker *broker, static virNetworkPtr bridge_to_network(virConnectPtr conn, const char *bridge) { +#if LIBVIR_VERSION_NUMBER >= 100002 + int num; + virNetworkPtr **nameList = NULL; + virNetworkPtr network = NULL; + flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE; + + num = virConnectListAllNetworks(conn, + &nameList, + flags); + if (num < 0) { + CU_DEBUG("Failed to get network pools."); + return NULL; + } + + for (i = 0; i < num; i++) { + char *_netname; + char *_bridge; + + _netname = virNetworkGetName(nameList[i]); + _bridge = virNetworkGetBridgeName(network); + CU_DEBUG("Network `%s' has bridge `%s'", _netname, _bridge); + if (STREQ(bridge, _bridge)) { + network = nameList[i]; + nameList[i] = NULL; + i = num; /* Loop breaker */ + } + free(_bridge); + } + + for (i = 0; i < num; i++) { + virNetworkFree(nameList[i]); + } + free(nameList); +#else char **networks = NULL; virNetworkPtr network = NULL; int num; @@ -566,7 +626,7 @@ static virNetworkPtr bridge_to_network(virConnectPtr conn, for (i = 0; i < num; i++) free(networks[i]); free(networks); - +#endif return network; }
@@ -748,6 +808,29 @@ static bool mempool_set_total(CMPIInstance *inst, virConnectPtr conn) static bool mempool_set_consumed(CMPIInstance *inst, virConnectPtr conn) { uint64_t memory = 0; +#if LIBVIR_VERSION_NUMBER >= 9013 + virDomainPtr *nameList = NULL; + int n_names, i; + int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE; + + n_names = virConnectListAllDomains(conn, + &nameList, + flags); + if (n_names < 0) { + CU_DEBUG("Failed to get a list of all domains"); + goto out; + } + + for (i = 0; i < n_names; i++) { + virDomainInfo dom_info; + if (virDomainGetInfo(nameList[i], &dom_info) == 0) + memory += dom_info.memory; + virDomainFree(nameList[i]); + } + free(nameList); + + out: +#else int *domain_ids = NULL; int count, i = 0;
@@ -781,6 +864,7 @@ static bool mempool_set_consumed(CMPIInstance *inst, virConnectPtr conn)
out: free(domain_ids); +#endif /* LIBVIR_VERSION_NUMBER >= 0913 */
CMSetProperty(inst, "Reserved", (CMPIValue *)&memory, CMPI_uint64); @@ -1034,6 +1118,10 @@ static CMPIStatus netpool_instance(virConnectPtr conn, char **netnames = NULL; int i; int nets = 0; +#if LIBVIR_VERSION_NUMBER >= 100002 + virNetworkPtr **nameList = NULL; + flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE; +#endif
if (id != NULL) { return _netpool_for_network(list, @@ -1044,7 +1132,16 @@ static CMPIStatus netpool_instance(virConnectPtr conn, broker); }
+#if LIBVIR_VERSION_NUMBER >= 100002 + nets = virConnectListAllNetworks(conn, + &nameList, + flags); + /* Avoids the need to realloc since we have a true number */ + if (nets >= 0) + nets++; +#else nets = virConnectNumOfNetworks(conn); +#endif if (nets < 0) { virt_set_status(broker, &s, CMPI_RC_ERR_FAILED, @@ -1062,6 +1159,18 @@ static CMPIStatus netpool_instance(virConnectPtr conn, goto out; }
+#if LIBVIR_VERSION_NUMBER >= 100002 + for (i = 0; i < nets - 1; i++) { + netnames[i] = strdup(virNetworkGetName(nameList[i])); + if (netnames[i] == NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed to strdup memory for %i net names", + nets); + goto out; + } + } +#else nets = virConnectListNetworks(conn, netnames, nets);
nets++; @@ -1072,6 +1181,7 @@ static CMPIStatus netpool_instance(virConnectPtr conn, "Failed to allocate memory for %i net names", nets); goto out; } +#endif
netnames[nets - 1] = strdup("0");
@@ -1085,6 +1195,13 @@ static CMPIStatus netpool_instance(virConnectPtr conn, }
out: +#if LIBVIR_VERSION_NUMBER >= 100002 + if (nameList != NULL) { + for (i = 0; i < nets - 1; i++) + virNetworkFree(nameList[i]); + free(nameList); + } +#endif if (netnames != NULL) { for (i = 0; i < nets; i++) free(netnames[i]);
-1, too much inline macros, which will make the file very hard to read later, please use mirrored function pairs. -- Best Regards Wenchao Xia
Caused error during make postinstall. Rather than print the $PROVIDERMODULES it attempted to execute the second one in the list. Added extra output just prior to CIMMOF commands to show which namespace is being modified from which directory. There appears to be some sort of issue registering some mofs as the following message is generated numerous times: Warning: the instance already exists. In this implementation, that means it cannot be changed. --- provider-register.sh | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/provider-register.sh b/provider-register.sh index 0616a14..b907df1 100755 --- a/provider-register.sh +++ b/provider-register.sh @@ -90,10 +90,10 @@ pegasus_transform() chatter "cimserver version is " $version if compare_version "$version" "2.11.0" then - chatter "Processing provider modules (w/o ModuleGroupName):" \ - $PROVIDERMODULES + chatter "Processing provider modules (w/o ModuleGroupName):" for pm in $PROVIDERMODULES do + chatter "...processing " $pm cat >> $OUTFILE <<EOFPM instance of PG_ProviderModule { @@ -109,10 +109,10 @@ EOFPM done else - chatter "Processing provider modules (w/ ModuleGroupName):" \ - $PROVIDERMODULES + chatter "Processing provider modules (w/ ModuleGroupName):" for pm in $PROVIDERMODULES do + chatter "...processing " $pm cat >> $OUTFILE <<EOFPM instance of PG_ProviderModule { @@ -272,6 +272,7 @@ pegasus_install() if pegasus_transform $_REGFILENAME $namespace $myregs then chatter Registering providers with $state cimserver '('$version')' + chatter Installing mofs into namespace $namespace from path $mofpath $CIMMOF -uc -I $mofpath -n $namespace $mymofs && $CIMMOF -uc -n root/PG_Interop $_REGFILENAME else -- 1.8.1.4
于 2013-3-15 6:56, John Ferlan 写道:
Caused error during make postinstall. Rather than print the $PROVIDERMODULES it attempted to execute the second one in the list.
Added extra output just prior to CIMMOF commands to show which namespace is being modified from which directory. There appears to be some sort of issue registering some mofs as the following message is generated numerous times:
Warning: the instance already exists. In this implementation, that means it cannot be changed. --- provider-register.sh | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/provider-register.sh b/provider-register.sh index 0616a14..b907df1 100755 --- a/provider-register.sh +++ b/provider-register.sh @@ -90,10 +90,10 @@ pegasus_transform() chatter "cimserver version is " $version if compare_version "$version" "2.11.0" then - chatter "Processing provider modules (w/o ModuleGroupName):" \ - $PROVIDERMODULES + chatter "Processing provider modules (w/o ModuleGroupName):" for pm in $PROVIDERMODULES do + chatter "...processing " $pm cat >> $OUTFILE <<EOFPM instance of PG_ProviderModule { @@ -109,10 +109,10 @@ EOFPM done
else - chatter "Processing provider modules (w/ ModuleGroupName):" \ - $PROVIDERMODULES + chatter "Processing provider modules (w/ ModuleGroupName):" for pm in $PROVIDERMODULES do + chatter "...processing " $pm cat >> $OUTFILE <<EOFPM instance of PG_ProviderModule { @@ -272,6 +272,7 @@ pegasus_install() if pegasus_transform $_REGFILENAME $namespace $myregs then chatter Registering providers with $state cimserver '('$version')' + chatter Installing mofs into namespace $namespace from path $mofpath $CIMMOF -uc -I $mofpath -n $namespace $mymofs && $CIMMOF -uc -n root/PG_Interop $_REGFILENAME else
+1 -- Best Regards Wenchao Xia
Thanks for your patches, most seems good, some I can't confirm in a short time. Since I am not the maintainer, I'll include those got +1 in my serial with Signed-off-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> And then let Danial V decide whether to pull.
As requested here is a series of patches to address a couple of issues I discovered during my initial foray into using libvirt-cim as well as from my code review. These are all *based on* the code I reviewed.
Feel free to pick, choose, and apply. I have run them on my f18 system and things seem to work, but it's possible I've made a configuration error. I also will have a series of cimtest fixes, but I need to tidy them up a bit before sending.
I did figure out that rather than uninstalling and reinstalling the package, I can use make preuninstall, make preinstall, restart the CIMOM, make install, and make postinsall which seems to replace the mofs "mostly correctly". There is an error in the postinstall phase which I haven't had the cycles to chase yet.
John Ferlan (10): Remove empty newline at bottom Makefile.am: Remove the $(top_srcdir) from subst command xmlgen: Only support script on bridge for xen domains CSI: Fix bug found during 'make distcheck' Makefile.am: Use the top_srcdir rather than direct path in subst libxkutil: Use virConnectListAllDomains() to fetch domains libvirt-cim.spec: Use systemctl for tog-pegasus restart libxkutil: Adjust get_dominfo() logic DevicePool: Use the virConnectListAll interfaces register: Adjust the chatter output
Makefile.am | 18 +++--- libvirt-cim.spec.in | 10 ++- libxkutil/cs_util_instance.c | 23 +++++++ libxkutil/device_parsing.c | 10 +-- libxkutil/xmlgen.c | 25 +++++--- provider-register.sh | 9 +-- schema/SwitchService.registration | 1 - src/Virt_ComputerSystemIndication.c | 4 ++ src/Virt_DevicePool.c | 121 +++++++++++++++++++++++++++++++++++- 9 files changed, 190 insertions(+), 31 deletions(-)
-- Best Regards Wenchao Xia
participants (2)
-
John Ferlan -
Wenchao Xia