Re: [Libvirt-cim] [PATCH 1/4] VSSD: Add properties for arch and machine

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.

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(a)linux.vnet.ibm.com&gt; --- 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;

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; + };

[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; }