于 2013-8-19 11:31, Wenchao Xia 写道:

于 2013-8-15 22:48, Viktor Mihajlovski 写道:

...

For architectures like s390 the machine type is relevant for the proper guest construction. We add the necessary properties to the schema and the C structures and the necessary code for CIM-to-libvirt mapping.

While doing this I noticed that the union fields in os_info were set by means of XML parsing which doesn't take into account that certain fields are depending on the virtualization type. I think this is a issue. Could u split this patch into two: 1 consider virt type for os_info, bugfix. 2 add xml-domain-VSSD mapping for properties machine and arch.

Thus will make commit history clear and easier to review.

...

This could lead both to memory overwrites and memory leaks. Fixed by using temporary variables and type-based setting of fields

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 85 ++++++++++++++++++++++------- libxkutil/device_parsing.h | 2 + libxkutil/xmlgen.c | 6 ++ schema/VSSD.mof | 6 ++ src/Virt_VSSD.c | 9 +++ src/Virt_VirtualSystemManagementService.c | 14 +++++ 6 files changed, 101 insertions(+), 21 deletions(-)

diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index ffdf682..df7a87a 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -1077,23 +1077,41 @@ int parse_fq_devid(const char *devid, char **host, char **device) return 1; }

+static void cleanup_bootlist(char **blist, unsigned blist_ct) +{ + while (blist_ct > 0) { + free(blist[--blist_ct]); + } + free(blist); +} + static int parse_os(struct domain *dominfo, xmlNode *os) { xmlNode *child; char **blist = NULL; unsigned bl_size = 0; + char *arch = NULL; + char *machine = NULL; + char *kernel = NULL; + char *initrd = NULL; + char *cmdline = NULL; + char *loader = NULL; + char *boot = NULL; + char *init = NULL;

for (child = os->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "type")) + if (XSTREQ(child->name, "type")) { STRPROP(dominfo, os_info.pv.type, child); - else if (XSTREQ(child->name, "kernel")) - STRPROP(dominfo, os_info.pv.kernel, child); + arch = get_attr_value(child, "arch"); + machine = get_attr_value(child, "machine"); + } else if (XSTREQ(child->name, "kernel")) + kernel = get_node_content(child); else if (XSTREQ(child->name, "initrd")) - STRPROP(dominfo, os_info.pv.initrd, child); + initrd = get_node_content(child); else if (XSTREQ(child->name, "cmdline")) - STRPROP(dominfo, os_info.pv.cmdline, child); + cmdline = get_node_content(child); else if (XSTREQ(child->name, "loader")) - STRPROP(dominfo, os_info.fv.loader, child); + loader = get_node_content(child); else if (XSTREQ(child->name, "boot")) { char **tmp_list = NULL;

@@ -1111,7 +1129,7 @@ static int parse_os(struct domain *dominfo, xmlNode *os) blist[bl_size] = get_attr_value(child, "dev"); bl_size++; } else if (XSTREQ(child->name, "init")) - STRPROP(dominfo, os_info.lxc.init, child); + init = get_node_content(child); }

if ((STREQC(dominfo->os_info.fv.type, "hvm")) && @@ -1128,17 +1146,45 @@ static int parse_os(struct domain *dominfo, xmlNode *os) else dominfo->type = -1;

- if (STREQC(dominfo->os_info.fv.type, "hvm")) { + switch (dominfo->type) { + case DOMAIN_XENFV: + case DOMAIN_KVM: + case DOMAIN_QEMU: + dominfo->os_info.fv.loader = loader; + dominfo->os_info.fv.arch = arch; + dominfo->os_info.fv.machine = machine; "machine = NULL;" is missing? Otherwise dominfo->os_info.fv.machine point to a value which is freed later.

Sorry I missed that it is in the following lines, so this is not a problem.

...

dominfo->os_info.fv.bootlist_ct = bl_size; dominfo->os_info.fv.bootlist = blist; - } else { - int i; - - for (i = 0; i < bl_size; i++) - free(blist[i]); - free(blist); + loader = NULL; + arch = NULL; + machine = NULL; + blist = NULL; + bl_size = 0; + break; + case DOMAIN_XENPV: + dominfo->os_info.pv.kernel = kernel; + dominfo->os_info.pv.initrd = initrd; + dominfo->os_info.pv.cmdline = cmdline; + kernel = NULL; + initrd = NULL; + cmdline = NULL; + break; + case DOMAIN_LXC: + dominfo->os_info.lxc.init = init; + init = NULL; + break; + default: + break; }

+ free(arch); + free(machine); + free(kernel); + free(initrd); + free(cmdline); + free(boot); + free(init); + cleanup_bootlist(blist, bl_size); return 1; }

@@ -1334,15 +1380,12 @@ void cleanup_dominfo(struct domain **dominfo) free(dom->os_info.pv.cmdline); } else if ((dom->type == DOMAIN_XENFV) || (dom->type == DOMAIN_KVM) || (dom->type == DOMAIN_QEMU)) { - int i; - free(dom->os_info.fv.type); free(dom->os_info.fv.loader); - - for (i = 0; i < dom->os_info.fv.bootlist_ct; i++) { - free(dom->os_info.fv.bootlist[i]); - } - free(dom->os_info.fv.bootlist); + free(dom->os_info.fv.arch); + free(dom->os_info.fv.machine); + cleanup_bootlist(dom->os_info.fv.bootlist, + dom->os_info.fv.bootlist_ct); } else if (dom->type == DOMAIN_LXC) { free(dom->os_info.lxc.type); free(dom->os_info.lxc.init); diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 2b6d3d1..379d48c 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -136,6 +136,8 @@ struct pv_os_info {

struct fv_os_info { char *type; /* Should always be 'hvm' */ + char *arch; + char *machine; char *loader; unsigned bootlist_ct; char **bootlist; diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 4287d42..f19830f 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -802,6 +802,12 @@ static char *_kvm_os_xml(xmlNodePtr root, struct domain *domain) if (tmp == NULL) return XML_ERROR;

+ if (os->arch) + xmlNewProp(tmp, BAD_CAST "arch", BAD_CAST os->arch); + + if (os->machine) + xmlNewProp(tmp, BAD_CAST "machine", BAD_CAST os->machine); + ret = _fv_bootlist_xml(root, os); if (ret == 0) return XML_ERROR; diff --git a/schema/VSSD.mof b/schema/VSSD.mof index 0359d67..2734d8e 100644 --- a/schema/VSSD.mof +++ b/schema/VSSD.mof @@ -48,6 +48,12 @@ class KVM_VirtualSystemSettingData : Virt_VirtualSystemSettingData [Description ("The emulator the guest should use during runtime.")] string Emulator;

+ [Description ("The guest's architecture.")] + string Arch; + + [Description ("The guest's machine type")] + string Machine; + };

I haven't check DMTF docs, but wonder if there are existing DMTF file point out where this property should belong. If no, I think put it in VSSD is OK.

...

[Description ( diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c index 3363b38..67e56aa 100644 --- a/src/Virt_VSSD.c +++ b/src/Virt_VSSD.c @@ -121,6 +121,15 @@ static CMPIStatus _set_fv_prop(const CMPIBroker *broker, goto out; }

+ if (dominfo->os_info.fv.arch != NULL) + CMSetProperty(inst, "Arch", + (CMPIValue *)dominfo->os_info.fv.arch, + CMPI_chars); + + if (dominfo->os_info.fv.machine != NULL) + CMSetProperty(inst, "Machine", + (CMPIValue *)dominfo->os_info.fv.machine, + CMPI_chars); out: return s; } diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 8ced2d6..3df878f 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -543,6 +543,20 @@ static int fv_vssd_to_domain(CMPIInstance *inst, if (!fv_set_emulator(domain, val)) return 0;

+ free(domain->os_info.fv.arch); + ret = cu_get_str_prop(inst, "Arch", &val); + if (ret == CMPI_RC_OK) + domain->os_info.fv.arch = strdup(val); + else + domain->os_info.fv.arch = NULL; + + free(domain->os_info.fv.machine); + ret = cu_get_str_prop(inst, "Machine", &val); + if (ret == CMPI_RC_OK) + domain->os_info.fv.machine = strdup(val); + else + domain->os_info.fv.machine = NULL; + return 1; }

-- Best Regards Wenchao Xia

On 08/15/2013 10:48 AM, Viktor Mihajlovski wrote:

From: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>

Introspecting the libvirt capabilities and creating an internal capabilities data structure. Methods are provided for retrieving default values regarding architecture, machine and emulator for easy of use in the provider code.

Further, xml_parse_test was extendend to display hypervisor capabilities and defaults.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- libxkutil/Makefile.am | 2 + libxkutil/capability_parsing.c | 462 ++++++++++++++++++++++++++++++++++++++++ libxkutil/capability_parsing.h | 93 ++++++++ libxkutil/xml_parse_test.c | 134 +++++++++++- 4 files changed, 688 insertions(+), 3 deletions(-) create mode 100644 libxkutil/capability_parsing.c create mode 100644 libxkutil/capability_parsing.h

This seems to be missing something as my build fails (Fedora 19): CC xml_parse_test.o CCLD xml_parse_test xml_parse_test.o: In function `capinfo_for_dom': /path/to/libxkutil/xml_parse_test.c:263: undefined reference to `get_capabilities' xml_parse_test.o: In function `main': /path/to/libxkutil/xml_parse_test.c:431: undefined reference to `get_default_arch' /path/to/libvirt-cim.coverity/libxkutil/xml_parse_test.c:433: undefined reference to `get_default_machine' /path/to/libvirt-cim.coverity/libxkutil/xml_parse_test.c:435: undefined reference to `get_default_emulator' /path/to/libvirt-cim.coverity/libxkutil/xml_parse_test.c:437: undefined reference to `get_default_machine' /path/to/libvirt-cim.coverity/libxkutil/xml_parse_test.c:439: undefined reference to `get_default_emulator' /path/to/libvirt-cim.coverity/libxkutil/xml_parse_test.c:444: undefined reference to `findDomainInfo' /path/to/libvirt-cim.coverity/libxkutil/xml_parse_test.c:449: undefined reference to `findDomainInfo' collect2: error: ld returned 1 exit status make[2]: *** [xml_parse_test] Error 1 make[2]: Leaving directory `/path/to/libxkutil' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/path/to' make: *** [all] Error 2 I did not dig deeper into the code John

diff --git a/libxkutil/Makefile.am b/libxkutil/Makefile.am index 8d436ad..dd7be55 100644 --- a/libxkutil/Makefile.am +++ b/libxkutil/Makefile.am @@ -7,6 +7,7 @@ noinst_HEADERS = \ cs_util.h \ misc_util.h \ device_parsing.h \ + capability_parsing.h \ xmlgen.h \ infostore.h \ pool_parsing.h \ @@ -20,6 +21,7 @@ libxkutil_la_SOURCES = \ cs_util_instance.c \ misc_util.c \ device_parsing.c \ + capability_parsing.c \ xmlgen.c \ infostore.c \ pool_parsing.c \ diff --git a/libxkutil/capability_parsing.c b/libxkutil/capability_parsing.c new file mode 100644 index 0000000..2185584 --- /dev/null +++ b/libxkutil/capability_parsing.c @@ -0,0 +1,462 @@ +/* + * Copyright IBM Corp. 2013 + * + * Authors: + * Boris Fiuczynski <fiuczy@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 <stdio.h> +#include <string.h> +#include <stdlib.h> +#include <stdbool.h> +#include <inttypes.h> +#include <sys/stat.h> +#include <stdint.h> + +#include <libcmpiutil/libcmpiutil.h> +#include <libvirt/libvirt.h> +#include <libxml/xpath.h> +#include <libxml/parser.h> +#include <libxml/tree.h> + +#include "misc_util.h" +#include "capability_parsing.h" +#include "xmlgen.h" +#include "../src/svpc_types.h" + +static void cleanup_cap_machine(struct cap_machine *machine) +{ + if (machine == NULL) + return; + free(machine->name); + free(machine->canonical_name); +} + +void cleanup_cap_domain_info(struct cap_domain_info *cgdi) +{ + int i; + if (cgdi == NULL) + return; + free(cgdi->emulator); + free(cgdi->loader); + for (i = 0; i < cgdi->num_machines; i++) + cleanup_cap_machine(&cgdi->machines[i]); + free(cgdi->machines); +} + +static void cleanup_cap_domain(struct cap_domain *cgd) +{ + if (cgd == NULL) + return; + free(cgd->typestr); + cleanup_cap_domain_info(&cgd->guest_domain_info); +} + +static void cleanup_cap_arch(struct cap_arch *cga) +{ + int i; + if (cga == NULL) + return; + free(cga->name); + cleanup_cap_domain_info(&cga->default_domain_info); + for (i = 0; i < cga->num_domains; i++) + cleanup_cap_domain(&cga->domains[i]); + free(cga->domains); +} + +void cleanup_cap_guest(struct cap_guest *cg) +{ + if (cg == NULL) + return; + free(cg->ostype); + cleanup_cap_arch(&cg->arch); +} + +static void extend_cap_machines(struct cap_domain_info *cg_domaininfo, + char *name, char *canonical_name) +{ + struct cap_machine *tmp_list = NULL; + tmp_list = realloc(cg_domaininfo->machines, + (cg_domaininfo->num_machines + 1) * + sizeof(struct cap_machine)); + + if (tmp_list == NULL) { + /* Nothing you can do. Just go on. */ + CU_DEBUG("Could not alloc space for " + "guest domain info list"); + return; + } + cg_domaininfo->machines = tmp_list; + + struct cap_machine *cap_gm = + &cg_domaininfo->machines[cg_domaininfo->num_machines]; + cap_gm->name = name; + cap_gm->canonical_name = canonical_name; + cg_domaininfo->num_machines++; +} + +static void parse_cap_domain_info(struct cap_domain_info *cg_domaininfo, + xmlNode *domain_child_node) +{ + CU_DEBUG("Capabilities guest domain info element node: %s", + domain_child_node->name); + + if (XSTREQ(domain_child_node->name, "emulator")) { + cg_domaininfo->emulator = + get_node_content(domain_child_node); + } else if (XSTREQ(domain_child_node->name, "loader")) { + cg_domaininfo->loader = + get_node_content(domain_child_node); + } else if (XSTREQ(domain_child_node->name, "machine")) { + extend_cap_machines(cg_domaininfo, + get_node_content(domain_child_node), + get_attr_value(domain_child_node, + "canonical")); + } +} + +static void parse_cap_domain(struct cap_domain *cg_domain, + xmlNode *guest_dom) +{ + CU_DEBUG("Capabilities guest domain node: %s", guest_dom->name); + + xmlNode *child; + + cg_domain->typestr = get_attr_value(guest_dom, "type"); + + for (child = guest_dom->children; child != NULL; child = child->next) + parse_cap_domain_info(&cg_domain->guest_domain_info, child); +} + +static void parse_cap_arch(struct cap_arch *cg_archinfo, + xmlNode *arch) +{ + CU_DEBUG("Capabilities arch node: %s", arch->name); + + xmlNode *child; + + cg_archinfo->name = get_attr_value(arch, "name"); + + for (child = arch->children; child != NULL; child = child->next) { + if (XSTREQ(child->name, "wordsize")) { + char *wordsize_str; + unsigned int wordsize; + wordsize_str = get_node_content(child); + /* Default to 0 wordsize if garbage */ + if (wordsize_str == NULL || + sscanf(wordsize_str, "%i", &wordsize) != 1) + wordsize = 0; + free(wordsize_str); + cg_archinfo->wordsize = wordsize; + } else if (XSTREQ(child->name, "domain")) { + struct cap_domain *tmp_list = NULL; + tmp_list = realloc(cg_archinfo->domains, + (cg_archinfo->num_domains + 1) * + sizeof(struct cap_domain)); + if (tmp_list == NULL) { + /* Nothing you can do. Just go on. */ + CU_DEBUG("Could not alloc space for " + "guest domain"); + continue; + } + memset(&tmp_list[cg_archinfo->num_domains], + 0, sizeof(struct cap_domain)); + cg_archinfo->domains = tmp_list; + parse_cap_domain(&cg_archinfo-> + domains[cg_archinfo->num_domains], + child); + cg_archinfo->num_domains++; + } else { + /* Check for the default domain child nodes */ + parse_cap_domain_info(&cg_archinfo->default_domain_info, + child); + } + } +} + +static void parse_cap_guests(xmlNodeSet *nsv, struct cap_guest **cap_guests) +{ + xmlNode **nodes = nsv->nodeTab; + xmlNode *child; + int numGuestNodes = nsv->nodeNr; + int i; + + for (i = 0; i < numGuestNodes; i++) { + for (child = nodes[i]->children; child != NULL; + child = child->next) { + if (XSTREQ(child->name, "os_type")) { + STRPROP(cap_guests[i], ostype, child); + } else if (XSTREQ(child->name, "arch")) { + parse_cap_arch(&cap_guests[i]->arch, child); + } + } + } +} + +static void compare_copy_domain_info_machines( + struct cap_domain_info *def_gdomi, + struct cap_domain_info *cap_gadomi) +{ + int i,j; + int org_l = cap_gadomi->num_machines; + char *cp_name = NULL; + char *cp_canonical_name = NULL; + bool found; + + for (i = 0; i < def_gdomi->num_machines; i++) { + found = false; + for (j = 0; j < org_l; j++) { + if (STREQC(def_gdomi->machines[i].name, + cap_gadomi->machines[j].name)) { + found = true; + continue; + /* found match => check next default */ + } + } + if (!found) { /* no match => insert default */ + cp_name = NULL; + cp_canonical_name = NULL; + if (def_gdomi->machines[i].name != NULL) + cp_name = strdup(def_gdomi->machines[i].name); + if (def_gdomi->machines[i].canonical_name != NULL) + cp_canonical_name = + strdup(def_gdomi-> + machines[i].canonical_name); + + extend_cap_machines(cap_gadomi, + cp_name, + cp_canonical_name); + } + } +} + +static void extend_defaults_cap_guests(struct capabilities *caps) +{ + struct cap_arch *cap_garch; + struct cap_domain_info *cap_gadomi; + struct cap_domain_info *def_gdomi; + int i,j; + + if (caps == NULL) + return; + + for (i = 0; i < caps->num_guests; i++) { + cap_garch = &caps->guests[i].arch; + def_gdomi = &cap_garch->default_domain_info; + + for (j = 0; j < cap_garch->num_domains; j++) { + /* compare guest_domain_info */ + cap_gadomi = &cap_garch->domains[j].guest_domain_info; + if (cap_gadomi->emulator == NULL && + def_gdomi->emulator != NULL) + cap_gadomi->emulator = + strdup(def_gdomi->emulator); + if (cap_gadomi->loader == NULL && + def_gdomi->loader != NULL) + cap_gadomi->loader = strdup(def_gdomi->loader); + + compare_copy_domain_info_machines(def_gdomi, + cap_gadomi); + } + } +} + +static int _get_cap_guests(const char *xml, struct capabilities *caps) +{ + int len; + + xmlDoc *xmldoc = NULL; + xmlXPathContext *xpathctx = NULL; + xmlXPathObject *xpathobj = NULL; + const xmlChar *xpathstr = (xmlChar *)"//capabilities//guest"; + xmlNodeSet *nsv; + + len = strlen(xml) + 1; + + if ((xmldoc = xmlParseMemory(xml, len)) == NULL) + goto err; + + if ((xpathctx = xmlXPathNewContext(xmldoc)) == NULL) + goto err; + + if ((xpathobj = xmlXPathEvalExpression(xpathstr, xpathctx)) == NULL) + goto err; + if (xmlXPathNodeSetIsEmpty(xpathobj->nodesetval)) { + CU_DEBUG("No capabilities guest nodes found!"); + goto err; + } + + nsv = xpathobj->nodesetval; + caps->guests = calloc(nsv->nodeNr, sizeof(struct cap_guest)); + if (caps->guests == NULL) + goto err; + caps->num_guests = nsv->nodeNr; + + parse_cap_guests(nsv, &caps->guests); + extend_defaults_cap_guests(caps); + return 1; + + err: + xmlXPathFreeObject(xpathobj); + xmlXPathFreeContext(xpathctx); + xmlFreeDoc(xmldoc); + return 0; +} + +int get_caps_from_xml(const char *xml, struct capabilities **caps) +{ + CU_DEBUG("In get_caps_from_xml"); + + *caps = calloc(1, sizeof(struct capabilities)); + if (*caps == NULL) + goto err; + + if (_get_cap_guests(xml, *caps) == 0) + goto err; + + return 1; + + err: + free(*caps); + *caps = NULL; + return 0; +} + +int get_capabilities(virConnectPtr conn, struct capabilities **caps) +{ + char *caps_xml = NULL; + int ret = 0; + + if (conn == NULL) { + CU_DEBUG("Unable to connect to libvirt."); + return 0; + } + + caps_xml = virConnectGetCapabilities(conn); + + if (caps_xml == NULL) { + CU_DEBUG("Unable to get capabilities xml."); + return 0; + } + + ret = get_caps_from_xml(caps_xml, caps); + + free(caps_xml); + + return ret; +} + +struct cap_domain_info *findDomainInfo(struct capabilities *caps, + const char *os_type, + const char *arch, + const char *domain_type) +{ + int i,j; + struct cap_arch *ar; + for (i = 0; i < caps->num_guests; i++) { + if (os_type == NULL || + STREQC(caps->guests[i].ostype, os_type)) { + ar = &caps->guests[i].arch; + if (arch == NULL || STREQC(ar->name,arch)) + for (j = 0; j < ar->num_domains; j++) + if (domain_type == NULL || + STREQC(ar->domains[j].typestr, + domain_type)) + return &ar->domains[j]. + guest_domain_info; + } + } + return NULL; +} + +char *get_default_arch(struct capabilities *caps, + const char *os_type) +{ + char *ret = NULL; + int i; + + if (caps != NULL) { + if (os_type == NULL) { /* pick first guest */ + if (caps->num_guests > 0) + ret = caps->guests[0].arch.name; + } else { /* search first matching guest */ + for (i = 0; i < caps->num_guests; i++) + if (STREQC(caps->guests[i].ostype, os_type)) { + ret = caps->guests[i].arch.name; + break; + } + } + } + return ret; +} + +char *get_default_machine( + struct capabilities *caps, + const char *os_type, + const char *arch, + const char *domain_type) +{ + char *ret = NULL; + struct cap_domain_info *di; + + if (caps != NULL) { + di = findDomainInfo(caps, os_type, arch, domain_type); + if (di != NULL && di->num_machines > 0) + ret = di->machines[0].canonical_name; + } + return ret; +} + +char *get_default_emulator(struct capabilities *caps, + const char *os_type, + const char *arch, + const char *domain_type) +{ + char *ret = NULL; + struct cap_domain_info *di; + + if (caps != NULL) { + di = findDomainInfo(caps, os_type, arch, domain_type); + if (di != NULL) + ret = di->emulator; + } + return ret; +} + +bool use_kvm(struct capabilities *caps) { + if (host_supports_kvm(caps) && !get_disable_kvm()) + return true; + else + return false; +} + +bool host_supports_kvm(struct capabilities *caps) +{ + bool kvm = false; + if (caps != NULL) + if (findDomainInfo(caps, NULL, NULL, "kvm") != NULL) + kvm = true; + return kvm; +} +/* + * Local Variables: + * mode: C + * c-set-style: "K&R" + * tab-width: 8 + * c-basic-offset: 8 + * indent-tabs-mode: nil + * End: + */ diff --git a/libxkutil/capability_parsing.h b/libxkutil/capability_parsing.h new file mode 100644 index 0000000..13af510 --- /dev/null +++ b/libxkutil/capability_parsing.h @@ -0,0 +1,93 @@ +/* + * Copyright IBM Corp. 2013 + * + * Authors: + * Boris Fiuczynski <fiuczy@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 + */ +#ifndef __CAPABILITY_PARSING_H +#define __CAPABILITY_PARSING_H + +#include <stdint.h> +#include <stdbool.h> + +struct cap_machine { + char *name; + char *canonical_name; +}; + +struct cap_domain_info { + char *emulator; + char *loader; + int num_machines; + struct cap_machine *machines; +}; + +struct cap_domain { + char *typestr; + struct cap_domain_info guest_domain_info; +}; + +struct cap_arch { + char *name; + unsigned int wordsize; + struct cap_domain_info default_domain_info; + int num_domains; + struct cap_domain *domains; +}; + +struct cap_guest { + char *ostype; + struct cap_arch arch; +}; + +struct capabilities { + int num_guests; + struct cap_guest *guests; +}; + +int get_caps_from_xml(const char *xml, struct capabilities **caps); +int get_capabilities(virConnectPtr conn, struct capabilities **caps); +char *get_default_arch(struct capabilities *caps, + const char *os_type); +char *get_default_machine(struct capabilities *caps, + const char *os_type, + const char *arch, + const char *domain_type); +char *get_default_emulator(struct capabilities *caps, + const char *os_type, + const char *arch, + const char *domain_type); +struct cap_domain_info *findDomainInfo(struct capabilities *caps, + const char *os_type, + const char *arch, + const char *domain_type); +bool use_kvm(struct capabilities *caps); +bool host_supports_kvm(struct capabilities *caps); +void cleanup_cap_guest(struct cap_guest *cg); +void cleanup_cap_domain_info(struct cap_domain_info *cgdi); + +#endif + +/* + * Local Variables: + * mode: C + * c-set-style: "K&R" + * tab-width: 8 + * c-basic-offset: 8 + * indent-tabs-mode: nil + * End: + */ diff --git a/libxkutil/xml_parse_test.c b/libxkutil/xml_parse_test.c index 384593d..5c5f2df 100644 --- a/libxkutil/xml_parse_test.c +++ b/libxkutil/xml_parse_test.c @@ -6,6 +6,7 @@ #include <libvirt/libvirt.h>

#include "device_parsing.h" +#include "capability_parsing.h" #include "xmlgen.h"

static void print_value(FILE *d, const char *name, const char *val) @@ -183,6 +184,91 @@ static char *read_from_file(FILE *file) return xml; }

+static void print_cap_domain_info(struct cap_domain_info *capgdiinfo, + FILE *d) +{ + struct cap_machine capgminfo; + int i; + + if (capgdiinfo==NULL) + return; + + if (capgdiinfo->emulator!=NULL) + print_value(d, " Emulator", capgdiinfo->emulator); + if (capgdiinfo->loader!=NULL) + print_value(d, " Loader", capgdiinfo->loader); + for (i=0; i<capgdiinfo->num_machines; i++) { + capgminfo = capgdiinfo->machines[i]; + fprintf(d, " Machine name : %-15s canonical name : %s\n", + capgminfo.name, capgminfo.canonical_name); + } + fprintf(d, "\n"); +} + +static void print_cap_domains(struct cap_arch caparchinfo, + FILE *d) +{ + struct cap_domain capgdinfo; + int i; + for (i=0; i<caparchinfo.num_domains; i++) { + capgdinfo = caparchinfo.domains[i]; + print_value(d, " Type", capgdinfo.typestr); + print_cap_domain_info(&capgdinfo.guest_domain_info, d); + } +} + +static void print_cap_arch(struct cap_arch caparchinfo, + FILE *d) +{ + print_value(d, " Arch name", caparchinfo.name); + fprintf(d, " Arch wordsize : %i\n", caparchinfo.wordsize); + fprintf(d, "\n -- Default guest domain settings --\n"); + print_cap_domain_info(&caparchinfo.default_domain_info, d); + fprintf(d, " -- Guest domains (%i) --\n", caparchinfo.num_domains); + print_cap_domains(caparchinfo, d); +} + +static void print_cap_guest(struct cap_guest *capginfo, + FILE *d) +{ + print_value(d, "Guest OS type", capginfo->ostype); + print_cap_arch(capginfo->arch, d); +} + +static void print_capabilities(struct capabilities *capsinfo, + FILE *d) +{ + int i; + fprintf(d, "\n### Capabilities ###\n"); + fprintf(d, "-- Guest (%i) --\n", capsinfo->num_guests); + for (i=0; i<capsinfo->num_guests; i++) { + print_cap_guest(&capsinfo->guests[i], d); + } +} + +static int capinfo_for_dom(const char *uri, + struct domain *dominfo, + struct capabilities **capsinfo) +{ + virConnectPtr conn = NULL; + char *caps_xml = NULL; + int ret = 0; + + conn = virConnectOpen(uri); + if (conn == NULL) { + printf("Unable to connect to libvirt\n"); + goto out; + } + + ret = get_capabilities(conn, capsinfo); + + out: + free(caps_xml); + virConnectClose(conn); + + return ret; +} + static int dominfo_from_dom(const char *uri, const char *domain, struct domain **d) @@ -246,12 +332,13 @@ static int dominfo_from_file(const char *fname, struct domain **d) static void usage(void) { printf("xml_parse_test -f [FILE | -] [--xml]\n" - "xml_parse_test -d domain [--uri URI] [--xml]\n" + "xml_parse_test -d domain [--uri URI] [--xml] [--cap]\n" "\n" "-f,--file FILE Parse domain XML from file (or stdin if -)\n" "-d,--domain DOM Display dominfo for a domain from libvirt\n" "-u,--uri URI Connect to libvirt with URI\n" "-x,--xml Dump generated XML instead of summary\n" + "-c,--cap Display the libvirt default capability values for the specified domain\n" "-h,--help Display this help message\n"); }

@@ -262,7 +349,10 @@ int main(int argc, char **argv) char *uri = "xen"; char *file = NULL; bool xml = false; + bool cap = false; struct domain *dominfo = NULL; + struct capabilities *capsinfo = NULL; + struct cap_domain_info *capgdinfo = NULL; int ret;

static struct option lopts[] = { @@ -270,13 +360,14 @@ int main(int argc, char **argv) {"uri", 1, 0, 'u'}, {"xml", 0, 0, 'x'}, {"file", 1, 0, 'f'}, + {"cap", 0, 0, 'c'}, {"help", 0, 0, 'h'}, {0, 0, 0, 0}};

while (1) { int optidx = 0;

- c = getopt_long(argc, argv, "d:u:f:xh", lopts, &optidx); + c = getopt_long(argc, argv, "d:u:f:xch", lopts, &optidx); if (c == -1) break;

@@ -297,11 +388,14 @@ int main(int argc, char **argv) xml = true; break;

+ case 'c': + cap = true; + break; + case '?': case 'h': usage(); return c == '?'; - }; }

@@ -326,6 +420,40 @@ int main(int argc, char **argv) print_devices(dominfo, stdout); }

+ if (cap && file == NULL) { + ret = capinfo_for_dom(uri, dominfo, &capsinfo); + if (ret == 0) { + printf("Unable to get capsinfo\n"); + return 3; + } else { + print_capabilities(capsinfo, stdout); + + fprintf(stdout, "-- Default Arch is: %s\n", + get_default_arch(capsinfo,NULL)); + fprintf(stdout, "-- Default Machine is: %s\n", + get_default_machine(capsinfo,NULL,NULL,NULL)); + fprintf(stdout, "-- Default Emulator is: %s\n", + get_default_emulator(capsinfo,NULL,NULL,NULL)); + fprintf(stdout, "-- Default Machine for domain type=kvm : %s\n", + get_default_machine(capsinfo,NULL,NULL,"kvm")); + fprintf(stdout, "-- Default Emulator for domain type=kvm : %s\n", + get_default_emulator(capsinfo,NULL,NULL,"kvm")); + + fprintf(stdout, "\n-- Default Domain Search for: \n" + "guest type=hvm - guest arch=* - guest domain type=kvm\n"); + capgdinfo = findDomainInfo(capsinfo, "hvm", NULL, "kvm"); + print_cap_domain_info(capgdinfo, stdout); + + fprintf(stdout, "-- Default Domain Search for: \n" + "guest type=* - guest arch=* - guest domain type=*\n"); + capgdinfo = findDomainInfo(capsinfo, NULL, NULL, NULL); + print_cap_domain_info(capgdinfo, stdout); + } + } else if (cap) { + printf("Need a data source (--domain) to get default capabilities\n"); + return 4; + } + return 0; }

On 08/15/2013 10:48 AM, Viktor Mihajlovski wrote:

From: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>

In the DefineSystem call the architecture, machine and emulator for KVM are set to the hypervisor-specific default values if they did not get provided. This now allows architecture based decision making in the CIM providers to work for all platforms.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/Virt_VirtualSystemManagementService.c | 130 ++++++++++++----------------- 1 file changed, 55 insertions(+), 75 deletions(-)

This patch had some coverity - resource_leak issues... See below And fails cimtest miserably... I'll dig some more later to see if I find something, but I have to leave for the day now...

diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 3fad33b..b0d81d1 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -35,6 +35,7 @@ #include "cs_util.h" #include "misc_util.h" #include "device_parsing.h" +#include "capability_parsing.h" #include "xmlgen.h"

#include <libcmpiutil/libcmpiutil.h> @@ -388,59 +389,6 @@ static bool fv_set_emulator(struct domain *domain, return true; }

-static bool system_has_kvm(const char *pfx) -{ - CMPIStatus s; - virConnectPtr conn = NULL; - char *caps = NULL; - bool disable_kvm = get_disable_kvm(); - xmlDocPtr doc = NULL; - xmlNodePtr node = NULL; - int len; - bool kvm = false; - - /* sometimes disable KVM to avoid problem in nested KVM */ - if (disable_kvm) { - CU_DEBUG("Enter disable kvm mode!"); - goto out; - } - - conn = connect_by_classname(_BROKER, pfx, &s); - if ((conn == NULL) || (s.rc != CMPI_RC_OK)) { - goto out; - } - - caps = virConnectGetCapabilities(conn); - if (caps != NULL) { - len = strlen(caps) + 1; - - doc = xmlParseMemory(caps, len); - if (doc == NULL) { - CU_DEBUG("xmlParseMemory() call failed!"); - goto out; - } - - node = xmlDocGetRootElement(doc); - if (node == NULL) { - CU_DEBUG("xmlDocGetRootElement() call failed!"); - goto out; - } - - if (has_kvm_domain_type(node)) { - CU_DEBUG("The system support kvm!"); - kvm = true; - } - } - -out: - free(caps); - free(doc); - - virConnectClose(conn); - - return kvm; -} - static int bootord_vssd_to_domain(CMPIInstance *inst, struct domain *domain) { @@ -511,13 +459,17 @@ static int bootord_vssd_to_domain(CMPIInstance *inst,

static int fv_vssd_to_domain(CMPIInstance *inst, struct domain *domain, - const char *pfx) + const char *pfx, + virConnectPtr conn) { int ret; const char *val; + struct capabilities *capsinfo = NULL; + + get_capabilities(conn, &capsinfo);

(1) Event alloc_arg: "get_capabilities(virConnectPtr, struct capabilities **)" allocates memory that is stored into "capsinfo". [details] Also see events: [leaked_storage] Secondary to that if get_capabilities() fails, then there's bound to be some nasty failures after this...

if (STREQC(pfx, "KVM")) { - if (system_has_kvm(pfx)) + if (use_kvm(capsinfo)) domain->type = DOMAIN_KVM; else domain->type = DOMAIN_QEMU;

... 478 } else { 479 CU_DEBUG("Unknown fullvirt domain type: %s", pfx); (6) Event leaked_storage: Variable "capsinfo" going out of scope leaks the storage it points to. Also see events: [alloc_arg] 480 return 0; 481 } ... 'capsinfo' is leaked

@@ -532,30 +484,49 @@ static int fv_vssd_to_domain(CMPIInstance *inst, if (ret != 1) return 0;

Same here

- ret = cu_get_str_prop(inst, "Emulator", &val); - if (ret != CMPI_RC_OK) - val = NULL; - else if (disk_type_from_file(val) == DISK_UNKNOWN) { - CU_DEBUG("Emulator path does not exist: %s", val); - return 0; - } - - if (!fv_set_emulator(domain, val)) - return 0; - free(domain->os_info.fv.arch); ret = cu_get_str_prop(inst, "Arch", &val); - if (ret == CMPI_RC_OK) + if (ret != CMPI_RC_OK) { + if (capsinfo != NULL) { /* set default */ + val = get_default_arch(capsinfo, "hvm"); + CU_DEBUG("Set Arch to default: %s", val); + } else + val = NULL; + } + if (val != NULL) domain->os_info.fv.arch = strdup(val); - else - domain->os_info.fv.arch = NULL;

free(domain->os_info.fv.machine); ret = cu_get_str_prop(inst, "Machine", &val); - if (ret == CMPI_RC_OK) + if (ret != CMPI_RC_OK) { + if (capsinfo != NULL) { /* set default */ + val = get_default_machine(capsinfo, "hvm", + domain->os_info.fv.arch, + "kvm"); + CU_DEBUG("Set Machine to default: %s", val); + } else + val = NULL; + } + if (val != NULL) domain->os_info.fv.machine = strdup(val); - else - domain->os_info.fv.machine = NULL; + + ret = cu_get_str_prop(inst, "Emulator", &val); + if (ret != CMPI_RC_OK) { + if (capsinfo != NULL) { /* set default */ + val = get_default_emulator(capsinfo, "hvm", + domain->os_info.fv.arch, + "kvm"); + CU_DEBUG("Set Emulator to default: %s", val); + } else + val = NULL; + } + if (val != NULL && disk_type_from_file(val) == DISK_UNKNOWN) { + CU_DEBUG("Emulator path does not exist: %s", val); + return 0;

Same here

+ } + + if (!fv_set_emulator(domain, val)) + return 0;

Same here

return 1; } @@ -663,6 +634,8 @@ static int vssd_to_domain(CMPIInstance *inst, bool bool_val; bool fullvirt; CMPIObjectPath *opathp = NULL; + virConnectPtr conn = NULL; + CMPIStatus s = { CMPI_RC_OK, NULL };

opathp = CMGetObjectPath(inst, NULL); @@ -748,9 +721,16 @@ static int vssd_to_domain(CMPIInstance *inst, } }

- if (fullvirt || STREQC(pfx, "KVM")) - ret = fv_vssd_to_domain(inst, domain, pfx); - else if (STREQC(pfx, "Xen")) + if (fullvirt || STREQC(pfx, "KVM")) { + conn = connect_by_classname(_BROKER, cn, &s); + if (conn == NULL || (s.rc != CMPI_RC_OK)) { + CU_DEBUG("libvirt connection failed"); + ret = 0; + goto out; + } + ret = fv_vssd_to_domain(inst, domain, pfx, conn); + virConnectClose(conn); + } else if (STREQC(pfx, "Xen")) ret = xenpv_vssd_to_domain(inst, domain); else if (STREQC(pfx, "LXC")) ret = lxc_vssd_to_domain(inst, domain);