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

于 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(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; "machine = NULL;" is missing? Otherwise dominfo->os_info.fv.machine 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; > } >