[Libvirt-cim] [PATCH 1/4] VSSD: Add properties for arch and machine
Wenchao Xia
xiawenc at linux.vnet.ibm.com
Mon Aug 19 03:31:24 UTC 2013
于 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 at 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.
> 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
More information about the Libvirt-cim
mailing list