δΊ 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.
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
Thus will make commit history clear and easier to review. point to a value which is freed later.
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